Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maven profiles, active profiles, and activeByDefault #4269

Open
crankydillo opened this issue Jun 18, 2024 · 10 comments
Open

Maven profiles, active profiles, and activeByDefault #4269

crankydillo opened this issue Jun 18, 2024 · 10 comments
Labels
bug Something isn't working question Further information is requested

Comments

@crankydillo
Copy link

crankydillo commented Jun 18, 2024

We have a settings.xml file that has an activeProfile specified. We have many parent pom files that have profiles with <activeByDefault>true</active>. We primarily use this to group sets of managed dependencies. All of this builds and works fine with Maven; however, it does not with openrewrite.

My read of Maven's doc on activeByDefault is the activeByDefault=true profiles should be active in poms that do not have a profile with the same id as the active one specified in our settings.xml.

What version of OpenRewrite are you using?

  • OpenRewrite v8.27.4
  • Maven/Gradle plugin v5.33.0
  • rewrite module

How are you running OpenRewrite?

Maven plugin + https://github.com/openrewrite/rewrite/blob/main/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java

What is the smallest, simplest way to reproduce the problem?

TEST PR. If I've misunderstood some of your testing libs, I can give a sample project that will show the problem.

What did you expect to see?

Test to pass (maven dependency versions resolved)

What did you see instead?

Failure to resolve Maven dependency versions

Are you interested in contributing a fix to OpenRewrite?

Took a peek, but may bit a bit too extensive for how much time I have...

@crankydillo crankydillo added the bug Something isn't working label Jun 18, 2024
crankydillo added a commit to crankydillo/rewrite that referenced this issue Jun 18, 2024
These versions resolve with Maven.  If I'm misuing the test APIs, I can
create a sample project.

 openrewrite#4269
crankydillo added a commit to crankydillo/rewrite that referenced this issue Jun 18, 2024
These versions resolve with Maven.  If I'm misuing the test APIs, I can
create a sample project.

 openrewrite#4269
@crankydillo
Copy link
Author

What do you think @timtebeek ? Does this look like a bug to you? If it's not high on your priority list, we can work on it. We just want to make sure you agree it's a bug before doing that.

@timtebeek
Copy link
Contributor

Hi! I've pushed a small change to your reproducer to make that pass locally; I think the order of the pom files matters for the tests.

Might there be anything else needed to replicate the issue you're seeing locally?

@timtebeek timtebeek added the question Further information is requested label Jun 25, 2024
@crankydillo
Copy link
Author

I will continue to work on this, but the intent is to mimic behavior when a Maven settings file has a section like this:

<activeProfiles>
       <activeProfile>foobar</activeProfile>
</activeProfiles>

When I try to perform an actual rewrite on a real project, ResolvePom#activeProfiles will be foobar and that seems to throw everything off. I will soon push a change that I think may represent this with these test-based rewrite libs, but I'm still learning how to work with those, so if you can help me represent in the test what I describe above correctly, I would appreciate it:)

Alternatively, I can push a project-based adhoc reproducer. I mean mimic what I'm really trying to do..

@timtebeek
Copy link
Contributor

Perhaps seeing the implementation of this method will help you more faithfully reproduce what you're after with a unit test.

static void customizeExecutionContext(ExecutionContext ctx) {
if(MavenSettings.readFromDiskEnabled()) {
MavenExecutionContextView mctx = MavenExecutionContextView.view(ctx);
mctx.setMavenSettings(MavenSettings.readMavenSettingsFromDisk(mctx));
}
}

crankydillo pushed a commit to crankydillo/rewrite that referenced this issue Jun 25, 2024
@crankydillo
Copy link
Author

Perhaps seeing the implementation of this method will help you more faithfully reproduce what you're after with a unit test.

static void customizeExecutionContext(ExecutionContext ctx) {
if(MavenSettings.readFromDiskEnabled()) {
MavenExecutionContextView mctx = MavenExecutionContextView.view(ctx);
mctx.setMavenSettings(MavenSettings.readMavenSettingsFromDisk(mctx));
}
}

I have been trying some variations of that:). I did push changes that I hope does better with this commit. Let me know if there's a better way to craft the test or you need me to submit an adhoc project.

crankydillo pushed a commit to crankydillo/rewrite that referenced this issue Jun 25, 2024
crankydillo pushed a commit to crankydillo/rewrite that referenced this issue Jun 25, 2024
crankydillo pushed a commit to crankydillo/rewrite that referenced this issue Jun 25, 2024
@crankydillo
Copy link
Author

I went ahead and created an adhoc test here. Apologies for hijacking your repo. I do have a reason:)

crankydillo pushed a commit to crankydillo/rewrite that referenced this issue Jun 26, 2024
crankydillo pushed a commit to crankydillo/rewrite that referenced this issue Jun 26, 2024
crankydillo pushed a commit to crankydillo/rewrite that referenced this issue Jun 26, 2024
crankydillo pushed a commit to crankydillo/rewrite that referenced this issue Jun 26, 2024
@crankydillo
Copy link
Author

Please ignore the 'Another adhoc' + 3 following commits above. That is a different issue I'm having. Sorry about that.

@crankydillo
Copy link
Author

crankydillo commented Jun 28, 2024

@timtebeek Sorry to pester, but I am:) Did you take a peek? I have an idea on a fix, but don't want to start that if you don't see this as an issue. Of course, we'd love it if you could fix or let us know where this lands on the priority list.

@timtebeek
Copy link
Contributor

Hi @crankydillo ; Apologies for the delayed response; it's at times hard to get to everything.

I've pushed yet another change to your PR to (I think) set the active profile correctly. That again makes the test pass. With that test passing I feel I still don't fully understand the issue you're having, and how settings.xml factors in.

I can see the various pom.xml files you have, and the profiles in those, and how you set a different active profile "foobar". I'd expect that to still keep active-profile-1 and active-profile-2 active, but perhaps you have other ideas.

Any help towards solving a problem is of course appreciated, and perhaps helps explain the issue too, but for now it's a bit unclear to me still. Please note that you're always welcome in our Slack as well, if you'd prefer a more synchronous response.

@crankydillo
Copy link
Author

crankydillo commented Jun 29, 2024

Hi @crankydillo ; Apologies for the delayed response; it's at times hard to get to everything.

No need to apologize @timtebeek and thanks for reminder about slack:). Very appreciative of you looking at this!

I've pushed yet another change to your PR to (I think) set the active profile correctly. That again makes the test pass. With that test passing I feel I still don't fully understand the issue you're having, and how settings.xml factors in.

I dropped a comment on your change. One of the things that has become clear over the past few days is I don't fully understand rewriteRun, mavenProject, etc enough to use them correctly. In this case, I created this adhoc project, which is supposed to mimic our scenario and the issue I've reported here.

If you use the 'does not work' scenario, mvnDebug, and put a breakpoint ProfileActivation.isActive, you should see the issue. If the behavior of the rewrite code is correct, then I don't think a 'normal' Maven build would even work because it wouldn't be able to resolve the dependencies. However, it does.

I can see the various pom.xml files you have, and the profiles in those, and how you set a different active profile "foobar". I'd expect that to still keep active-profile-1 and active-profile-2 active, but perhaps you have other ideas.

Yes, this is our expectation as well. However, it does not appear to be the case. Might be better to work with the adhoc test I have above.

Any help towards solving a problem is of course appreciated, and perhaps helps explain the issue too, but for now it's a bit unclear to me still.

I may go ahead and work on a fix, but the root of the issue is that a profile that has <activeByDefault>true</activeByDefault> should be considered active even if a set of active profiles is specified from command-line, settings.xml, etc (e.g. user-specified active profiles), unless the particular pom that has activeByDefault profiles contains a profile from the set of user-specified active profiles. Maven doc on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
Status: No status
Development

No branches or pull requests

2 participants