Activated profiles should not always deactivate POM profiles that are active by default #4270

crankydillo · 2024-06-18T21:30:58Z

These versions resolve with Maven. If I'm misuing the test APIs, I can create a sample project.

Maven settings with profiles fails to active other profiles from parent activeByDefault #4269

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java

crankydillo · 2024-06-29T12:02:31Z

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java

@@ -1267,9 +1262,7 @@ void activeByDefault() {
                        </profile>
                    </profiles>
                </project>
-                """, spec -> {
-                  ((MavenParser.Builder) spec.getParser()).activeProfiles(activeProfile);


When you remove this, it no longer seems to mimic what I see when I really try to use rewrite in this scenario. In other words, the problematic code (or at least I think it's problematic:) is not exposed. I totally get what you did in this change, and it's what I first tried. However, when evaluation gets to where it determines if a profile is active, active profile is not foobar. It's empty, which does not expose the issue.

Ah sorry about that! Thanks for keeping a sharp eye out. What's the current status of this PR now then? As I see a todo comment and draft status. Anything you'd like feedback on or plan to continue? Just so I know what to review/merge, or wait for.

Hi @timtebeek . I was kind of hoping you all would run with it, but I may be able to finish this up. I'm just back from vacation.

Is it safe to assume that your team agrees #4269 actually is a bug? :)

It does indeed seem like we should have more profiles marked as active due to activeByDefault, even if a settings.xml explicitly lists some profiles already. Hard to fully dive into this for me these past couple weeks, but that's what I'm getting from reading your issue & code changes here.

Thanks! We plan to pick this back up soon.

crankydillo · 2024-07-18T18:19:10Z

Hi. We're back on this.

Has there been any work on profile deactivation: mvn -P-activated-profile or mvn -D '!activated-profile, where those profiles would not be tagged as active even if they would be activated in some other way (e.g. activeByDefault)? I have changed some code in rewrite-maven, and my tests, which do not explore this idea, pass and no existing one fails. However, when I add tests around disabling profiles those fail as it still views them as active. I can see the flaws in my code about this, but I haven't found any old code that handles this.

I searched a bit in PRs and issues. 'profile deactivation', 'profile disabling', and 'deactivation' didn't turn up anything.

crankydillo · 2024-07-18T19:17:40Z

@timtebeek I pinky promise not to sidetrack (far) on this, but got a quick solve for this (and in -java-17, -java-21)?

$ ./gradlew :rewrite-java-11:build
Starting a Gradle Daemon, 1 busy and 1 incompatible Daemons could not be reused, use --status for details

> Configure project :
Inferred project: rewrite, version: 8.29.0-SNAPSHOT

FAILURE: Build failed with an exception.

* What went wrong:
Could not determine the dependencies of task ':rewrite-java-11:compatibilityTest'.
> Could not resolve all dependencies for configuration ':rewrite-java-11:compatibilityTestRuntimeClasspath'.
   > Could not find org.junit.platform:junit-platform-launcher:.
     Required by:
         project :rewrite-java-11

My skills, internet, AI, and colleagues have all failed so far. We're primarily a Maven shop. Trying to regress before pushing the changes. I did check your build guidance.

crankydillo · 2024-07-19T12:02:09Z

rewrite-maven/src/main/java/org/openrewrite/maven/MavenSettings.java

@@ -83,6 +84,29 @@ public MavenSettings(@Nullable String localRepository, @Nullable Profiles profil
        this.servers = servers;
    }

+    public List<Profile> activeProfiles(final Iterable<String> userSpecifiedProfiles) {


I made the code a little more production-ready. I did copy this, due to my ignorance with lombok, between the model meant for POM files and the model meant for Maven settings filesI need to add tests for the changes that deal with Maven settings.xml profiles, but am hoping to get a bit of feedback on this approach.

@timtebeek I pushed one way to isolate code in f9bea1a. I'm far from happy with it. I'll gladly remove/revert that commit if you are OK with copying this activeProfiles algorithm in 2 places. Just let me know.

timtebeek · 2024-07-19T12:07:21Z

Hi @crankydillo I've been traveling since Wednesday, getting home Sunday. I'll try to have a more detailed look next week. As for your IDE setup, one tip is to create a copy of this file to limit what modules are loaded. That could help speed up development as well.

rewrite/IDE.properties.tmp

Lines 1 to 7 in 537d023

    
           ######################################################## 
        
           # If you are only working on a subset of the modules in this project, you can optimize your 
        
           # IDE to only load those modules. Copy `IDE.properties.tmp` to `IDE.properties` and comment out 
        
           # any lines that correspond to modules you do not want to work on. This will cause Gradle to 
        
           # swap those project dependencies for binary dependencies resolved from either Maven local or 
        
           # the OSS snapshots repository, and speed up your IDE. 
        
           ########################################################

I don't recall any work towards explicit profile deactivation. What kind of usage patterns do you see there beyond what Maven already does for us when you execute the rewrite-maven-plugin with those profiles enabled/disabled?

crankydillo · 2024-07-23T11:13:30Z

I don't recall any work towards explicit profile deactivation. What kind of usage patterns do you see there beyond what Maven already does for us when you execute the rewrite-maven-plugin with those profiles enabled/disabled?

Sorry, I missed this question. It's probably best for me to just add a test for this. I'll get that done soon. I don't think I pushed it as part of this PR (because it currently fails:).

Update: I have already pushed it. It's just @Disabled. You can find it here. Got to be honest, this isn't a big use case for us, at least not at this time. I just don't want to break existing, correct behavior in this case.

github-actions

Some suggestions could not be made:

rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36

timtebeek · 2024-07-25T15:04:13Z

rewrite-maven/src/main/java/org/openrewrite/maven/tree/Profile.java

-    public boolean isActive(Iterable<String> activeProfiles) {
-        return ProfileActivation.isActive(id, activeProfiles, activation);


In general we're fairly conservative with changes to any tree.* classes, as these are also serialized, with a more high impact change for our users. Is there anything we can do to restore the old methods to minimize the impact?

Sorry @timtebeek I didn't see this. Let me check!

We're talking about standard object serialization, right? I didn't think methods like the ones I removed/added played a role in that. I thought it was more about state.

Regardless, let me see what I can easily put back.

It's a little more involved, but ideally we minimize changes to the API where we can. Thanks for having a look!

Makes sense. I should have thought about public API. I pushed some changes. Let me know what you think.

Any ideas on how we can help with this? We have some workarounds, but they don't work quite as well as we thought.

Apologies for the delayed response; it's hard to get to everything these days. I think to start revert the implements WithProfiles on the ..tree.* classes here would help get this in a state that's closer to review & merge. Not sure about what the exact replacement should be, but that would help restart this, along with resolving the conflicts.

I understand. I will see if I can get rid of the interface. I know I can if I just copy code around. I looked at Trait, but if there's an obvious application here, I'm not seeing it. Seems oriented around a cursor, and that doesn't seem to be playing a role.

Let me see what I can come up with over the next few days.

Ok, I pushed a change that does not involve interfaces. Unfortunately, I think Maven settings will still have an issue (the interface-approach tried to deal with that). However, I don't think that Maven settings problem affects us. I'll have to verify.

github-actions

Some suggestions could not be made:

rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36

timtebeek · 2024-07-30T20:55:29Z

Thanks for the fresh work on this @crankydillo ; let me know when this is ready for review, as currently it's still marked as a draft.

crankydillo · 2024-07-30T22:39:43Z

I have it as draft, because there's still a bit of work (see here). I'll finish that up if you think the approach looks good.

Update: I'll just go ahead and do that. Should have it out of draft by tomorrow!

rewrite-maven/src/main/java/org/openrewrite/maven/WithProfiles.java

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java

github-actions · 2024-11-02T15:20:37Z

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java

+            mavenCtx.setMavenSettings(settings);
+
+            rewriteRun(recipeSpec -> recipeSpec.executionContext(mavenCtx),
+              pomXml("""


Suggested change

pomXml("""

pomXml(

"""

crankydillo · 2024-11-02T15:22:37Z

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java

+            assertThat(err.getMessage()).contains("Problem parsing a/pom.xml");  // brittle:(, but class above is broad
+        }
+
+        @Issue("TODO: create issue")


This test is related to my understanding of this Maven doc. It is related, but not the same as what I've written up in #4269, so thinking about creating a new issue for it. If you think I've misunderstood something or this test is invalid, we can just remove. This is not a current use-case for us.

timtebeek · 2024-11-02T15:26:30Z

Thanks for the rework @crankydillo ! Much appreciate the narrower scope as that makes this easier to review. Quick headsup though that I'll be mostly out this coming week ahead of JFall, but hope to revisit the week after.

crankydillo · 2024-12-17T13:56:56Z

:bump

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java

…est.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

crankydillo mentioned this pull request Jun 18, 2024

Maven settings with profiles fails to active other profiles from parent activeByDefault #4269

Open

timtebeek self-requested a review June 19, 2024 11:14

timtebeek self-assigned this Jun 19, 2024

timtebeek marked this pull request as draft June 19, 2024 11:14

timtebeek reviewed Jun 22, 2024

View reviewed changes

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java Outdated Show resolved Hide resolved

github-actions bot reviewed Jun 22, 2024

View reviewed changes

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java Outdated Show resolved Hide resolved

timtebeek mentioned this pull request Jun 23, 2024

FAQ Entry on "How do I remove an element from the LST?" openrewrite/rewrite-docs#290

Closed

crankydillo commented Jun 29, 2024

View reviewed changes

timtebeek added the bug Something isn't working label Jul 8, 2024

timtebeek assigned crankydillo Jul 8, 2024

crankydillo force-pushed the 4269_maven-activebydefault-profiles branch 2 times, most recently from 8e78595 to 494402f Compare July 18, 2024 22:27

crankydillo changed the title ~~Disabled test showing issues with profiles and activeByDefault=true~~ Activated profiles should not always deactivate POM profiles that are active by default Jul 18, 2024

crankydillo commented Jul 19, 2024

View reviewed changes

timtebeek self-requested a review July 19, 2024 13:46

github-actions bot reviewed Jul 25, 2024

View reviewed changes

timtebeek reviewed Jul 25, 2024

View reviewed changes

crankydillo force-pushed the 4269_maven-activebydefault-profiles branch 2 times, most recently from 288f3ef to 910f121 Compare July 30, 2024 01:05

github-actions bot reviewed Jul 30, 2024

View reviewed changes

crankydillo force-pushed the 4269_maven-activebydefault-profiles branch 3 times, most recently from 407616b to 1db298d Compare July 31, 2024 03:12

crankydillo marked this pull request as ready for review July 31, 2024 11:57

github-actions bot reviewed Aug 7, 2024

View reviewed changes

rewrite-maven/src/main/java/org/openrewrite/maven/WithProfiles.java Outdated Show resolved Hide resolved

timtebeek mentioned this pull request Aug 22, 2024

Do not merge in parent dependency where child overrides the version or scope #4434

Merged

3 tasks

crankydillo force-pushed the 4269_maven-activebydefault-profiles branch from 00da607 to 0e58942 Compare October 28, 2024 12:50

github-actions bot reviewed Oct 28, 2024

View reviewed changes

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java Outdated Show resolved Hide resolved

crankydillo force-pushed the 4269_maven-activebydefault-profiles branch from 0e58942 to 5a40cf9 Compare November 1, 2024 18:17

crankydillo added 3 commits November 1, 2024 14:19

[4269] partial

01f91a0

Make activeProfiles method package private.

400bd51

Tweaks to test and add test showing issue with settings active profiles

1f36d4d

crankydillo force-pushed the 4269_maven-activebydefault-profiles branch from 5a40cf9 to 1f36d4d Compare November 2, 2024 15:16

github-actions bot reviewed Nov 2, 2024

View reviewed changes

crankydillo commented Nov 2, 2024

View reviewed changes

Merge branch 'main' into 4269_maven-activebydefault-profiles

7533efc

timtebeek self-requested a review December 17, 2024 23:47

github-actions bot reviewed Dec 17, 2024

View reviewed changes

rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java Outdated Show resolved Hide resolved

crankydillo and others added 2 commits December 18, 2024 11:52

Update rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserT…

36656de

…est.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into 4269_maven-activebydefault-profiles

e5f0689

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Activated profiles should not always deactivate POM profiles that are active by default #4270

Activated profiles should not always deactivate POM profiles that are active by default #4270

crankydillo commented Jun 18, 2024 •

edited by timtebeek

Loading

crankydillo Jun 29, 2024

timtebeek Jul 8, 2024

crankydillo Jul 10, 2024 •

edited

Loading

timtebeek Jul 15, 2024

crankydillo Jul 16, 2024

crankydillo commented Jul 18, 2024

crankydillo commented Jul 18, 2024 •

edited

Loading

crankydillo Jul 19, 2024

crankydillo Jul 31, 2024

timtebeek commented Jul 19, 2024 •

edited

Loading

crankydillo commented Jul 23, 2024 •

edited

Loading

github-actions bot left a comment

timtebeek Jul 25, 2024

crankydillo Jul 29, 2024

crankydillo Jul 29, 2024

timtebeek Jul 29, 2024

crankydillo Jul 30, 2024

crankydillo Sep 16, 2024

crankydillo Oct 26, 2024

timtebeek Oct 27, 2024

crankydillo Oct 28, 2024 •

edited

Loading

crankydillo Nov 2, 2024 •

edited

Loading

github-actions bot left a comment

timtebeek commented Jul 30, 2024

crankydillo commented Jul 30, 2024 •

edited

Loading

github-actions bot Nov 2, 2024

crankydillo Nov 2, 2024

timtebeek commented Nov 2, 2024

crankydillo commented Dec 17, 2024

		public boolean isActive(Iterable<String> activeProfiles) {
		return ProfileActivation.isActive(id, activeProfiles, activation);

Activated profiles should not always deactivate POM profiles that are active by default #4270

Are you sure you want to change the base?

Activated profiles should not always deactivate POM profiles that are active by default #4270

Conversation

crankydillo commented Jun 18, 2024 • edited by timtebeek Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crankydillo Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crankydillo commented Jul 18, 2024

crankydillo commented Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timtebeek commented Jul 19, 2024 • edited Loading

crankydillo commented Jul 23, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crankydillo Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

crankydillo Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

timtebeek commented Jul 30, 2024

crankydillo commented Jul 30, 2024 • edited Loading

github-actions bot Nov 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timtebeek commented Nov 2, 2024

crankydillo commented Dec 17, 2024

crankydillo commented Jun 18, 2024 •

edited by timtebeek

Loading

crankydillo Jul 10, 2024 •

edited

Loading

crankydillo commented Jul 18, 2024 •

edited

Loading

timtebeek commented Jul 19, 2024 •

edited

Loading

crankydillo commented Jul 23, 2024 •

edited

Loading

crankydillo Oct 28, 2024 •

edited

Loading

crankydillo Nov 2, 2024 •

edited

Loading

crankydillo commented Jul 30, 2024 •

edited

Loading