Skip to content
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

Change the packaging to jar and configure a war profile #9034

Merged
merged 10 commits into from Feb 12, 2019

Conversation

PierreBesson
Copy link
Contributor

@PierreBesson PierreBesson commented Jan 9, 2019

This needs review and a lot of testing to make sure it works for every case.

In order to build the war, you can run:
./mvnw -Pprod,war verify

In my testing, running the produced war file in tomcat and deploying to GAE worked well.

  • Travis tests are green
  • Tests are added where necessary
  • Documentation is added/updated where necessary
  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@PierreBesson
Copy link
Contributor Author

For gradle I have had a look at it but I'm not sure how to do it : @atomfrede do you have an idea ?

@atomfrede
Copy link
Member

@PierreBesson Just to make sure, the title of this PR is wrong, right? You mean Change the packaging to **jar** for maven and configure a war profile?
For gradle would not use a -P switch. Need to check but I think we can just change the default packing to jar. As we still apply the war plugin there is bootWar task available. Everything that needs a war file must just call bootWar instead bootJar or assemble, but will have look to check if thats correct.

@loosebazooka
Copy link

@atomfrede If I recall correctly, using war will explicitly disable the jar task for gradle.

You can still do some switching based on a flag though. Whether that is a system property using -D or a project property using -P, perhaps something like:

build.gradle

// if using -Dpackaging=war
if ("war".equals(System.getProperty("packaging"))) {
  apply plugin 'war'
}

// if using -Ppackaging=war (although I think you might have to check for existence) 
if ("war".equals(project.ext.packaging)) {
  apply plugin 'war'
}

@chanseokoh
Copy link

The title says "Change the packaging to war" and I was confused until I actually see the code. It should say "Change the packaging to jar".

@PierreBesson PierreBesson changed the title Change the packaging to war for maven and configure a war profile Change the packaging to jar for maven and configure a war profile Jan 9, 2019
@PierreBesson
Copy link
Contributor Author

Sorry about the typo. In this case it's really misleading...

@atomfrede
Copy link
Member

@loosebazooka We currently have war and jar packaging available, just a matter of calling bootJar directly. I would prefer not to add more hidden flags as they are not discoverable via gradle tasks, but maybe that needs a vote.

@loosebazooka
Copy link

Aye, I must have misunderstood the problem.

@chanseokoh
Copy link

@atomfrede @PierreBesson is it possible to remove the WAR Plugin for Gradle?

 apply plugin: 'maven'
 apply plugin: 'org.springframework.boot'
-apply plugin: 'war'
 apply plugin: 'propdeps'
 apply plugin: 'com.moowork.node'
 apply plugin: 'io.spring.dependency-management'

@chanseokoh
Copy link

Actually, more places with WAR:

diff --git a/build.gradle b/build.gradle
index 6e4c913..798935d 100644
--- a/build.gradle
+++ b/build.gradle
@@ -34,7 +34,6 @@ assert System.properties['java.specification.version'] == '1.8'
 
 apply plugin: 'maven'
 apply plugin: 'org.springframework.boot'
-apply plugin: 'war'
 apply plugin: 'propdeps'
 apply plugin: 'com.moowork.node'
 apply plugin: 'io.spring.dependency-management'
@@ -54,14 +53,6 @@ version = '0.0.1-SNAPSHOT'
 
 description = ''
 
-bootWar {
-   mainClassName = 'com.example.demoapp.JhipsterApp'
-}
-
-war {
-
-}
-
 springBoot {
     mainClassName = 'com.example.demoapp.JhipsterApp'
     buildInfo()
@@ -255,9 +246,6 @@ wrapper {
     gradleVersion = '4.10.2'
 }
 
-task stage(dependsOn: 'bootWar') {
-}
-
 if (project.hasProperty('nodeInstall')) {
     node {
         version = "${node_version}"
diff --git a/gradle/profile_dev.gradle b/gradle/profile_dev.gradle
index a1b1eb5..22bbce8 100644
--- a/gradle/profile_dev.gradle
+++ b/gradle/profile_dev.gradle
@@ -28,10 +28,6 @@ task webpackBuildDev(type: NpmTask, dependsOn: 'npm_install') {
     args = ["run", "webpack:build"]
 }
 
-war {
-    webAppDirName = 'build/www/'
-}
-
 task copyIntoStatic (type: Copy) {
     from 'build/www/'
     into 'build/resources/main/static'
diff --git a/gradle/profile_prod.gradle b/gradle/profile_prod.gradle
index 26a2978..1eec73c 100644
--- a/gradle/profile_prod.gradle
+++ b/gradle/profile_prod.gradle
@@ -27,10 +27,6 @@ task webpack(type: NpmTask, dependsOn: 'npm_install') {
     args = ["run", "webpack:prod"]
 }
 
-war {
-    webAppDirName = 'build/www/'
-}
-
 task copyIntoStatic (type: Copy) {
     from 'build/www/'
     into 'build/resources/main/static'

@PierreBesson
Copy link
Contributor Author

@chanseokoh will correct me if I'm wrong but I think the problem is as long as there is the war plugin enabled in build.gradle, jib (starting with v0.10) will assume that we want war packaging.

In my tests I have been able to work around that by removing all the war code highlighted above and added this to the build.gradle :

if (project.hasProperty('war')) {
    apply plugin: 'war'
    war {
        enabled = true
    }
}

In this regard it will work similarly to maven. Should I go forward with this change ?

@chanseokoh
Copy link

chanseokoh commented Jan 14, 2019

as long as there is the war plugin enabled in build.gradle, jib (starting with v0.10) will assume that we want war packaging.

That is correct.

Should I go forward with this change ?

From the perspective of a Jib developer, that (not applying the WAR Plugin) is the way we want.

Just from my personal curiosity, is war.enabled = true necessary?

@atomfrede
Copy link
Member

@PierreBesson I would like to remove most -P flags, but in that case it seems we need to have it.

@PierreBesson
Copy link
Contributor Author

@chanseokoh No it is actually not required. I'm not sure why it was there but it could very well be because it was needed for older Spring Boot versions. JHipster actually started with very early versions of Spring Boot.

@atomfrede
Copy link
Member

@chanseokoh No I think I didn't remove it. According to the boot documentation this is supposed to enable war creation in addition to jar creation, but I realized is has no effect (maybe because the war plugin is already applied). If jib can handle executable jars and we do not support container deploymend anymore we can definitl;y remove the war plugin as there are bootJar and bootWar available via the boot plugin.

@PierreBesson
Copy link
Contributor Author

PierreBesson commented Jan 14, 2019

@atomfrede That is not correct, if you don't add the apply plugin: war then the spring boot plugin won't expose a bootWar task so I think the solution using -Pwar is required.

@atomfrede
Copy link
Member

atomfrede commented Jan 14, 2019 via email

@@ -293,7 +302,7 @@ To stop it and remove the container, run:
You can also fully dockerize your application and all the services that it depends on.
To achieve this, first build a docker image of your app by running:

<% if (buildTool === 'maven') { %>./mvnw package -Pprod verify jib:dockerBuild<% } %><% if (buildTool === 'gradle') { %>./gradlew bootWar -Pprod jibDockerBuild<% } %>
<% if (buildTool === 'maven') { %>./mvnw package -Pprod verify jib:dockerBuild<% } %><% if (buildTool === 'gradle') { %>./gradlew bootJar -Pprod jibDockerBuild<% } %>

Choose a reason for hiding this comment

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

FYI: bootJar may not be necessary. Jib doesn't look into a packaged JAR, so normally it's enough to do ./gradlew jibDockerBuild, unless you wired up extra tasks with bootJar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wished our build could be as fast as just compiling the sources but we have a complex builds with resource copying and filtering and in some case even a front-end nodejs build to run. However I wish we could create a special mode to speed up everything and have a fast dev workflow with jib + scaffold. Anyway thanks a lot for your help !

@@ -49,7 +49,7 @@ function checkImages() {
runCommand = './mvnw package -Pprod verify jib:dockerBuild';
} else {
imagePath = this.destinationPath(`${this.directoryPath + appsFolder}/build/jib-cache`);
runCommand = './gradlew bootWar -Pprod jibDockerBuild';
runCommand = './gradlew bootJar -Pprod jibDockerBuild';

Choose a reason for hiding this comment

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

ditto

@@ -49,7 +49,7 @@ function checkImages() {
runCommand = './mvnw package -Pprod verify jib:dockerBuild';

Choose a reason for hiding this comment

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

Related, package is no longer required for the Jib standard (non-WAR) containerization. Normally, ./mvnw compile jib:dockerBuild should work (although if we were to do the WAR containerization, package is required.)

@PierreBesson PierreBesson force-pushed the make-jar-not-war branch 5 times, most recently from f3ecd6a to 2d31df8 Compare January 16, 2019 13:14
@PierreBesson PierreBesson changed the title Change the packaging to jar for maven and configure a war profile Change the packaging to jar and configure a war profile Jan 16, 2019
@jdubois jdubois removed the v6 label Jan 29, 2019
@deepu105 deepu105 added the v6 label Feb 9, 2019
@pascalgrimaud
Copy link
Member

I'll start to review this, but as there are some conflicts, can you rebase your branch plz @PierreBesson ?
Tell me when it's done

@PierreBesson
Copy link
Contributor Author

The rebase is done.

@pascalgrimaud
Copy link
Member

Tested and it works well.
I'm going to merge this, waiting the CI to finish

@pascalgrimaud
Copy link
Member

good job @PierreBesson !

@pascalgrimaud pascalgrimaud merged commit cf33f1d into jhipster:master Feb 12, 2019
@DanielFran
Copy link
Member

@PierreBesson @pascalgrimaud @jdubois Did not found any update in documentation about those changes...

@pascalgrimaud
Copy link
Member

@DanielFran : maybe we forgot to do it... Can you open a new ticket plz ? So we won't forget to do it

@DanielFran
Copy link
Member

@pascalgrimaud Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants