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

Resolve issues with using as a pom extension #896

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

rnc
Copy link
Contributor

@rnc rnc commented Nov 24, 2021

@rnc rnc requested a review from dwalluck November 24, 2021 16:15
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #896 (bb2f169) into main (adc8ec2) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #896      +/-   ##
============================================
- Coverage     83.57%   83.56%   -0.02%     
+ Complexity     1882     1881       -1     
============================================
  Files           116      116              
  Lines          7014     7015       +1     
  Branches       1210     1210              
============================================
  Hits           5862     5862              
  Misses          672      672              
- Partials        480      481       +1     
Impacted Files Coverage Δ
...mmonjava/maven/ext/integrationtest/ITestUtils.java 81.88% <100.00%> (+0.13%) ⬆️
...a/maven/ext/io/resolver/MavenLocationExpander.java 75.45% <0.00%> (-0.91%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adc8ec2...bb2f169. Read the comment docs.

@dwalluck
Copy link
Member

It looks good to me. It looks like we "just" had to exclude all instances of plexus-utils from our classpath.

I just wanted to be clear on two things said on https://issues.apache.org/jira/browse/MNG-7160:

  1. That "this will only work when the extension does not actually need plexus-utils classes other than the XPP3 ones exported from the maven api realm". I guess we just get lucky there, otherwise it would not be possible?
  2. That "this state means that it's always possible to use the same extension as a core extension or as a build extension, which I don't think is a good state". So is this a bug or a feature? In other words, even though we have a workaround, I suppose we could have left it as only "officially" supported as a build extension.

@gnodet
Copy link

gnodet commented Nov 26, 2021

It looks good to me. It looks like we "just" had to exclude all instances of plexus-utils from our classpath.

I just wanted to be clear on two things said on https://issues.apache.org/jira/browse/MNG-7160:

  1. That "this will only work when the extension does not actually need plexus-utils classes other than the XPP3 ones exported from the maven api realm". I guess we just get lucky there, otherwise it would not be possible?

Yes, that's right. This is fixed by the PR apache/maven#616.

  1. That "this state means that it's always possible to use the same extension as a core extension or as a build extension, which I don't think is a good state". So is this a bug or a feature? In other words, even though we have a workaround, I suppose we could have left it as only "officially" supported as a build extension.

Sorry, I meant "it's not always possible", I fixed the comment... The goal is to reconcile core and build extensions when possible. The apache/maven#616 PR offers some ways to configure the classloading strategy for core extensions in way which is similar to build extensions, so this bridges the classloading gap.

@dwalluck
Copy link
Member

Sorry, I meant "it's not always possible", I fixed the comment... The goal is to reconcile core and build extensions when possible. The apache/maven#616 PR offers some ways to configure the classloading strategy for core extensions in way which is similar to build extensions, so this bridges the classloading gap.

OK, thanks. We had only been using the extension via the POM method before this PR, so I just wanted to make sure we weren't doing anything out of the ordinary.

@rnc rnc merged commit 0e9b2ee into release-engineering:main Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project version manipulation not working (?!?)
3 participants