Apply single jar rename in non uberJar case as well #1345

geoand · 2019-03-08T13:32:47Z

cescoffier · 2019-03-08T14:37:56Z

If the runner jar is still the main artifact, don't rename it, that can break other plugins.

geoand · 2019-03-08T14:46:14Z

What gets renamed with this PR is not the -runner jar file, but the original .jar file produced by Maven itself.
Would renaming that be a problem you think? We already do what when uberJar is configured.

vorburger · 2019-03-08T14:46:44Z

Please hold off merging this until I can test if this PR actually fixes #1305 ... will try to do so ASAP. Based on what I meanwhile learnt in #1344 (which I didn't fully understand before), it may actually not. It could still be "nice" to do something like this though - but that would be separate from and not really related to #1344 anymore.

vorburger · 2019-03-08T15:05:33Z

Actually... it's a PITA 😈 to properly test this - because I'd have to get the maven build running inside the S2I builder container to use v999-SNAPSHOT artifacts outside of the container; doable with more effort - but no point: Now that I understand the libs/ business, I don't see how it could work as-is until we do more work in the S2I Builder to also copy libs/ - would you agree @geoand ?

So I think this PR is still a good idea so that we are ready for later, but it (alone) does not fix #1305.

geoand · 2019-03-08T15:17:55Z

@vorburger agreed! it is not going to work for s2i since the assemble script will not copy the lib directory

cescoffier · 2019-03-08T15:19:34Z

Ok, but do we want to suffer from startup penalty because of this?

geoand · 2019-03-08T15:21:46Z

I would say that it would be best to get S2I fixed and not use the uberJar as the primary option but I'm not sure how easily/quickly that could happen

cescoffier · 2019-03-08T15:25:13Z

it seems the @gastaldi has a secret he may wants to share with us...

gastaldi · 2019-03-08T15:30:16Z

I fixed that in the Launcher using the registry.access.redhat.com/redhat-openjdk-18/openjdk18-openshift builder image and:

Adding the following to the pom.xml

    <finalName>${project.artifactId}</finalName>

Adding the following BuildConfig parameters

JAVA_APP_JAR=app-runner.jar
ARTIFACT_COPY_ARGS="-p -r lib/ $JAVA_APP_JAR"

geoand · 2019-03-08T15:32:44Z

Thanks @gastaldi!

So this PR if merged would mean that only ARTIFACT_COPY_ARGS would be required

cescoffier · 2019-03-08T15:41:00Z

@geoand which PR?

geoand · 2019-03-08T15:45:27Z

The current one we are talking in :).
If applied is would change an output like this:

-rw-r--r--. 1 jboss root 34673 Mar  7 13:49 quarkus-quickstart-1.0-SNAPSHOT-runner.jar
-rw-r--r--. 1 jboss root  3353 Mar  7 13:49 quarkus-quickstart-1.0-SNAPSHOT.jar

to one like this:

-rw-r--r--. 1 jboss root 34673 Mar  7 13:49 quarkus-quickstart-1.0-SNAPSHOT-runner.jar
-rw-r--r--. 1 jboss root  3353 Mar  7 13:49 quarkus-quickstart-1.0-SNAPSHOT.jar.original

thus making JAVA_APP_JAR redundant

cescoffier · 2019-03-08T15:46:30Z

@geoand did you see my comment on the main artifacts with some maven plugin?

cescoffier · 2019-03-08T15:46:42Z

(and sorry, I'm bit exhausted)

geoand · 2019-03-08T15:47:10Z

(and sorry, I'm bit exhausted)

No worries, I completely understand :)

geoand · 2019-03-08T15:48:32Z

@geoand did you see my comment on the main artifacts with some maven plugin?

Would you then propose that the -runner suffix be removed in this case?
Making the output something like:

-rw-r--r--. 1 jboss root 34673 Mar  7 13:49 quarkus-quickstart-1.0-SNAPSHOT.jar
-rw-r--r--. 1 jboss root  3353 Mar  7 13:49 quarkus-quickstart-1.0-SNAPSHOT.jar.original

(which is what Spring Boot does BTW)

cescoffier · 2019-03-08T15:50:00Z

I need to check which one is declared as the main artifact. I believe it's the "original" jar.

geoand · 2019-03-08T15:55:49Z

I need to check which one is declared as the main artifact. I believe it's the "original" jar.

Let me know what possible failures/pitfalls you have in mind, because I didn't see any to be honest :)

gsmet · 2019-03-12T10:31:43Z

@geoand could you rebase please?
@cescoffier could you check your theory? I think we're good as, even with a final name, the runner jar was suffixed -runner.

Fixes: quarkusio#1305

geoand · 2019-03-12T10:38:48Z

@gsmet PR rebased onto latest master

cescoffier · 2019-03-12T10:56:15Z

No we are not good. The main artifact currently is the bare one:

[INFO] --- maven-install-plugin:2.4:install (default-install) @ getting-started ---
[INFO] Installing /Users/clement/tmp/foo/target/getting-started-1.0-SNAPSHOT.jar to /Users/clement/.m2/repository/org/acme/getting-started/1.0-SNAPSHOT/getting-started-1.0-SNAPSHOT.jar
[INFO] Installing /Users/clement/tmp/foo/pom.xml to /Users/clement/.m2/repository/org/acme/getting-started/1.0-SNAPSHOT/getting-started-1.0-SNAPSHOT.pom

Actually, the runner is not even attached to the project and thus not installed/deployed.

We cannot use the runner as main artifact as it would break dependencies.

So we must still produce the bare artifact and it should be the main artifact.

gsmet · 2019-03-12T11:04:21Z

@cescoffier I don't understand how your comment is related to this PR? AFAICS, it just renames the main jar to .original when building the runner jar, which we do anyway if the uberJar option is enabled?

Why does it become a problem now?

geoand · 2019-03-12T11:04:24Z

I see the problem the problem that @cescoffier mentions as well.

@gsmet In a generated project if the user does mvn install it will fail with this PR because there is no jar with expected name that the install plugin expects.
However even if the user does mvn install without the PR, the jar that gets installed doesn't make much sense.

cescoffier · 2019-03-12T11:09:42Z

If we change the jar name, we must update the Mojo to declare this new file as the main artifact. And even with this, it can be weird. If the finalName is set, this finalName must be used despite the naming done in this PR.

geoand · 2019-03-12T11:11:02Z

@cescoffier Would it make sense to disable the maven install plugin for the generated projects?
Does doing install even make sense on a Quarkus project?

gsmet · 2019-03-12T11:12:10Z

Hmmm, OK. The problem is not new though as we already do this if the uberJar option is enabled. I could see how it could become a more widespread issue if we do it in all cases.

cescoffier · 2019-03-12T11:13:22Z

"install" still make sense. You can depends on classes and resources from your application.

geoand · 2019-03-12T11:16:00Z

"install" still make sense. You can depends on classes and resources from your application.

Understood, makes absolute sense

geoand · 2019-03-12T11:16:53Z

@gsmet @cescoffier I guess I should close this PR for time being since I don't thing we have a good solution around the install issue at this point.

gsmet · 2019-03-12T11:23:10Z

OK, closing then.

geoand changed the title ~~Apply single jar fix in non uberJar case as well~~ Apply single jar rename in non uberJar case as well Mar 8, 2019

geoand force-pushed the #1305 branch from 2bf0bdb to 4d8d0c5 Compare March 8, 2019 13:36

This was referenced Mar 8, 2019

Support OpenJDK S2I #1305

Closed

quarkus-maven-plugin uberJar configuration knob should be clearly documented #1344

Closed

vorburger mentioned this pull request Mar 8, 2019

[WIP] avoid using <uberJar>true in getting-started-openshift-s2i quarkusio/quarkus-quickstarts#82

Closed

Apply single jar fix in non uberJar case as well

1b297bf

Fixes: quarkusio#1305

geoand force-pushed the #1305 branch from 4d8d0c5 to 1b297bf Compare March 12, 2019 10:38

gsmet closed this Mar 12, 2019

geoand deleted the #1305 branch March 12, 2019 14:28

gsmet added the triage/invalid This doesn't seem right label Mar 13, 2019

Apply single jar rename in non uberJar case as well #1345

Apply single jar rename in non uberJar case as well #1345

Uh oh!

Conversation

geoand commented Mar 8, 2019

Uh oh!

cescoffier commented Mar 8, 2019

Uh oh!

geoand commented Mar 8, 2019

Uh oh!

vorburger commented Mar 8, 2019

Uh oh!

vorburger commented Mar 8, 2019

Uh oh!

geoand commented Mar 8, 2019

Uh oh!

cescoffier commented Mar 8, 2019

Uh oh!

geoand commented Mar 8, 2019

Uh oh!

cescoffier commented Mar 8, 2019

Uh oh!

gastaldi commented Mar 8, 2019

Adding the following to the pom.xml

Adding the following BuildConfig parameters

Uh oh!

geoand commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cescoffier commented Mar 8, 2019

Uh oh!

geoand commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cescoffier commented Mar 8, 2019

Uh oh!

cescoffier commented Mar 8, 2019

Uh oh!

geoand commented Mar 8, 2019

Uh oh!

geoand commented Mar 8, 2019

Uh oh!

cescoffier commented Mar 8, 2019

Uh oh!

geoand commented Mar 8, 2019

Uh oh!

gsmet commented Mar 12, 2019

Uh oh!

geoand commented Mar 12, 2019

Uh oh!

cescoffier commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsmet commented Mar 12, 2019

Uh oh!

geoand commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cescoffier commented Mar 12, 2019

Uh oh!

geoand commented Mar 12, 2019

Uh oh!

gsmet commented Mar 12, 2019

Uh oh!

cescoffier commented Mar 12, 2019

Uh oh!

geoand commented Mar 12, 2019

Uh oh!

geoand commented Mar 12, 2019

Uh oh!

gsmet commented Mar 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

geoand commented Mar 8, 2019 •

edited

Loading

geoand commented Mar 8, 2019 •

edited

Loading

cescoffier commented Mar 12, 2019 •

edited

Loading

geoand commented Mar 12, 2019 •

edited

Loading