Fix maven wrapper not working in codestarts #12170
Conversation
|
@gsmet this is the issue I am talking about. This is a serious issue as when using |
|
this was undetected because all the tests are running in a quarkus subfolder, and quarkus contains the |
|
The |
| * Default URL to download the maven-wrapper.jar from, if no 'downloadUrl' is provided. | ||
| */ | ||
| private static final String DEFAULT_DOWNLOAD_URL = "https://repo.maven.apache.org/maven2/io/takari/maven-wrapper/" | ||
| + WRAPPER_VERSION + "/maven-wrapper-" + WRAPPER_VERSION + ".jar"; |
There was a problem hiding this comment.
Where is this expression resolved?
There was a problem hiding this comment.
@aloubyansky this is not my code, but copied from the maven wrapper.
There was a problem hiding this comment.
| else | ||
| jarUrl="https://repo.maven.apache.org/maven2/io/takari/maven-wrapper/{maven.version}/maven-wrapper-{maven.version}.jar" | ||
| jarUrl="https://repo.maven.apache.org/maven2/io/takari/maven-wrapper/{maven.wrapper-version}/maven-wrapper-{maven.wrapper-version}.jar" |
There was a problem hiding this comment.
So just to be clear: I think it's a VERY bad idea to have copied the Maven Wrapper in our code. It's going to be a nightmare to maintain.
Not related to this PR per se though.
There was a problem hiding this comment.
I agree with @gsmet here. Ideally the Maven Wrapper plugin should be run to produce these artifacts to avoid people from maintaining this snippet
There was a problem hiding this comment.
@gastaldi @gsmet if you look closer, you will see that we just provides the scripts (not the actual wrapper) which is downloaded automatically when the user first start the command.
The scripts shouldn't change very often (and are not coupled to any version) so it shouldn't be a burden to maintain them.
Regarding the maven version, we could make it dynamic using the maven version used during the build, I didn't have time to do it and didn't know if it wasn't better to have it coded?
There was a problem hiding this comment.
@ia3andy I know, it's the wrapper script I was referring to. Since this is auto-generated from the Tanaki plugin I don't see the point why we need to replicate that behavior.
Just my 2c ;)
There was a problem hiding this comment.
@gastaldi feel free to submit a PR that auto-generate those files in the src/main/resources/codestarts/quarkus/core/tooling/maven-wrapper/base/ directory, that would be cool indeed. Just be careful not to avoid generating the jar, and if you do it, you could also remove the data in the codestart.yml as the version will already be in the generated file.
|
This won't be in 1.8.1, I tried to backport it and it's not a clean backport, there are probably other PRs to backport before this one and it's too late for that. It will have to wait for 1.8.2 if we do an 1.8.2 and you will either have to mark other PRs for backport or write a specific patch. |
|
@gsmet should we merge this PR or rerun CI? |
gastaldi
added
the
triage/waiting-for-ci
maven-wrapper.properties is necessary to automatically download the jar it was working by luck when a parent folder contains a directory
gsmet
force-pushed
the
fix-maven-wrapper-bug-codestarts
branch
from
d6867f6 to
7701784
Compare
|
Got hit by CI master issue, I rebased and force pushed. |
|
JDK 15 Failing because of: merging.. |
maven-wrapper.properties is necessary to automatically download the jar
it was working by luck when a parent folder contains a directory