-
Notifications
You must be signed in to change notification settings - Fork 91
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
Support for filters (Maven variables) & WAR overlays - PART 1: WAR modules #1104
Comments
@scottkurz That completely works for me. I can then control it with Maven Profiles whether its Development or Production mode. |
Actually the We'll have to think about that a bit. @melloware thanks for the quick feedback before. What's your opinion on this latest point? Is this useful to you if it only performs filtering in the web.xml file but not the beans.xml or faces-config.xml files? |
I think web.xml is definitely important but I am not sure if others will run into the same issue with their other XML configs I can definitely see the need. |
Thinking about this a bit more... I think we probably should include beans.xml or faces-config.xml in the solution, to handle war plugin config like: <webResources>
<resource>
<directory>${basedir}/src/main/webapp</directory>
<filtering>true</filtering> I think we have to decide roughly between:
(or some variant in between somehow). ANOTHER ISSUEFinally, I don't think the dev mode loop is handling filtering even in, e.g. src/main/resources/META-INF/persistence.xml with a simple POM snippet like:
which maybe should also be fixed at the same time here. |
I have been giving this some more thought. I'm wondering if we should instead use the E.g. build the loose app as: <archive>
<dir sourceOnDisk=".../project/target/myapp-1.0" targetInArchive="/"/>
<file sourceOnDisk=".../project/target/tmp/META-INF/MANIFEST.MF" targetInArchive="/META-INF/MANIFEST.MF"/>
</archive> We could modify the dev mode loop by adding new calls to:
This one change would add support for "filters" both in src/main/resources as well as src/main/webapp since both get copied into this directory. It would also allow us to support WAR overlays which https://dzone.com/articles/mavens-war-overlay-what-are the loose app format currently doesn't seem to handle. I think that the war plugin is "smart" about only copying new dependencies and deleting "stale" dependencies... I think the performance hit should be minimal. Talking more internally on the subject but wanted to post this update. |
I believe that is how the Maven Jetty plugin works it allows you to use overlays. |
Noting another request to allow overlays: #593 |
Was thinking whether I should look at the plugin-level configuration (of maven-war-plugin) or pick a specific goal's configuration. Though the change in #767 adding support for |
Made some progress here: I haven't spent any time on trying to detect when/how to use this loose app config, so am assuming it's triggered by a new config parm: "exploded=true" (defaults to 'false') |
Prototype seems to be working fairly well. Two points for further investigation:
Even if we can auto-detect, we still might need an explicit switch, e.g. to support overlays. That could lead to a choice of "exploded=true | false | auto" perhaps ? Getting ahead of myself, that's a bit further down the road. |
One final point we might need to clarify is that we're talking about a build time, not a run time substitution. That is, we're not saying you can deploy a web.xml with contents including: |
SETUP FOR BIG WARTo address performance concerns, I did some measurements enlarging my test WAR in two ways:
Observations:
If you were going to package a WAR eventually you'd have had to have paid this later, but if you were just going to push to Git after your changes this is new, extra. You don't have to pay iteratively because the war plugin uses a "smart" copy, so it's akin to installing Liberty and the features .
We seem to need to do something like only watch web resource paths when filtering is enabled (parsing the war/resources config). What I'm not sure of is if we would then simply build the loose app from the single "exploded" directory: OVERLAYFinally, it's worth nothing that for this particular app, io.openliberty... it might fit well into an "overlay" type of use case. I realize it might not be worth modifying the build now, but interesting to realize it'd be an option if we went ahead with this new function... in which case none of these webResources are "source" at all for this particular module (so don't need to be monitored).. but rather the jekyll build becomes more like a dependency. |
Yeah, now that the design suggests not watching/monitoring non-"filtered" web resources dirs, the running app would no longer pick up any changes to those resources (neither static HTML or something like beans.xml). So the loose app needs to be defined with a more fine-grained mix from the different locations. Not a complete rewrite needed, but have to rework and retest. |
PROTOTYPE UPDATEReworked to only monitor "filtered" source resource directories: The fact that loose apps guarantee loading the "virtual" contents in element order within the loose app XML file, per: <archive>
<!-- One dir of unfiltered web resources, as source -->
<dir sourceOnDisk="C:\git\sample.batch.bonuspayout\src\main\more-unfiltered-resources" targetInArchive="/"/>
<!-- Another dir of unfiltered web resources, as source -->
<dir sourceOnDisk="C:\git\sample.batch.bonuspayout\target\jekyll-webapp" targetInArchive="/"/>
<!-- The target web app dir, which has classes, resources, and filtered web resources (with lower precedence than the previous two) -->
<dir sourceOnDisk="C:\git\sample.batch.bonuspayout\target\myapp-1.0" targetInArchive="/"/>
<file sourceOnDisk="C:\git\sample.batch.bonuspayout\target\tmp\META-INF\MANIFEST.MF" targetInArchive="/META-INF/MANIFEST.MF"/> PERFORMANCENow that we have eliminated scanning webResources dirs for which filtering is disabled, the only remaining performance concern is the ONE-TIME copying of dependencies and other resources into the target webapp dir. It is a one-time hit because the war:exploded logic is "mostly" smart about avoiding unnecessary re-copying. As mentioned before, a few bigger smaller files aren't a big deal but many, even smaller files, can add time.. e.g. up to 30s for a large app like the openlibery.io website jekyll deps. Note that if you were ever going to do a mvn package or war:war you'd pay this cost anyway, and so it's just a matter of shifting when you would pay this cost, upfront during initial execution of dev mode or later when doing the package. It's only in the case where you never build the WAR package that this is a purely extra penalty....though this case would still be fairly common so it is a concern. But this could be addressed too by not making this the default, and requiring a new parameter like -Dexploded (as in this prototype), to enable the new behavior |
@melloware can I just ask you to confirm one thing here ? I think we are clear on this and have been following along on the same page since you started the discussion but... I just want to be clear that what I'm proposing involves resolving the filter-type substitutions at build time... rather than at run time. If you are running out of dev mode, of course, the distinction collapses, since dev mode involves building and running in one easy step. But I'm just trying to clarify that we're NOT proposing allowing you to package a server with an app deployed include a web.xml with a Does that sound like we are still on the same page? Thanks for your input here. |
Correct I am just saying when running in DEV the substitution should be applied and when packaging for PRD the substitution should be applied not that is is dynamic in a packaged WAR file. |
Since we are discussing filtering at the Epic, high-level, we should probably consider server.xml filtering at the same time, e.g. this issue: #587. Though the implementation may be different we might want to say, enable both with a common external (plugin parameter). |
UFO (design) link: https://ibm.box.com/s/k6zinwfzvwfzfp0fl4mv7fv9mbmuubfa |
UFO Review Recording - https://ibm.ent.box.com/file/846043148864 |
Changed property name to: |
One subtlety I thought of since the design review: in the existing loose app format, a change to a dep in a local m2 repo will cause the app to recycle...since the loose app points to the dep location in the local m2. Under the new exploded loose app design this would not be the case, since the deps would typically now come from the target webapp dir. This could in theory affect a "shared library" type of use case in which a separate module is updated and built (and installed into m2 repo) while dev mode is active for an app consuming this "shared lib" (as a dep). The user could sort of workaround this limitation by making a trivial pom.xml change to force another call to war:exploded. Perhaps that's a good question to ask more generally even..is there a way to force another call to war:exploded? |
UPDATE: having merged: #1289 Roughly we are DONE:
We did NOT yet get to:
One thing I'm not sure about is:
|
@melloware - I just wanted to give you in particular an update, since you opened the original issue. In the As I maybe hinted at in my earlier comment we are likely to release this level of support in our next release of the plugin and implement the rest at a later date. If you are interested in trying it out, you could download the snapshot by adding to your POM: <pluginRepositories>
<!-- Configure Sonatype OSS Maven snapshots repository -->
<pluginRepository>
<id>sonatype-nexus-snapshots</id>
<name>Sonatype Nexus Snapshots</name>
<url>https://oss.sonatype.org/content/repositories/snapshots/</url>
<snapshots>
<enabled>true</enabled>
</snapshots>
<releases>
<enabled>false</enabled>
</releases>
</pluginRepository>
</pluginRepositories> No special config is required to use the new function; the If you are able to give it a try and have any feedback we would be happy to hear. Thank you! |
Nice work! |
@melloware - Thanks for the quick response. Can I take that to mean you had a chance to try it out ? |
yep it works for my use case of switching the JSF Development stage |
I will monitor once this is released and update my Liberty Faces example project to do this. |
See #1104 (comment) for where we are. We will look to make available "overlay & resource filtering enabled for single-WAR-modules" in the next release of the plugin. |
Let me summarize again since the earlier comment summarizing: #1104 (comment) included a maybe confusing question.
|
Delivered the final fix: #1714 targeted for liberty-maven-plugin v3.8.3. |
ISSUE
This use case was brought up first as a the runtime issue: OpenLiberty/open-liberty#16133
I think a useful starting point, rather than requiring even more invention unique to liberty-maven-plugin, is to assume the user is maven-war-plugin configuration:
PROPOSAL OUTLINE
So dev mode and loose app support in general would need to be enhanced to support a "filtered" web.xml, beans.xml, etc.
At a lower level, the algorithm we'd need to use could be something like:
filteringDeploymentDescriptors
Unclear to me if 2. is already captured in some function we can call (either goal or at least API) or do we have to reverse engineer it? Maybe https://maven.apache.org/plugins/maven-war-plugin/exploded-mojo.html could help?
MORE CONTEXT
For src/main/resources content, like persistence.xml, OTOH.. I think this already "just works", since loose app is pointing to "target/classes" and I'm assuming the 'resources' goal already in dev mode loop does the right thing. It would be worth spending a bit of effort manually confirming
THOUGHTS ?
@melloware - does this outline sound like it would be useful to you? Would use of
filteringDeploymentDescriptors
fit your needs? If not would you be able to give some explanation and maybe examples of your Maven POM showing what might be a better fit instead? (Thank you.)The text was updated successfully, but these errors were encountered: