Move manifest generation in a specific method #1540
Conversation
core/creator/src/main/java/io/quarkus/creator/phase/runnerjar/RunnerJarPhase.java
Outdated
Show resolved
Hide resolved
core/creator/src/main/java/io/quarkus/creator/phase/runnerjar/RunnerJarPhase.java
Outdated
Show resolved
Hide resolved
| @@ -346,6 +340,54 @@ public void accept(Path path) { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
I am not sure if the below comments are necessary..
There was a problem hiding this comment.
I do prefer to leave that comment since it explain why this method exist
There was a problem hiding this comment.
I do prefer to leave that comment since it explain why this method exist
core/creator/src/main/java/io/quarkus/creator/phase/runnerjar/RunnerJarPhase.java
Outdated
Show resolved
Hide resolved
core/creator/src/main/java/io/quarkus/creator/phase/runnerjar/RunnerJarPhase.java
Outdated
Show resolved
Hide resolved
| * @see https://github.com/quarkusio/quarkus/issues/1443 | ||
| * @param runnerZipFs | ||
| * @param classPath | ||
| * @throws IOException |
There was a problem hiding this comment.
You can remove all the @ entries, they are useless.
If you squash everything and has the issue id in the commit comment (I usually add "Fixes #xxxx" as a second line), it will serve this purpose with the Git history.
There was a problem hiding this comment.
I will try the squash (but am not enough git-ready to make sure I will succeed)
core/creator/src/main/java/io/quarkus/creator/phase/runnerjar/RunnerJarPhase.java
Outdated
Show resolved
Hide resolved
core/creator/src/main/java/io/quarkus/creator/phase/runnerjar/RunnerJarPhase.java
Outdated
Show resolved
Hide resolved
core/creator/src/main/java/io/quarkus/creator/phase/runnerjar/RunnerJarPhase.java
Outdated
Show resolved
Hide resolved
| if (attributes.containsKey(Attributes.Name.MAIN_CLASS)) { | ||
| String existingMainClass = attributes.getValue(Attributes.Name.MAIN_CLASS); | ||
| if (!mainClass.equals(existingMainClass)) { | ||
| log.errorf("Your MANIFEST.MF defines as %s %s, while Quarkus is configured to use %s. " |
There was a problem hiding this comment.
Maybe "Your MANIFEST.MF already defined a MAIN_CLASS entry. Quarkus has overwritten your existing entry." would be better.
You can have the name of the entries as a variable but in this case, please be consistent across the 2 error message.
|
@Riduidel will you have time to address my comments or should I apply them? We are releasing a new release tomorrow and it would be nice to have this one in. Thanks! |
|
I will do the modifications this evening, you can expect them to be
available around 22H (french time)
Le 19/03/2019 à 15:07, Guillaume Smet a écrit :
…
@Riduidel <https://github.com/Riduidel> will you have time to address
my comments or should I apply them?
We are releasing a new release tomorrow and it would be nice to have
this one in. Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1540 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKcCu9iE4-8iIIUz43Z66e4AxEUo_9Cks5vYO8egaJpZM4b4bXM>.
|
|
And indeed, I don't know how to squash those commits that are already pushed to GitHub ... |
…od, invoked after copying all files The goal of this method is to make sure the entries we write in manfiest do not exist already or, if they exist, are compatible with quarkus requirements. This goal is reached for MAIN_CLASS attribute, but not totally for CLASSPATH
gsmet
force-pushed
the
bugfix-1443-overwritten-manifest
branch
from
0ec82e4
to
983d992
Compare
|
@Riduidel I rebased, squashed and pushed to your branch. Let's wait for CI to go green again. As for rebasing, what I usually do is:
|
|
Merged, thanks! |
cescoffier
changed the title
The manifest generation method is to be invoked after file copy.
The goal of this method is to make sure the entries we write in manfiest do not exist already or, if they exist, are compatible with quarkus requirements.
This goal is reached for MAIN_CLASS attribute, but not totally for CLASSPATH