-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update Maven to 3.8.5 and maven-invoker to 3.1.0 #24285
Conversation
@@ -616,7 +617,8 @@ private Model loadCurrentProjectModel() throws BootstrapMavenException { | |||
.addProfileActivator(new PropertyProfileActivator()) | |||
.addProfileActivator(new JdkVersionProfileActivator()) | |||
.addProfileActivator(new OperatingSystemProfileActivator()) | |||
.addProfileActivator(new FileProfileActivator().setPathTranslator(new DefaultPathTranslator())); | |||
.addProfileActivator(new FileProfileActivator().setProfileActivationFilePathInterpolator( | |||
new ProfileActivationFilePathInterpolator().setPathTranslator(new DefaultPathTranslator()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See apache/maven#649
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@famod do you know whether this change is backward compatible? Would this work in Maven 3.6.2. for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aloubyansky very good point! I don't think it is. I'll most likely have to resort to reflection (check the precence of ProfileActivationFilePathInterpolator
).
Edit: Might just work after all. 🤔 Anyway, I'll check...
Interestingly, if I just use Maven 3.8.5 with my Quarkus 2.7.4 based project, the following profile doesn't work anymore: <profile>
<id>image-build</id>
<properties>
<quarkus.container-image.name>register</quarkus.container-image.name>
<quarkus.phase>package</quarkus.phase>
</properties>
<dependencies>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-container-image-jib</artifactId>
</dependency>
</dependencies>
</profile> Meaning: |
Wow! |
If move But my So dependencies in profiles seem broken in Maven 3.8.5. I'll see what the mailing list has to say... |
Nothing I currently aware of from the top of my head. Since I don't know quarkus, best is to bisect. But honestly, I intentionally leave people more than a week to try out a release before it goes GA. Same happened with previous releases 😭 @cstamas you remember we have talked about. I will cut down the voting period down to three days again. |
@michael-o Thanks for your feedback! After some trial and error I had to resort to bisecting and this is the offending commit/change: apache/maven#668 "[3.8.x][MNG-7400] - Allow more WorkspaceReader's to participate" Line 164 in 657ae29
is not getting back the dependency from the profile anymore. At first glance I don't really see what's "wrong" with that change, but it has been a long day and I cannot say I know that code that well. @aloubyansky does that make any sense to you? |
@laeubi can you help? |
I am confused why it actually fails because the default behavior did not change... |
I've just bisected it again, same result. Test project: q_mvn385.zip
but
(and obviously builds the image, as expected) PS: You should have docker running. |
FTR, I built maven like this when bisecting:
and then used the result to build the test project:
|
I'll try to take a look but know nothing about quarkus, I assume that I get errors about missing permissions to access docker is fine an not part of the problem :-) |
@michael-o I think this is good (one week testing period) we do the same for tycho as well. I know that m2e and tycho have done testing of the release so please keep the longer period :-) What might help is, that https://maven.apache.org/download.cgi would not only list current + previous releases but an download for "upcomming" (aka SNAPSHOT) so people are getting more aware of there is something to preview/test even before the the release is started. |
Snapshots aren't guaranteed. I virtually never push them. They maybe master or branches. Best is to build from sources. |
For sure this should be automated, what about Github Action with automatic push to Github packages or something? |
Likely ASF Jenkins does, but I am not sure. @olamy do you know better? |
I debugged this and there is a very slight variation that (actually) should not make a difference but I have not found out why it seems to cause an issue here. 3.8.4https://github.com/apache/maven/blob/9b656c72d54e5bacbed989b64718c159fe39b537/maven-core/src/main/java/org/apache/maven/DefaultMaven.java#L252-L253 3.8.5https://github.com/apache/maven/blob/3599d3414f046de2324203b78ddcf9b5e4388aa0/maven-core/src/main/java/org/apache/maven/DefaultMaven.java#L341-L342 ConsequenceIf code in maven or quarkus using reflection, instanceof or unchecked casts that assume a |
as far as I can see in Jenkinsfile, any build of |
I have searched for reference in maven itself but can't find any reference that directly uses The same hack exits here: and as |
I'm missing the binary distribution here.
I think that would be useful as well. |
ChainedWorkspaceReader is part of maven-resolver ... But maybe maven should simply implement its own MavenChainedWorkspaceReader? |
Absolutely |
Those two hacks you have found are already known as severe issues of over-eagerly caching. People request to remove them. There is at least a JIRA issue for that. |
It's also not obvious for me why that commit breaks it. AFAICS, this issue can be reproduced w/o Quarkus. Here is what I see. |
I can verify (with a local build) that these "hacks" make the difference for quarkus. If I let
Do you think you can provide a Unit-Test or at least an integration test for this? Because even if we fix this now, it will break again if these hacks are removed otherwise... Funny enough this should ale be the case if there was always ever a IDE WSR in place (but probably no one notice it in that context). |
@olamy should I open a PR for a quick-fix? |
yup sure if you have otherwise I will look at it later today. |
and guys you should better talk about maven issues in a Maven place rather than here. |
Right, it was just to get the ball rolling. 🙂
Thanks a lot for your support and for finding the root cause! @aloubyansky so I think we need to skip 3.8.5, agreed? |
Signed-off-by: Christoph Läubrich <christoph@laeubi-soft.de>
Signed-off-by: Christoph Läubrich <christoph@laeubi-soft.de>
Signed-off-by: Christoph Läubrich <christoph@laeubi-soft.de>
Agreed.
@laeubi I am trying to create a more simple reproducer for now that does not involve Quarkus. |
I'll close this for now and will take another look once 3.8.6 is out. Thanks everyone! PS: Feel free to continue discussing here if it doesn't fit elsewhere. |
BTW, @famod looks like there is a workaround for you. If you add activation to your profile, e.g.
and replace |
@laeubi I created a simple reproducer here https://github.com/aloubyansky/playground/tree/maven-3.8.5-profile-activation Hope it'll help. |
@aloubyansky as I don't know much about why/what this does reagrding the hacks @michael-o might can give a hint what the coresponding JIRA issue is so we can link it there? |
@aloubyansky excellent work on that reproducer, very nice rundown of the problem! Do we already have a MNG ticket? If not I can create one later. |
I have created https://issues.apache.org/jira/browse/MNG-7435 now. |
See also:
Btw, "CI Friendly Versions" are now "pluggable": https://issues.apache.org/jira/browse/MNG-7407
We might need to adapt to this in the long run...
In passing, I also updated maven-invoker which hasn't been touched for years.