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

Packaging check in maven jetty plugin breaking execution #2372

Closed
PhantomYdn opened this issue Mar 23, 2018 · 15 comments

Comments

@PhantomYdn
Copy link

commented Mar 23, 2018

Hello,

We had a very nice feature in our build system: any jar module can be run on a web server just by running mvn jetty:run. Example how that was configured: https://github.com/OrienteerBAP/Orienteer/blob/master/orienteer-core/pom.xml#L274-L306

But with jetty update that become impossible because of the following check:
4b513c6#diff-28ee2c02100a8cae9e1d1754ea32af10R64

Please clarify why this check was introduced? And can you please consider removing this check or add some workaround for it? Can't understand what particular value this check is bringing. Per your issues history there are others who has problem exactly because of these check.

@olamy olamy self-assigned this Mar 23, 2018

@olamy

This comment has been minimized.

Copy link
Collaborator

commented Mar 23, 2018

The idea is to avoid installing jars for multi modules projects:

pom.xml
              module-a (jar)
              module-b (jar)
              module-c (war) (using module-a and module-b)

So previously if you wanted to run module-c with jetty:run you had to first install all the modules then run jetty:run only on module-c. A bit time consuming...
Now you simply run jetty:run from the top and everything is compiled etc.. and included in your webapp classloader (including module-a/b target/classes). So really time saving. Well for this use jetty:run need to avoid start launching webapp in module-a/b so the packaging check.
I hope this make sense. What I can do is to add a boolean skipPackaging (false per default).

@olamy olamy added this to the 9.4.x milestone Mar 23, 2018

@olamy olamy added the Enhancement label Mar 23, 2018

@olamy

This comment has been minimized.

Copy link
Collaborator

commented Mar 23, 2018

Well reading your pom. I think there is something chicken and egg: core depends on war module but war module depends on core as well..
IMHO you should use jetty:run from the war module and not the core.

@PhantomYdn

This comment has been minimized.

Copy link
Author

commented Mar 23, 2018

There is only war file being used from 'orienteer-war' module. And actually all modules which are packed as jar allowing to check just them by running 'mvn jetty:run' right now. For example:
https://github.com/OrienteerBAP/Orienteer/blob/master/orienteer-bpm/pom.xml#L185-L215
https://github.com/OrienteerBAP/Orienteer/blob/master/orienteer-pages/pom.xml#L95-L126
All modules available from this link: https://github.com/OrienteerBAP/Orienteer
So don't see any chicken and eggs problems here: it's just one file placed in one place and being reused from different modules instead of copying and pasting it everywhere.
One more point: our war does not include all modules. Those modules can be statically included into
child projects or dynamically through runtime loading which we implemented.

Yes - if you can help a way to work-around this check - as you propose with 'skipPackaging' - it will be great! Thanks!

olamy added a commit to olamy/jetty.project that referenced this issue Mar 23, 2018

add flag skipPackagingTest eclipse#2372
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@olamy

This comment has been minimized.

Copy link
Collaborator

commented Mar 23, 2018

look #2375 can you please test with your project if that works?

@joakime

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

No, don't do skipPackingTest logic.

If we are going to do any skip logic it will be to skip the entire plugin execution, not just a specific step in it.

There's 1 problem and 1 new feature we should consider instead for this issue.

First, we shouldn't be throwing a MojoExecutionException if the packaging isn't war.

if ( !"war".equals( project.getPackaging() ))
throw new MojoExecutionException("Not war packaging");

This should be a simple logged warning and skipped mojo execution.

Something like ...

            if ( !"war".equals( project.getPackaging() )
            {
                getLog().info( "Skipping non-war project " + project);
                return; // don't run this mojo
            }

Then we should create a new configurable which allows the users of the plugin to define what set of packaging types are allowed in their project. A list/collection/array of packaging type strings.
This would even allow for those users that use complex plugin setups that extend from the war packaging type to function.
This list/colllection/array would be defaulted to "war" (obviously).

The above check would then check the list/collection/array of allowed packaging types instead of a hardcoded one.

@olamy

This comment has been minimized.

Copy link
Collaborator

commented Mar 23, 2018

@joakime good catch LGTM I will change that

olamy added a commit to olamy/jetty.project that referenced this issue Mar 24, 2018

add list of supported packaging for run mojos eclipse#2372
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@olamy

This comment has been minimized.

Copy link
Collaborator

commented Mar 24, 2018

packaging list support added
@PhantomYdn
simply use:

            <configuration>
              <supportedPackagings>
                <supportedPackaging>jar</supportedPackaging>
              </supportedPackagings>
            </configuration>
@janbartel

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2018

I don't exactly see why we need the test of the packaging of the project at all? Presumably if a user has defined the jetty plugin in the pom, then they want it executed (unless they've explicitly configured the "skip" option)?

@olamy

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2018

janbartel added a commit that referenced this issue Mar 27, 2018

add list of supported packaging for run mojos #2372 (#2375)
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@janbartel

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

Fix checked in via pr #2375

@janbartel janbartel closed this Mar 27, 2018

alee added a commit to virtualcommons/foraging that referenced this issue May 11, 2018

eppleton added a commit to eppleton/dolphin-platform-examples that referenced this issue Jul 23, 2018

project does not build due to pom errors. this commit fixes the probl…
…ems:

wrong packaging (war)

simple-security-client-javascript aus: [ERROR] Failed to execute goal org.apache.maven.plugins:maven-war-plugin:2.2:war (default-war) on project simple-security-client-javascript: Error assembling WAR: webxml attribute is required (or pre-existing WEB-INF/web.xml if executing in update mode) -> [Help 1] 

can be solved by changing packing (remove war):

Also todo-list fails due to jetty mvn plugin updates (eclipse/jetty.project#2372), can be fixed by:

           <configuration>
              <supportedPackagings>
                <supportedPackaging>jar</supportedPackaging>
              </supportedPackagings>
            </configuration>
@gracekarina

This comment has been minimized.

Copy link

commented Aug 11, 2018

Hi, this is still happening to me, is there any special configuration I should add to my pom? Thanks in advance.
My jetty version is <jetty-version>9.4.9.v20180320</jetty-version>

                <groupId>org.eclipse.jetty</groupId>
                <artifactId>jetty-maven-plugin</artifactId>
                <version>${jetty-version}</version>
                <configuration>
                    <useTestScope>true</useTestScope>
                    <systemProperties>
                        <systemProperty>
                            <name>config</name>
                            <value>src/test/config/config1.yaml</value>
                        </systemProperty>
                    </systemProperties>
                    <webApp>
                        <contextPath>/</contextPath>
                    </webApp>
                    <stopPort>8079</stopPort>
                    <stopKey>stopit</stopKey>
                    <httpConnector>
                        <port>8080</port>
                        <idleTimeout>60000</idleTimeout>
                    </httpConnector>
                </configuration>
                <executions>
                    <execution>
                        <id>start-jetty</id>
                        <phase>pre-integration-test</phase>
                        <goals>
                            <goal>start</goal>
                        </goals>
                        <configuration>
                            <supportedPackagings>
                                <supportedPackaging>jar</supportedPackaging>
                            </supportedPackagings>
                            <scanIntervalSeconds>0</scanIntervalSeconds>
                            <daemon>true</daemon>
                        </configuration>
                    </execution>
                    <execution>
                        <id>stop-jetty</id>
                        <phase>post-integration-test</phase>
                        <goals>
                            <goal>stop</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>
@olamy

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2018

@gracekarina should not. Can you try with version 9.4.11.v20180605?
Do you have a sample project to reproduce?

@gracekarina

This comment has been minimized.

Copy link

commented Aug 14, 2018

Hi @olamy, sorry for the delay, this is the project https://github.com/swagger-api/swagger-inflector/blob/update-jetty-version/pom.xml Thanks in advance!

@olamy

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2018

@gracekarina I'm not clear what your problem is? is it using mvn jetty:run
anyway please look at this pr swagger-api/swagger-inflector#269 this will fix your problem

@gracekarina

This comment has been minimized.

Copy link

commented Aug 15, 2018

Thanks @olamy that was the issue, and now it's fix with the PR above. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.