#3725 - jetty-maven-plugin - WAR overlay dependencies not resolved from maven reactor #3858

mironbalcerzak · 2019-07-08T21:38:12Z

Signed-off-by: Miron Balcerzak [email protected]

Howdy!
How are you? Hope you are doing all fine.

As the things are at the moment, this PR is not ready for merge just yet. It's more for code-review and comments on what else needs to be done. So....

Base project structure
https://github.com/mironbalcerzak/gist-jetty-maven-plugin-w-overlay

What's done?

Jetty-maven-plugin loads classes/resources right from the "target/classes"-dir or "src/main/webapp"-dir in multi-module maven environment if any of the modules is present in maven reactor.
Scanners are set to look at those directories too, so any change (and recompile), triggers redeploy and "hot-swap"
overlaying concept works fine (resources defined first overlay resources defined later)

What's not done? (for now)

maven-war-plugin's targetPath attribute is not handled at all in "new" functionality
maven-war-plugin's exclude/include is not handled at all in "new" functionality
generated-sources - no check done if those are present

More research needs to be done on my side, whether it's possible to create a org.eclipse.jetty.util.resource.Resource that will handle targetPath, exclusion, inclusion "on-the-fly", instead of extracting to jetty_overlays.

Nice to have / Proposals

in jetty_overlays scenario, extract WEB-INF/lib from each WAR to single directory - that way, we will avoid same class loading from multiple location
extracted WEB-INF/lib JARs, match with artifacts present in maven reactor using any checksum algo (is it even worth thinking of?)

How to test?

git clone https://github.com/mironbalcerzak/gist-jetty-maven-plugin-w-overlay ${PROJECT}
location of the loaded classes can be seen on localhost:8080

Test Case No.1
Executing jetty-maven-plugin on base dir of a multi-module project to create overlaid war of few WARs

cd ${PROJECT}
mvn deploy -Poverlay

JDK / maven repo files
[class] j.lang.Exception	->	file:/${JAVA_HOME}/jre/lib/rt.jar!/java/lang/Exception.class
[class] j.s.http.HttpServlet	->	file:/${M2_REPO}/repository/javax/servlet/javax.servlet-api/3.1.0/javax.servlet-api-3.1.0.jar!/javax/servlet/http/HttpServlet.class
Overlay module
[class] o.m.j.overlay.Overlay_servlet	->	(not available)
[class] o.m.j.overlay.OverlayNested_servlet	->	${PROJECT}/overlay2/overlay-nested/target/classes/org/mca/jetty/overlay/OverlayNested_servlet.class
Common
[class] o.m.j.common.Common	->	${PROJECT}/common/target/classes/org/mca/jetty/common/Common.class
Module A
[class] o.m.j.moduleA.ModuleA_api	->	${PROJECT}/module-a/module-a-api/target/classes/org/mca/jetty/moduleA/ModuleA_api.class
[class] o.m.j.moduleA.ModuleA_impl	->	${PROJECT}/module-a/module-a-impl/target/classes/org/mca/jetty/moduleA/ModuleA_impl.class
[class] o.m.j.moduleA.ModuleA_servlet	->	${PROJECT}/module-a/module-a-war/target/classes/org/mca/jetty/moduleA/ModuleA_servlet.class
[resource] ${webRoot}\module-a-api.txt	->	${PROJECT}/module-a/module-a-api/target/classes/module-a-api.txt
[resource] ${webRoot}\module-a-impl.txt	->	${PROJECT}/module-a/module-a-impl/target/classes/module-a-impl.txt
[resource] ${webRoot}\module-a-war.txt	->	${PROJECT}/module-a/module-a-war/target/classes/module-a-war.txt
[resource] ${webRoot}\module-a-root.txt	->	(not available)
Module B
[class] o.m.j.moduleB.ModuleB_api	->	${PROJECT}/module-b/module-b-api/target/classes/org/mca/jetty/moduleB/ModuleB_api.class
[class] o.m.j.moduleB.ModuleB_impl	->	${PROJECT}/module-b/module-b-impl/target/classes/org/mca/jetty/moduleB/ModuleB_impl.class
[class] o.m.j.moduleB.ModuleB_servlet	->	${PROJECT}/module-b/module-b-war/target/classes/org/mca/jetty/moduleB/ModuleB_servlet.class
[resource] ${webRoot}\module-b-api.txt	->	${PROJECT}/module-b/module-b-api/target/classes/module-b-api.txt
[resource] ${webRoot}\module-b-impl.txt	->	${PROJECT}/module-b/module-b-impl/target/classes/module-b-impl.txt
[resource] ${webRoot}\module-b-war.txt	->	${PROJECT}/module-b/module-b-war/target/classes/module-b-war.txt
[resource] ${webRoot}\module-b-root.txt	->	(not available)

Test Case No.2
Executing jetty-maven-plugin in one of the nested directories, to trigger "partial" handling of modules.

cd ${PROJECT}/overlay2
mvn deploy -PoverlayNested

Overlay Nested: Hello World
JDK / maven repo files
[class] j.lang.Exception	->	file:/${JAVA_HOME}/jre/lib/rt.jar!/java/lang/Exception.class
[class] j.s.http.HttpServlet	->	file:/${M2_REPO}/repository/javax/servlet/javax.servlet-api/3.1.0/javax.servlet-api-3.1.0.jar!/javax/servlet/http/HttpServlet.class
Overlay module
[class] o.m.j.overlay.Overlay_servlet	->	(not available)
[class] o.m.j.overlay.OverlayNested_servlet	->	${PROJECT}/overlay2/overlay-nested/target/classes/org/mca/jetty/overlay/OverlayNested_servlet.class
Common
[class] o.m.j.common.Common	->	file:/${M2_REPO}/repository/org/mca/common/1.0-SNAPSHOT/common-1.0-SNAPSHOT.jar!/org/mca/jetty/common/Common.class
Module A
[class] o.m.j.moduleA.ModuleA_api	->	file:/${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-a-war-1_0-SNAPSHOT_war/WEB-INF/lib/module-a-api-1.0-SNAPSHOT.jar!/org/mca/jetty/moduleA/ModuleA_api.class
[class] o.m.j.moduleA.ModuleA_impl	->	file:/${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-a-war-1_0-SNAPSHOT_war/WEB-INF/lib/module-a-impl-1.0-SNAPSHOT.jar!/org/mca/jetty/moduleA/ModuleA_impl.class
[class] o.m.j.moduleA.ModuleA_servlet	->	${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-a-war-1_0-SNAPSHOT_war/WEB-INF/classes/org/mca/jetty/moduleA/ModuleA_servlet.class
[resource] ${webRoot}\module-a-api.txt	->	file:/${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-a-war-1_0-SNAPSHOT_war/WEB-INF/lib/module-a-api-1.0-SNAPSHOT.jar!/module-a-api.txt
[resource] ${webRoot}\module-a-impl.txt	->	file:/${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-a-war-1_0-SNAPSHOT_war/WEB-INF/lib/module-a-impl-1.0-SNAPSHOT.jar!/module-a-impl.txt
[resource] ${webRoot}\module-a-war.txt	->	${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-a-war-1_0-SNAPSHOT_war/WEB-INF/classes/module-a-war.txt
[resource] ${webRoot}\module-a-root.txt	->	(not available)
Module B
[class] o.m.j.moduleB.ModuleB_api	->	file:/${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-b-war-1_0-SNAPSHOT_war/WEB-INF/lib/module-b-api-1.0-SNAPSHOT.jar!/org/mca/jetty/moduleB/ModuleB_api.class
[class] o.m.j.moduleB.ModuleB_impl	->	file:/${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-b-war-1_0-SNAPSHOT_war/WEB-INF/lib/module-b-impl-1.0-SNAPSHOT.jar!/org/mca/jetty/moduleB/ModuleB_impl.class
[class] o.m.j.moduleB.ModuleB_servlet	->	${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-b-war-1_0-SNAPSHOT_war/WEB-INF/classes/org/mca/jetty/moduleB/ModuleB_servlet.class
[resource] ${webRoot}\module-b-api.txt	->	file:/${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-b-war-1_0-SNAPSHOT_war/WEB-INF/lib/module-b-api-1.0-SNAPSHOT.jar!/module-b-api.txt
[resource] ${webRoot}\module-b-impl.txt	->	file:/${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-b-war-1_0-SNAPSHOT_war/WEB-INF/lib/module-b-impl-1.0-SNAPSHOT.jar!/module-b-impl.txt
[resource] ${webRoot}\module-b-war.txt	->	${PROJECT}/overlay2/overlay-nested/target/jetty_overlays/module-b-war-1_0-SNAPSHOT_war/WEB-INF/classes/module-b-war.txt
[resource] ${webRoot}\module-b-root.txt	->	(not available)
Module C
[class] o.m.j.moduleC.ModuleC_api	->	${PROJECT}/overlay2/module-c/module-c-api/target/classes/org/mca/jetty/moduleC/ModuleC_api.class
[class] o.m.j.moduleC.ModuleC_impl	->	${PROJECT}/overlay2/module-c/module-c-impl/target/classes/org/mca/jetty/moduleC/ModuleC_impl.class
[class] o.m.j.moduleC.ModuleC_servlet	->	${PROJECT}/overlay2/module-c/module-c-war/target/classes/org/mca/jetty/moduleC/ModuleC_servlet.class
[resource] ${webRoot}\module-c-api.txt	->	${PROJECT}/overlay2/module-c/module-c-api/target/classes/module-c-api.txt
[resource] ${webRoot}\module-c-impl.txt	->	${PROJECT}/overlay2/module-c/module-c-impl/target/classes/module-c-impl.txt
[resource] ${webRoot}\module-c-war.txt	->	${PROJECT}/overlay2/module-c/module-c-war/target/classes/module-c-war.txt
[resource] ${webRoot}\module-c-root.txt	->	(not available)

Test Case No.3
In scenario when there are no modules available, all war files are extracted to "jetty_overlays" as it is done now.

Zzzzzzzzzzzzzz......

What are your thoughts? Is there anything that I should consider in addition to what I've already mentioned in "what's not done" section?

Thanks for your time and have a great day! :)
-- mb

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyRunMojo.java

janbartel · 2019-07-11T09:54:17Z

Hi @mironbalcerzak,
thanks for looking into this issue, it's not an easy one!

I've left a couple of initial comments in the code, but we will discuss more of course.

A couple of comments also here:

maven-war-plugin's exclude/include is not handled at all in "new" functionality

I think we're going to need to honour the include/excludes for projects that are in the reactor. You will probably need a class similar to the SelectiveJarResource that handles the inclusions/exclusions. This is going to get very complicated if the the dependency war has itself overlays.

in jetty_overlays scenario, extract WEB-INF/lib from each WAR to single directory - that way, we will avoid same class loading from multiple location

We shouldn't need to do that, maven should ensure that everything in the reactor has all the transitive dependencies correctly resolved, shouldn't it?

mironbalcerzak · 2019-07-11T10:08:41Z

Hi @janbartel

thanks for looking into this issue, it's not an easy one!

No problem at all! Thanks for letting me work on that - it's nice (and fresh?) change from everyday work :)

A couple of comments also here:

maven-war-plugin's exclude/include is not handled at all in "new" functionality

I think we're going to need to honour the include/excludes for projects that are in the reactor. You will probably need a class similar to the SelectiveJarResource that handles the inclusions/exclusions. This is going to get very complicated if the the dependency war has itself overlays.

I will handle it naturally - this PR is more of a proof-of-concept for the core issue - and I agree, I was already playing around with SelectiveJarResource, it should be possible to achieve what we need.

in jetty_overlays scenario, extract WEB-INF/lib from each WAR to single directory - that way, we will avoid same class loading from multiple location

We shouldn't need to do that, maven should ensure that everything in the reactor has all the transitive dependencies correctly resolved, shouldn't it?

It totally should, and it does - apologies, I phrased it not clear enough.
In case of "jetty_overlays" scenario, dependencies are being extracted to WEB-INF/lib for each overlay, and one of the steps of jetty-maven-plugin adds them to webApp, If 2 (or more) overlays have a common JAR, we end up adding it few times - and AnnotationProcessor complains about it (which makes perfect sense). That way we would ensure that all libs are add only once (as we would be able to keep a track of what we add in a simple manner - and more efficient too as we would just be able to skip extraction of those that are already present)

have a great day!

-- mb

janbartel · 2019-07-11T11:47:14Z

In case of "jetty_overlays" scenario, dependencies are being extracted to WEB-INF/lib for each overlay, and one of the steps of jetty-maven-plugin adds them to webApp, If 2 (or more) overlays have a common JAR, we end up adding it few times - and AnnotationProcessor complains about it (which makes perfect sense).

Hhhmmm, shouldn't the include/exclude statements ensure that only 1 of those webapps is including the common jar? Otherwise having the common jar more than once shows up an error in the project configuration, no?

mironbalcerzak · 2019-07-11T12:13:33Z

Hhhmmm, shouldn't the include/exclude statements ensure that only 1 of those webapps is including the common jar? Otherwise having the common jar more than once shows up an error in the project configuration, no?

Isn't it overcomplicating? As far as I understand what you suggested, we would need to firstly extract names of JARs from each overlay (artifact would be present in ${m2_repo} - so we wouldn't have access to dependency graph through maven, then configure exclude/include of SelectiveJarResource in a "smart way". The solution with single WEB-INF/lib for all overlays, we would not bother with that at all. JARs would just overwrite one another, or maybe silently skip (I need to check what exactly SelectiveJarResource.copyTo() does)

...or do you mean, while configuring of "overlays" at the plugin level? in pom.xml? It certainly would make things easy, but does anyone ever do that? From personal experience I can tell that people are just adding things in to pom.xml and it "somehow" works - unless it does not, then time and effort is spent to make it work... 🤐
And configuration like that would need to be maintained each time deps are being added, updated, changed in a pattern manner - so I doubt it's really happening...

olamy · 2019-07-16T09:47:58Z

my main concern is a lot of changes and the lack of test. If there is a test project you can probably transform it to an it test?

mironbalcerzak · 2019-07-16T11:25:25Z

@olamy, I'm testing manually. I tried to do tests in #3349, but there was an issue with maven-invoker-plugin as far as I remember. Can give it another try if anything changed over there?

olamy · 2019-07-17T06:21:20Z

yes definitely. If any issue with maven-invoker-plugin please let me know I can have a look at fixing it.

mironbalcerzak · 2019-07-21T00:50:24Z

@olamy, @janbartel - is there any particular reason why nested zip files are not supported? What I mean is:

Resource.newResource("jar:file://jug.jar!/WEB-INF/lib/full-of.jar!/WEB-INF/classes/beer.txt")

I need it - and if you've got nothing against it, i'd like to deliver it too.

-mb

joakime · 2019-07-21T02:08:57Z

@olamy, @janbartel - is there any particular reason why nested zip files are not supported? What I mean is:
Resource.newResource("jar:file://jug.jar!/WEB-INF/lib/full-of.jar!/WEB-INF/classes/beer.txt")

We use java.net.URLConnection to access these kinds of resources.
Which in turn is a java.net.JarURLConnection if the URL starts with jar:.
Nested resources are not supported by Java.
It will see the first level path /WEB-INF/lib/full-of.jar!/WEB-INF/classes/beer.txt and complain about "!/" not being valid for the path portion of the jar:file: spec.

The existence of nested jar resources is a packaging smell, and not recommended.

Perhaps you want to look at alternative packaging ...

mironbalcerzak · 2019-07-21T13:40:55Z

@joakime - thanks for swift answer.

It's not a smell when we think of WAR files.
I am testing how to deploy WAR files without extracting them - it's a massive deployment speed boost.
With my local implementation, I managed to start jetty-maven-plugin with overlays without "unpacking" them - all resources are retrieved from m2-repo, and processed "on-the-fly".
All seems pretty alright...

But to achieve it, I had to tweak slightly jdk internals (URLConnection, URLStreamHandler, JarFile), which I find it hacky, and not like too much...

Code-wise it's custom Handler that delegates to native logic if no nesting is detected and is invoked as new URL(url, spec, Handler) in JarResource - so, it's pretty much seemless.

Is this approach something you guys would like to see or not really? If not, what is your suggestion for said use case? "uber-WARs" or "live-jetty-war" is not a solution.

joakime · 2019-07-21T15:51:25Z

But to achieve it, I had to tweak slightly jdk internals (URLConnection, URLStreamHandler, JarFile), which I find it hacky, and not like too much...

I caution the use of URLStreamHandler in a WebApp / Servlet environment.
It's an exceedingly fragile concept, that many other open source projects have started moving away from. (it gets more fragile the more modern your JDK)

See Issues #1448 and #3700 for our fights with URLStreamHandler.

mironbalcerzak · 2019-07-21T15:57:08Z

Yup - that's why I am not particularly happy of what I did there. Have you got any other solution? Or should I stick with what I've done and let you guys review it once I finish and push?

olamy · 2019-07-22T10:53:02Z

@mironbalcerzak I'm not really keen to merge such changes without any test...

mironbalcerzak · 2019-07-22T11:08:07Z

@olamy - tests will surely come. firstly I want to finish the feature development - this PR is not close to be in merging state just yet. I've opened it so you guys can see what I am doing and review right away as it's quite a big change to jetty-maven-plugin.

janbartel · 2019-07-26T04:50:31Z

@mironbalcerzak first let me say I've just got off a very long international flight, so jetlag is addling my brain.

So, I'm having trouble understanding exactly what we're doing here. I thought you wanted to be able to use webapps that are overlay dependencies straight from the reactor, without requiring that they are built first, then unpacked? If that's the case, then I expect to see you applying the overlay rules to the contents of the reactor, rather than to the contents of the built and unpacked war files.

I don't understand your question about using static resources packed inside dependency webapps - these will be in the reactor and therefore not packed. Any dependency webapps that are not in the reactor will be handled by the current, unchanged code.

Where am I misunderstanding what you want to do?

mironbalcerzak · 2019-07-29T09:36:26Z

@janbartel - you are correct - however, as I am refactoring "here and there", I've found a way to skip "unpacking" phase for artifacts not present in maven reactor - and dealing with them right from m2_repo :)

janbartel · 2019-07-30T00:40:36Z

Neither the JVM classloader nor the jsp engine can deal with classes and resources packed into wars, hence the unpacking. Pursuing something like that would be a whole other dimension of difficult, so I suggest this PR sticks just to the original premise.

mironbalcerzak · 2019-08-05T20:38:52Z

@janbartel thanks for your comment =) You've got a point - i will finish this PR as soon as I can and eventually prepare more in the future.

janbartel · 2019-08-19T06:52:23Z

@mironbalcerzak wow there are a huge number of changes in this PR! Too many changes to be able to be considered for a stable release like 9.4.x, so I think you'll have to base these changes off jetty-10.0.x instead.

Also, instead of rewriting a lot of the jetty maven plugin code, I wonder could you look at providing more utility classes and methods that can be called? I ask because I am working on a total rewrite of the jetty maven plugin universe for jetty-10.0.x which would make it exceedingly difficult to merge your new code, even if we shared a common base of jetty-10.0.x. Making your changes in additional classes that can be called either from the existing jetty-10.0.x plugin code, or from the new refactored code would be easier to deal with.

I get the feeling that these changes are still experimental, right?

janbartel

See also my comment on the conversation tab.

...-plugin/src/main/java/org/eclipse/jetty/maven/plugin/classloader/MavenWebappClassloader.java

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/utils/OverlayUtils.java

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyWebAppContext.java

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/utils/ResourceUtils.java

mironbalcerzak · 2019-08-19T10:25:05Z

Hey @janbartel - thanks for keeping an eye on the thread 👍

@mironbalcerzak wow there are a huge number of changes in this PR! Too many changes to be able to be considered for a stable release like 9.4.x, so I think you'll have to base these changes off jetty-10.0.x instead.

Not that many if you take a look at what's done. Mostly refactored, repackaged and moved to utility classes the existing logic from jetty:run goal. I didn't check what's in jetty-10.0.x branch so not sure if it's possible to rebase it there.

Also, instead of rewriting a lot of the jetty maven plugin code, I wonder could you look at providing more utility classes and methods that can be called?

Which logic am I duplicating? I thought I already removed a lot of duplicates... :)

I ask because I am working on a total rewrite of the jetty maven plugin universe for jetty-10.0.x which would make it exceedingly difficult to merge your new code, even if we shared a common base of jetty-10.0.x. Making your changes in additional classes that can be called either from the existing jetty-10.0.x plugin code, or from the new refactored code would be easier to deal with.

I get the feeling that these changes are still experimental, right?

Well - no, not anymore :) i still need to fix the broken tests and tidy up the codebase that I introduced. Plugin behaves (almost) as expected in my testing scenario (as said - one more commit to sort all out). Do you use hangouts/skype (i prefer hangouts - my ID is the email that I sign-off my commits with)? I think it would be a whole lot easier to explain what's done, and what I hoped to achieve.

Cheers,

mb

janbartel · 2019-08-20T00:05:33Z

@mironbalcerzak wow there are a huge number of changes in this PR! Too many changes to be able to be considered for a stable release like 9.4.x, so I think you'll have to base these changes off jetty-10.0.x instead.

Not that many if you take a look at what's done. Mostly refactored, repackaged and moved to utility classes the existing logic from jetty:run goal. I didn't check what's in jetty-10.0.x branch so not sure if it's possible to rebase it there.

There are numerous changes to existing method signatures and field types, which is a no-no for a point release, which is why I'm saying it needs to be in a major release like jetty-10. The jetty-10.0.x branch as far as the jetty maven plugin should be very similar to the jetty-9.4.x branch, so it should be possible to reapply your changes to a jetty-10.0.x base.

Also, instead of rewriting a lot of the jetty maven plugin code, I wonder could you look at providing more utility classes and methods that can be called?

Which logic am I duplicating? I thought I already removed a lot of duplicates... :)

I'm not saying you are duplicating code, I'm saying that the code you're writing is tightly bound into the JettyRunMojo/AbstractJettyMojo. Given the massive re-write I'm doing in this branch: https://github.com/eclipse/jetty.project/tree/jetty-10.0.x-1743-refactor-maven-plugin-redux it would be really difficult to work out how to incorporate it. If you compartmentalized your code a bit into some helper classes then these classes (with a very clear and well-documented api :) ) would be easier to re-use.

I get the feeling that these changes are still experimental, right?

Well - no, not anymore :) i still need to fix the broken tests and tidy up the codebase that I introduced. Plugin behaves (almost) as expected in my testing scenario (as said - one more commit to sort all out).

I'd like to see more comments in the code - there's a lot of code and a heck of a lot of lines moved around, it makes it very difficult to evaluate. I'm not seeing the type of code I was expecting to see, so I need some more comments to show what it is you're doing. And yes, we definitely are going to need some it tests for this.

mironbalcerzak · 2019-09-01T14:32:00Z

hey @janbartel,
Sorry for not getting back to you earlier. Life's busy...

I'm not saying you are duplicating code, I'm saying that the code you're writing is tightly bound into the JettyRunMojo/AbstractJettyMojo. Given the massive re-write I'm doing in this branch: https://github.com/eclipse/jetty.project/tree/jetty-10.0.x-1743-refactor-maven-plugin-redux it would be really difficult to work out how to incorporate it. If you compartmentalized your code a bit into some helper classes then these classes (with a very clear and well-documented api :) ) would be easier to re-use.

Alright. I will move it there (10.x.x branch). Meanwhile, some code has been moved out of JettyRunMojo to seperate service class, meant only for setting "dependencies" up. Should be easier to reuse.

I'd like to see more comments in the code - there's a lot of code and a heck of a lot of lines moved around, it makes it very difficult to evaluate. I'm not seeing the type of code I was expecting to see, so I need some more comments to show what it is you're doing. And yes, we definitely are going to need some it tests for this.

Which part is not clear? I'd rather refactor it to the state that code is "just readable", without need for comments. I am not a fan of "if something was hard to develop, shouldn't be easy to read" sayin ;)
And more importantly - what type of code were you expecting to see? I'm still open for skype/hangouts (i see that the proposal was silently skipped) - this would save some time up on both sides (..or wouldn't it?)

mironbalcerzak · 2019-09-02T00:54:44Z

hey @olamy, i've pushed a fair number of integration tests - please take a look (jetty-run-mojo-overlays-it) when you get a chance, and tell me what you think.

Plugin is not behaving as expected in one corner case (will fix it in next commit).

…m maven Signed-off-by: Miron Balcerzak <[email protected]>

mironbalcerzak · 2019-09-20T14:31:59Z

Hi @janbartel - hope all's good. I haven't heard from you in a while. I am slightly confused. Is there any work needed to be done on my behalf? Or we call a day and should I just drop any further development? Have a good day -mb

olamy · 2019-09-24T07:01:47Z

...y-run-mojo-overlays-it/common/src/main/java/org/mca/jetty/common/ClasspathAnalyzeHelper.java

+//  ========================================================================
+//
+
+package org.mca.jetty.common;


wrong package

olamy · 2019-09-24T07:02:01Z

...run-mojo-overlays-it/common/src/main/java/org/mca/jetty/common/ClasspathDisplayListener.java

+//  ========================================================================
+//
+
+package org.mca.jetty.common;


wrong package

olamy · 2019-09-24T07:06:36Z

almost all it tests are failing now...

olamy · 2019-09-24T07:02:13Z

...ugin/src/it/jetty-run-mojo-overlays-it/common/src/main/java/org/mca/jetty/common/Common.java

+//  ========================================================================
+//
+
+package org.mca.jetty.common;


wrong package

olamy · 2019-09-24T07:02:31Z

...-mojo-overlays-it/module-a/module-a-api/src/main/java/org/mca/jetty/moduleA/ModuleA_api.java

+//  ========================================================================
+//
+
+package org.mca.jetty.moduleA;


wrong package

olamy · 2019-09-24T07:02:38Z

...ojo-overlays-it/module-a/module-a-impl/src/main/java/org/mca/jetty/moduleA/ModuleA_impl.java

+//  ========================================================================
+//
+
+package org.mca.jetty.moduleA;


wrong package

olamy · 2019-09-24T07:02:51Z

...o-overlays-it/module-a/module-a-war/src/main/java/org/mca/jetty/moduleA/ModuleA_servlet.java

+//  ========================================================================
+//
+
+package org.mca.jetty.moduleA;


wrong package

olamy · 2019-09-24T07:03:15Z

...o-overlays-it/module-b/module-b-war/src/main/java/org/mca/jetty/moduleB/ModuleB_servlet.java

+//  ========================================================================
+//
+
+package org.mca.jetty.moduleB;


wrong package

olamy · 2019-09-24T07:03:27Z

...-run-mojo-overlays-it/overlay1-war/src/main/java/org/mca/jetty/overlay/Overlay1_servlet.java

+//  ========================================================================
+//
+
+package org.mca.jetty.overlay;


wrong package

olamy · 2019-09-24T07:03:37Z

...rlays-it/overlay2/module-c/module-c-api/src/main/java/org/mca/jetty/moduleC/ModuleC_api.java

+//  ========================================================================
+//
+
+package org.mca.jetty.moduleC;


wrong package

janbartel · 2019-09-24T07:31:51Z

@mironbalcerzak sorry for the silence, been trying to land the big refactor of the plugins for jetty-10.0.x.

As I mentioned before, the changes proposed here are extensive, and thus not appropriate for jetty-9, this would have to be in jetty-10. Preferably, you'd base your work off the branch for the plugin refactor, which is https://github.com/eclipse/jetty.project/tree/jetty-10.0.x-1743-refactor-maven-plugin-redux .

Thinking about it some more, that refactor is a work-in-progress, so it might be much easier to wait until it is finalized and checked in to jetty-10.0.x branch - so you're not trying to work on something that is a moving target. I'm hoping to get a PR incorporated within the next 2 weeks.

mironbalcerzak · 2019-09-27T19:20:03Z

@janbartel no problem at all! that's a good idea - i will wait until it's finished.
@olamy yup - some ITs are broken (old tests) - they need to be fixed naturally. Ive introduced a big amount of new ones too - and that's what I meant by calling you over :)))

have a great day & all the best,
-mb

janbartel · 2019-10-02T00:16:54Z

@mironbalcerzak ok great! I'm still working on the refactor, but it's going very well. Feel free to take a look at the branch to start getting familiar with the new code. I'll close this PR for now.

janbartel added Do Not Merge wip labels Jul 11, 2019

janbartel reviewed Jul 11, 2019

View reviewed changes

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyRunMojo.java Outdated Show resolved Hide resolved

janbartel reviewed Jul 11, 2019

View reviewed changes

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyRunMojo.java Outdated Show resolved Hide resolved

janbartel reviewed Jul 11, 2019

View reviewed changes

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyRunMojo.java Outdated Show resolved Hide resolved

janbartel reviewed Jul 11, 2019

View reviewed changes

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyRunMojo.java Outdated Show resolved Hide resolved

mironbalcerzak force-pushed the jetty-9.4.x-3725-resolve-deps-from-reactor branch from c2d62d3 to 6c6bbb0 Compare August 5, 2019 17:59

janbartel requested changes Aug 19, 2019

View reviewed changes

janbartel added the Enhancement label Aug 19, 2019

olamy added the Need Test label Aug 28, 2019

mironbalcerzak force-pushed the jetty-9.4.x-3725-resolve-deps-from-reactor branch 6 times, most recently from 32abdf9 to 78c892d Compare September 2, 2019 00:50

mironbalcerzak force-pushed the jetty-9.4.x-3725-resolve-deps-from-reactor branch from 78c892d to 0206b89 Compare September 2, 2019 19:26

mironbalcerzak requested a review from janbartel September 13, 2019 16:44

jetty#3725 jetty-maven-plugin - WAR overlay dependencies resolved fro…

07d94a2

…m maven Signed-off-by: Miron Balcerzak <[email protected]>

mironbalcerzak force-pushed the jetty-9.4.x-3725-resolve-deps-from-reactor branch 2 times, most recently from cf51bb3 to 07d94a2 Compare September 17, 2019 00:34

olamy reviewed Sep 24, 2019

View reviewed changes

olamy requested changes Sep 24, 2019

View reviewed changes

janbartel closed this Oct 2, 2019

#3725 - jetty-maven-plugin - WAR overlay dependencies not resolved from maven reactor #3858

#3725 - jetty-maven-plugin - WAR overlay dependencies not resolved from maven reactor #3858

Conversation

mironbalcerzak commented Jul 8, 2019 • edited Loading

janbartel commented Jul 11, 2019

mironbalcerzak commented Jul 11, 2019 • edited Loading

janbartel commented Jul 11, 2019

mironbalcerzak commented Jul 11, 2019 • edited Loading

olamy commented Jul 16, 2019 • edited Loading

mironbalcerzak commented Jul 16, 2019

olamy commented Jul 17, 2019

mironbalcerzak commented Jul 21, 2019

joakime commented Jul 21, 2019 • edited Loading

mironbalcerzak commented Jul 21, 2019 • edited Loading

joakime commented Jul 21, 2019

mironbalcerzak commented Jul 21, 2019 • edited Loading

olamy commented Jul 22, 2019

mironbalcerzak commented Jul 22, 2019 • edited Loading

janbartel commented Jul 26, 2019

mironbalcerzak commented Jul 29, 2019

janbartel commented Jul 30, 2019

mironbalcerzak commented Aug 5, 2019

janbartel commented Aug 19, 2019

janbartel left a comment

Choose a reason for hiding this comment

mironbalcerzak commented Aug 19, 2019 • edited Loading

janbartel commented Aug 20, 2019

mironbalcerzak commented Sep 1, 2019 • edited Loading

mironbalcerzak commented Sep 2, 2019

mironbalcerzak commented Sep 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olamy commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbartel commented Sep 24, 2019

mironbalcerzak commented Sep 27, 2019 • edited Loading

janbartel commented Oct 2, 2019

mironbalcerzak commented Jul 8, 2019 •

edited

Loading

mironbalcerzak commented Jul 11, 2019 •

edited

Loading

mironbalcerzak commented Jul 11, 2019 •

edited

Loading

olamy commented Jul 16, 2019 •

edited

Loading

joakime commented Jul 21, 2019 •

edited

Loading

mironbalcerzak commented Jul 21, 2019 •

edited

Loading

mironbalcerzak commented Jul 21, 2019 •

edited

Loading

mironbalcerzak commented Jul 22, 2019 •

edited

Loading

mironbalcerzak commented Aug 19, 2019 •

edited

Loading

mironbalcerzak commented Sep 1, 2019 •

edited

Loading

mironbalcerzak commented Sep 27, 2019 •

edited

Loading