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

[SHRINKRES-351] enh: Update components resulted from project-local resolution #340

Merged
merged 2 commits into from
Oct 15, 2024
Merged

[SHRINKRES-351] enh: Update components resulted from project-local resolution #340

merged 2 commits into from
Oct 15, 2024
+45 −112

Conversation

lprimak
Copy link
Contributor

@lprimak lprimak commented Sep 10, 2024 •
edited
Loading

Updates components (sisu, maven) where necessary
Adds Java 17 builds to CI matrix
Based, and will be rebased on #341 when it gets merged

closes #214

@lprimak lprimak force-pushed the local-resolver branch 2 times, most recently from abd7576 to e7cac17 Compare September 12, 2024 05:39
@lprimak
Copy link
Contributor Author

lprimak commented Sep 12, 2024 •
edited
Loading

TODO: successfully upgrade maven invoker component (not plugin)

@lprimak lprimak force-pushed the local-resolver branch 2 times, most recently from 9b14ce2 to 22fa996 Compare September 12, 2024 05:56
@petrberan
Copy link
Member

Thank you for looking into this @lprimak . I'll duplicate the main branch as new support and probably release Resolvers in version 4.x with JDK 17 support and leave JDK 8 for the 3.x branch

We should also update our test matrix for the JDK 17

@lprimak lprimak force-pushed the local-resolver branch 2 times, most recently from c6c51a4 to 4898da0 Compare September 13, 2024 03:14
@lprimak lprimak changed the title enh: use maven 4 local project resolver enh: Update components resulted from project-local repository Sep 13, 2024
@lprimak lprimak changed the title enh: Update components resulted from project-local repository enh: Update components resulted from project-local resolution Sep 13, 2024
@lprimak
Copy link
Contributor Author

lprimak commented Sep 13, 2024

Figured out how to keep JDK 8 compatibility. Only checkstyle needed to be downgraded, which is no big deal.
Now compiles on 8,11 and 17 with the new features and component upgrades

@lprimak lprimak mentioned this pull request Sep 13, 2024
Closed
@lprimak lprimak changed the title enh: Update components resulted from project-local resolution [SHRINKRES-351] enh: Update components resulted from project-local resolution Sep 21, 2024
@petrberan petrberan mentioned this pull request Sep 27, 2024
Merged
@lprimak lprimak marked this pull request as ready for review October 2, 2024 16:12
@lprimak
Copy link
Contributor Author

lprimak commented Oct 2, 2024

@petrberan This one is ready to go as well now.
Thank you!

petrberan
petrberan reviewed Oct 3, 2024
View reviewed changes
...n-embedded/src/main/java/org/jboss/shrinkwrap/resolver/impl/maven/embedded/BuildTrigger.java
@@ -39,10 +39,10 @@ private void setOutputHandlers(String expectedRegex, CountDownLatch countDownLat
new ResolverErrorOutputHandler(logBuffer, expectedRegex, countDownLatch);
ResolverOutputHandler outputHandler = new ResolverOutputHandler(logBuffer, expectedRegex, countDownLatch);

invoker.setOutputHandler(outputHandler);
invocationRequest.setOutputHandler(outputHandler);
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be duplicated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... I don't see where / how

Copy link
Member

Choose a reason for hiding this comment

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

L42 and L43 are the same - invocationRequest.setOutputHandler(outputHandler);
L45 and L46 are the same - invocationRequest.setErrorHandler(errorOutputHandler);

Or is it necessary to set the handlers 2 times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see it now. Thanks for pointing this out.
Fixed!

@petrberan
Copy link
Member

Shame we can't keep the Guice on 7.0.0 though. Besides just some minor details this looks solid @lprimak , can you have a look at the comments above?

@lprimak
Copy link
Contributor Author

lprimak commented Oct 3, 2024 •
edited
Loading

Yup, Plexus / Sisu is not compatible with Guice 7.
I will separate the CI Java 17 into separate commit.
I don't think the other comments are workable though.

Thanks for your help!

@lprimak
Copy link
Contributor Author

lprimak commented Oct 3, 2024

Done!

petrberan
petrberan approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@petrberan petrberan left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @lprimak , I'll merge this and release new Resolvers later this week

@petrberan petrberan merged commit 953b648 into shrinkwrap:master Oct 15, 2024
3 checks passed
@lprimak lprimak deleted the local-resolver branch October 15, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrberan petrberan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lprimak @petrberan