Skip to content

Rewrite artifact resolution and validate build plug-ins#46

Merged
slawekjaranowski merged 28 commits into
s4u:masterfrom
cobratbq:rewrite-artifact-resolution
Nov 25, 2019
Merged

Rewrite artifact resolution and validate build plug-ins#46
slawekjaranowski merged 28 commits into
s4u:masterfrom
cobratbq:rewrite-artifact-resolution

Conversation

@cobratbq

@cobratbq cobratbq commented Nov 18, 2019

Copy link
Copy Markdown
Contributor

Redesigned artifact dependency resolution (#45), including build plug-in resolution (#5) and further refactoring.

@slawekjaranowski Could you have a look at the approach I've taken now? I've redesigned the artifact resolution to use non-deprecated API. It would help if you can do an early review as you probably know a lot better what is happening than I do, as I haven't done anything with Maven plug-in development up to now.

Functionality:

  • Dependency version resolution by Maven.
  • Artifacts of type:
    • project dependencies
    • build plug-ins (NOTE: build plug-in dependencies are not yet validated.)
  • Artifacts resolved according to information in MavenProject contents, then updated according to maven's dependency resolution mechanism, for those dependencies that have been resolved.
  • Artifacts processed in determinate order for readability.
  • Version as defined in dependency-management sections should have been preprocessed by Maven, so should transparently be respected.

TODO:

  • Ensure dependency resolution is transitive.
  • Resolve FIXME markers.
  • Careful examination of error handling.
  • Add unit tests.
  • Fix and extend integration tests.
  • Determine whether or not this solves feature request - inherit proxy from Maven #24.
  • Remove final from arguments.
  • Provide configuration parameters for verifying: build plug-ins
  • Update copyright years.
  • Re-check if quiet configuration parameter is respected in all cases.

- Introduced CompositeSkipper to compose any number of SkipFilters with
semantics requiring all SkipFilters to be satisfied. This isolates all
logic for checking filters inside the CompositeSkipper.
- Moved most logic regarding artifact resolution to ArtifactResolver.
- Logic for dependency/plugin signature resolution remains in
PGPVerifyMojo, for now.
- Discovers project dependencies.
- Discovers build plug-ins.
- Discovers dependencies of build plug-ins.
- Filters all dependencies using existing skip filters mechanism.
- Uses resolved versions as determined by Maven dependency resolution process.
- Preserves order of artifact discovering throughout the process for readability.
@cobratbq cobratbq changed the title Rewrite artifact resolution Rewrite artifact resolution and validate build plug-ins and their dependencies Nov 18, 2019
@cobratbq cobratbq force-pushed the rewrite-artifact-resolution branch from dd68795 to 03df59a Compare November 18, 2019 18:28
Comment thread src/main/java/org/simplify4u/plugins/ArtifactResolver.java Outdated
Comment thread src/main/java/org/simplify4u/plugins/ArtifactResolver.java Outdated
Comment thread src/main/java/org/simplify4u/plugins/ArtifactResolver.java Outdated
Comment thread src/main/java/org/simplify4u/plugins/ArtifactResolver.java Outdated
Comment on lines +126 to +132
for (Artifact artifact : artifacts) {
final Artifact ascArtifact = resolveSignature(artifact, requirement);

if (ascArtifact != null || requirement == SignatureRequirement.STRICT) {
artifactToAsc.put(artifact, ascArtifact);
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe java8 strem will be ok

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are there any concerns?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, right! I misunderstood your comment. I'll have a look, see if it makes sense. It should be okay.

Maven does not have any other resource that contains build plug-in
resolution information, hence if we cannot resolve build plug-ins at
first try, there is no retry with different version/specification.
Hence, when failing to resolve build plug-in, immediately fail.
@slawekjaranowski

Copy link
Copy Markdown
Member

Please look at build result ... it look like transitive dependencies for projects are not resolved.

Current version of plugin check all transitive dependencies.

@cobratbq

Copy link
Copy Markdown
Contributor Author

Thank you for the feedback.

Please look at build result ... it look like transitive dependencies for projects are not resolved.

You are correct. I have this fixed, on my local machine. Unfortunately, maven does not help you in the same way to resolve build plug-in dependencies. This is my main focus at the moment. I may split off build dependencies for later time. (With these changes, all IT tests succeed, so no noticable regressions then.)

Current version of plugin check all transitive dependencies.

I am aware. Your IT test caught it, so thank you for that :-). It is tricky as there are multiple ways of resolving plug-ins each with their own disadvantages.

Also, in case you had not noticed: the PR is currently a draft. As long as I have it set as draft, it will not be ready to merge. (Just FYI)

Now using a different mechanism to resolve dependencies and plugins.
This mechanism seems to be the intended way in Maven. It does not,
however, resolve build plug-in dependencies. This is not implemented at
this stage and will need to be added later.

The dependency and plugin artifacts already have resolved the leading
artifact version. We do perform additional filtering according to
configuration.
@cobratbq

Copy link
Copy Markdown
Contributor Author

@slawekjaranowski you might want to double-check your multi-threading fix. I seem to still have the occasional NullPointerException while retrieving PGP keys. I'll copy a stack trace next time I have the issue. (It could be related to my own changes of course, but I don't believe I have touched these regions of code.)

@cobratbq cobratbq force-pushed the rewrite-artifact-resolution branch from 9a69033 to ba23301 Compare November 23, 2019 16:51
@cobratbq cobratbq marked this pull request as ready for review November 23, 2019 21:23
@cobratbq cobratbq changed the title Rewrite artifact resolution and validate build plug-ins and their dependencies Rewrite artifact resolution and validate build plug-ins Nov 23, 2019
Comment thread src/main/java/org/simplify4u/plugins/ArtifactResolver.java Outdated
Instead of sharing filters and filtering over all artifacts, we now
independently discover and filter dependencies. Then, if necessary
according to configuration parameter 'verifyPlugins', discover plugins.
Currently, there are no filters specifically for plug-ins.
This simplifies the existing SkipFilter implementations, as they need
not take into account maven build plug-ins as possible artifacts.
@cobratbq

Copy link
Copy Markdown
Contributor Author

@slawekjaranowski you might need to check Travis CI merge request build, because the results seem to be there but the results aren't picked up by the "Travis CI Pull Request" action above.

AFAICT, this work is ready to be merged.

@slawekjaranowski slawekjaranowski added this to the v1.5.0 milestone Nov 25, 2019
@slawekjaranowski slawekjaranowski added code quality Improvements, refactor or code cleanup. enhancement New feature or request. labels Nov 25, 2019
@slawekjaranowski slawekjaranowski merged commit 40266ac into s4u:master Nov 25, 2019
@cobratbq cobratbq deleted the rewrite-artifact-resolution branch November 25, 2019 22:56
pzygielo pushed a commit to pzygielo/pgpverify-maven-plugin that referenced this pull request May 31, 2024
[MRESOLVER-105] update plexus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality Improvements, refactor or code cleanup. enhancement New feature or request.

Development

Successfully merging this pull request may close these issues.

2 participants