Incomplete runtime classpath #72
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@cstamas can you rearrange/squash commits and update commit messages to better reflect the change? |
|
Done |
wendigo
approved these changes
Feb 6, 2024
wendigo
reviewed
Feb 6, 2024
| private List<Artifact> resolveRuntimeScopeTransitively() { | ||
| DependencyFilter runtimeFilter = new ScopeDependencyFilter("system", "provided", "test"); | ||
| List<org.eclipse.aether.graph.Dependency> dependencies = project.getDependencies().stream() | ||
| .map(d -> RepositoryUtils.toDependency(d, repositorySystemSession.getArtifactTypeRegistry())) |
wendigo
reviewed
Feb 6, 2024
| DependencyFilter runtimeFilter = new ScopeDependencyFilter("system", "provided", "test"); | ||
| List<org.eclipse.aether.graph.Dependency> dependencies = project.getDependencies().stream() | ||
| .map(d -> RepositoryUtils.toDependency(d, repositorySystemSession.getArtifactTypeRegistry())) | ||
| .filter(d -> runtimeFilter.accept(new DefaultDependencyNode(d), Collections.emptyList())) |
|
I tested this with Trino and now |
|
That sound wrong. @wendigo could you provide a reproducer for that? |
|
Or at least point me at trino module that reproduce this? |
|
I've built locally Trino with the provisio build from this PR. Comparing to the previous build (with the master version): While before this change: Both seems wrong too me but after this change it's even worse :) |
|
Mystery solved: trinodb/trino#20633 |
|
Not quite: why are all trino-spi instanced there? As I debugged locally trino-server build, it even does not have dependencies (so modded code in BaseMojo does not kick in, well yes, but ends up with 0 deps), but MavenProvisioner. |
|
@wendigo please try with this one, after all, there is only one trino-spi needed (blind guess, I have no idea about server, but seems logical that /lib/trino-spi is shared across all plugins) |
|
@cstamas it is, it's added to the plugin classpath by the engine when it creates a plugin. That way plugin can communicate with the engine (they are sharing the same classes). |
|
As when I build locally trino-server now with latest changes in provisio, I end up with only one trino-spi, that is in lib. |
|
I built this PR locally and updated Trino to use Provisio |
|
On a positive note, when I change |
|
@cstamas I have once again tried this PR after I've merged trinodb/trino#20633 and now some plugins contains provide dependencies which shouldn't be the case: Same goes for jol-core: |
|
If I check the dependency:tree for
|
|
Repro: |
|
In the which contains |
|
@wendigo see last commit, contains two things:
Please retry! |
|
Extended the new IT with "case 2" as well (case1=guice+guava story, case2=deps via compile deps should not enter if provided). This is the verbose graph of IT project now: And IT ensure two things:
|
|
With latest provisio changes rebuilt Trino, and in server tar I have these files (occurrence then file name): Unsure for the rest, but now the tarball has one trino-spi which is what is expected, I guess. |
wendigo
approved these changes
Feb 9, 2024
|
I think that we can squash commits 3 and 4 together. I've tested it against Trino master and looks good - for some plugins it's now excluding test dependencies which were incorrectly includes in the zip. |
Similar fix to m-assembly-p: do not rely on Maven Core to provide "runtime" resolution scope as it have issues, see: https://issues.apache.org/jira/browse/MNG-8041 Instead, use Resolver APIs directly to get what is needed. Contains several minor code improvements across Provisio maven-plugin module. Other changes in this PR: * added support for -X to dump proviso graph * marked Mojos as thread safe, removed now unneeded ask for resolution (as they use Resolver APIs) Contains IT from jvanzyl#70 Fixes jvanzyl#71
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Similar fix to m-assembly-p: do not rely on Maven Core to provide
"runtime" resolution scope as it have issues, see:
https://issues.apache.org/jira/browse/MNG-8041
Instead, use Resolver APIs directly to get what is needed.
Contains several minor code improvements across Provisio
maven-plugin module.
Other changes in this PR:
(as they use Resolver APIs)
Contains IT from #70
Fixes #71