-
Notifications
You must be signed in to change notification settings - Fork 81
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
fix(SHRINKRES-299): Support "Maven CI Friendly Versions" #142
fix(SHRINKRES-299): Support "Maven CI Friendly Versions" #142
Conversation
@bartoszmajsak I'll try to implement a test for this but as of now there is (apparently) no in-depth test for this class, only a "shallow" one testing dangling directories ( |
ba68c73
to
b67cdef
Compare
Test implemented. Actually I am pretty pleased with the outcome. |
dd4e4dd
to
5e327f1
Compare
This should be it, now. I had to fix a test failure by adding a normally ignored file and some typos. |
5e327f1
to
fb03eb2
Compare
by perferring the output of flatten-maven-plugin over the regular pom.xml which just has a "useless" placeholder like ${revision} as the version.
fb03eb2
to
0c72b50
Compare
@bartoszmajsak Ready when you are! :-) |
On it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for that improvement @famod. Just one thing I wanted to get your input for. See below :)
...src/test/java/org/jboss/shrinkwrap/resolver/impl/maven/aether/ClassPathScanningTestCase.java
Show resolved
Hide resolved
Ah, shoot! That dependency was already available. Will fix that... |
72fc90a
to
f322a43
Compare
Done. I decided to add the dependency to the root |
3764a74
to
3514a72
Compare
Well, that's strange. This second line is not there with I will stop investigating at this point because it's out of scope for this PR. @bartoszmajsak Do you want me to squash anything? If not I'm done (literally). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think we are good at locking it down to 1.17.0
for the time being, but it would be great to chase that up upstream.
@bartoszmajsak Thanks for merging. Are you still planning to cut a relase this year? |
@famod It's been released. I will push to master with the tag as soon as it lands in maven central. Again, many thanks for bringing it back to life :) |
by preferring the output of flatten-maven-plugin over the regular pom.xml
which just has a "useless" placeholder like ${revision} as the version.
Some notes about this approach:
There is already a TODO note in
ClasspathWorkspaceReader.createFoundArtifact(File)
about whether it would be better to load the pom (and its parents) "properly" via Maven.This might have also been a possible solution, but I am not sure whether such a heavy lifting should be done there.
The note regarding "cycle in graph reconstruction" also scared me off a bit, to be honest.
Furthermore, a
revision
property does not need to be defined in the pom at all (AFAIK). It can also be passed in via-Drevision=...
. Parsing the entire Maven model would not help in that case.My first approach was to completely swap the
FileInfo
:but this caused problems in
MavenResolvedArtifactImpl.artifactToFile(Artifact)
as the name of the artifact file would then not bepom.xml
anymore but.flattened-pom.xml
or something else. I would have needed to re-evaluate the new system property there. Or maybeorg.eclipse.aether.artifact.Artifact.getProperty(String, String)
could have been used (ClasspathWorkspaceReader
might be able to set a property that signals "this artifact is a pom!").I dropped that approach because it did not feel right.
And why did I introduce a new system property?
The name and the output directory of the flattened pom are completely configurable, see: https://www.mojohaus.org/flatten-maven-plugin/flatten-mojo.html#flattenedPomFilename
Also there's a ticket about changing the default output folder/name: mojohaus/flatten-maven-plugin#53
So, with the system property people can adapt
shrinkwrap-resolver
to their flatten config.