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.

Sign up for GitHub

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

Jump to bottom

Update Maven to 3.8.6, maven-wrapper to 3.1.1 and maven-invoker to 3.2.0 #26086

Merged
merged 1 commit into from
Jun 14, 2022
Merged

Update Maven to 3.8.6, maven-wrapper to 3.1.1 and maven-invoker to 3.2.0 #26086

merged 1 commit into from
Jun 14, 2022

Conversation

famod
Copy link
Member

@famod famod commented Jun 13, 2022 •
edited
Loading

https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12316922&version=12351556

[MNG-7432] - [REGRESSION] Resolver session contains non-MavenWorkspaceReader

is the one that caused Quarkus to not update to 3.8.5.

PS: Bumping wrapper and invoker is not mandatory, but I saw those were outdated and just updated them as well.

@famod famod requested a review from aloubyansky June 13, 2022 13:34
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven area/platform Issues related to definition and interaction with Quarkus Platform area/testing labels Jun 13, 2022
famod
famod commented Jun 13, 2022
View reviewed changes
test-framework/maven/src/main/java/io/quarkus/maven/it/verifier/MavenProcessInvoker.java
@@ -48,7 +48,7 @@ public InvocationResult execute(InvocationRequest request) throws MavenInvocatio

File workingDirectory = getWorkingDirectory();
if (workingDirectory != null) {
cliBuilder.setWorkingDirectory(getWorkingDirectory());
cliBuilder.setBaseDirectory(getWorkingDirectory());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See apache/maven-invoker@ca84e5c#diff-7a2ee223cf0ee38d98ed325df11438abe1b5134ce5d7430933461efc0ff5963cL725
I think this change should be ok given the way Quarkus is using this method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking API change though, isn't it? Does this code work with Maven 3.8.6 w/o this change?

Copy link
Member Author

@famod famod Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change in maven-invoker, not in actual Maven, so I don't think Maven < 3.8.6 will have a problem with this.

Should I break this up into three PRs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be OK then, given that it's managed in the quarkus-bom.

@famod
Copy link
Member Author

famod commented Jun 13, 2022

Good candidate for 2.10.0 AFAICS, but too bad it comes too late for CR1.

aloubyansky
aloubyansky approved these changes Jun 13, 2022
View reviewed changes
Copy link
Member

@aloubyansky aloubyansky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @famod

@quarkus-bot

This comment has been minimized.

Sign in to view
@famod
Copy link
Member Author

famod commented Jun 13, 2022

@gsmet any objections?

@famod
Copy link
Member Author

famod commented Jun 14, 2022

I'll go ahead and merge this.

@famod famod merged commit 853d8d7 into quarkusio:main Jun 14, 2022
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jun 14, 2022
@famod famod deleted the mvn-3.8.6 branch June 14, 2022 12:07
@gsmet gsmet modified the milestones: 2.11 - main, 2.10.0.Final Jun 15, 2022
@rsvoboda
Copy link
Member

I think this PR should have noteworthy (or breaking change ) label. And probably migration guide entry.

maven-wagon was updated to 3.5.1 and no longer pulls in jsoup dependency.

We have seen two kinds of troubles

A) failures with consul extension - quarkiverse/quarkus-config-extensions#70

B) usage of jsoup in tests, jsoup was transitive dependency of io.quarkus:quarkus-junit5 and people probably didn't define jsoup explicitly in their projects

@gsmet gsmet modified the milestones: 2.10.0.Final, 2.11 - main Jun 15, 2022
@gsmet
Copy link
Member

gsmet commented Jun 15, 2022

Hmmm, let's not backport it then.

@aloubyansky
Copy link
Member

Thanks for noticing @rsvoboda We'll need to check whether we still want to manage jsoup in the quarkus-bom.

@gsmet
Copy link
Member

gsmet commented Jun 15, 2022

I think we shouldn't. JSoup sometimes have CVE so I would rather avoid it if we don't have a dependency on it.

@famod
Copy link
Member Author

famod commented Jun 15, 2022 •
edited
Loading

So the idea is to:

  • remove jsoup from independent-projects/bootstrap/**
  • add it to build-parent (just managed)
  • revisit and probably remove the exclusion from docs

❓

@gsmet
Copy link
Member

gsmet commented Jun 15, 2022

If we are not depending on it, yes. Now I'm a bit unclear if we are still dragging it in some cases?

@aloubyansky
Copy link
Member

The extension processor uses it. I would say yes to your plan @famod. Were you planning to take care of that?

@rsvoboda
Copy link
Member

I created #26141 for jsoup in the morning, these discussions should be probably moved there

@rsvoboda rsvoboda mentioned this pull request Jun 15, 2022
Closed
@rsvoboda
Copy link
Member

about add it to build-parent (just managed) .. there was #21406 for move from build-parent to bootstrap-bom in November last year ;)

@aloubyansky
Copy link
Member

True, the bootstrap doesn't use it any more though.

@famod famod mentioned this pull request Jul 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloubyansky aloubyansky aloubyansky approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven area/platform Issues related to definition and interaction with Quarkus Platform area/testing
Projects
None yet
Milestone
2.11.0.CR1
Development

Successfully merging this pull request may close these issues.
4 participants
@famod @rsvoboda @gsmet @aloubyansky