You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/04/06 00:46:38 UTC
[GitHub] [maven-shade-plugin] gzm55 opened a new pull request, #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
gzm55 opened a new pull request, #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129
The runtime injected pom will be set via `MavenProject::setFile` or`MavenProject::setPomFile`, including this plugin and the flatten plugin. So to chain these kind of plugins, when generating the d-r-p pom, this pr first tries to parse a model directly from `MavenProject::getFile` and fallback to a clone of `MavenProject::getOriginalModel` if no pom file set.
Following this checklist to help us incorporate your contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MSHADE) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MSHADE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MSHADE-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] gzm55 commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
gzm55 commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843928400
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
+ ? project.getOriginalModel().clone()
+ : new MavenXpp3Reader().read( new FileInputStream( project.getFile() ) );
Review Comment:
ok, now we use ModelReader to parse the pom
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] gzm55 commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
gzm55 commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843673856
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
hi @rmannibucau , the in Maven, what is 'originalModel` defined to be, a model of the original pom or a model of the current pom file? If defined as the latter one, the global solution of updating originalModel could be done in `MavenProject::setPomFile` or `MavenProject::setFile`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843529949
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
looks like an issue any plugin consuming the pom will get so it should likely be fixed in flatten plugin using `setOriginalModel` to be a global fix and not a local one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] gzm55 commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
gzm55 commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843673856
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
hi @rmannibucau , the in Maven, what is `originalModel` defined to be, a model of the original pom or a model of the current pom file? If defined as the latter one, the global solution of updating originalModel could be done in `MavenProject::setPomFile` or `MavenProject::setFile`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843918306
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
+ ? project.getOriginalModel().clone()
+ : new MavenXpp3Reader().read( new FileInputStream( project.getFile() ) );
Review Comment:
I think we can blame "history" there, xpp3 is kind of old these days ;).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843877455
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
+ ? project.getOriginalModel().clone()
+ : new MavenXpp3Reader().read( new FileInputStream( project.getFile() ) );
Review Comment:
the inputstream will not be closed but the correct way to read a pom is org.apache.maven.model.io.ModelReader anyway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843924166
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
Hmm, I just checked flatten code and it only works if `updatePomFile` is set to true no? So long story short, if you want it to work reliably you must ensure originalModel is set before shade plugin is executed. From a design point of view it is the responsability of flatten plugin to do that since it is the one hacking the user pom but an intermediary plugin setting it from a file when flatten plugin dumped its value would work too but I don't think there is any bug in shade there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] gzm55 commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
gzm55 commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843879478
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
+ ? project.getOriginalModel().clone()
+ : new MavenXpp3Reader().read( new FileInputStream( project.getFile() ) );
Review Comment:
thx, let me fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] gzm55 commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
gzm55 commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r844060718
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
it seems better to have a convention or rule that the original model should be equal to the pom file. the shade plugin also breaking this convention, it reverts the parent.relativePath after write the model to the file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] gzm55 commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
gzm55 commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843730700
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
ok, this pr is the first choice, which prefer a practice single source to get current raw model.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] gzm55 commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
gzm55 commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843882391
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
+ ? project.getOriginalModel().clone()
+ : new MavenXpp3Reader().read( new FileInputStream( project.getFile() ) );
Review Comment:
the flatten plugins (not only https://github.com/mojohaus) are using `MavenXpp3Reader` way~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] jglick commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
jglick commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843869980
##########
src/it/projects/MSHADE-323-compatible-with-flatten-pom/pom.xml:
##########
@@ -0,0 +1,84 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<project>
+ <modelVersion>4.0.0</modelVersion>
+
+ <groupId>org.apache.maven.its.shade.flatten</groupId>
+ <artifactId>test</artifactId>
+ <version>1.0</version>
+
+ <name>MSHADE-323</name>
+ <description>
+ Test that integration of shade and flatten plugins.
Review Comment:
```suggestion
Test the integration of Shade and Flatten plugins.
```
##########
src/it/projects/MSHADE-323-compatible-with-flatten-pom/verify.groovy:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+File pomFile = new File( basedir, "target/dependency-reduced-pom.xml" );
+assert pomFile.isFile()
+
+def ns = new groovy.xml.Namespace("http://maven.apache.org/POM/4.0.0")
Review Comment:
```suggestion
def ns = new groovy.xml.Namespace( "http://maven.apache.org/POM/4.0.0" )
```
for consistency
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
+ ? project.getOriginalModel().clone()
+ : new MavenXpp3Reader().read( new FileInputStream( project.getFile() ) );
Review Comment:
Possible resource leak?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] gzm55 commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
gzm55 commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843874851
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
+ ? project.getOriginalModel().clone()
+ : new MavenXpp3Reader().read( new FileInputStream( project.getFile() ) );
Review Comment:
@jglick how to leak the resource?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] gzm55 commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
gzm55 commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843876935
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
it is ok to not fall back to original model when `getFile()` is null, throwing an exception instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843825833
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
@gzm55 this pr does not impl 1 since it should never use original model in case 1 but always file to enable to chain plugins.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r843695691
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
My guess is that it was the really original file but since maven is a chain (per module) it is in practise the current pom file - otherwise it can't really be used by any plugins and some do.
So guess the choice is:
1. keep original untouched but means no plugin should use it except help/debug ones (so fix is to always read getFile())
2. ensure flatten plugin updates original model to let downstream plugins consumes the post processing model
Personally I think the option 2 is what makes the most sense for the ecosystem (inter-plugin operability).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on a diff in pull request #129: [MSHADE-323] make shade plugin compatible with the flatten plugin
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #129:
URL: https://github.com/apache/maven-shade-plugin/pull/129#discussion_r844129180
##########
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java:
##########
@@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}
- Model model = project.getOriginalModel();
+ final Model model = project.getFile() == null
Review Comment:
shade plugins reset it cause it just needs to change it while dumping the pom (that said good point it breaks thread safety in concurrent builds).
agree shade plugin breaks this rules and happy to get it fixed, it is just not that important cause shade plugin is generally in the end of the chain (so dowstream consumers list is empty) and it also handles itself the pom rewriting but you are right.
long story short: I don't think we should re-read from the filesystem any pom model anytime in maven build otherwise the design itself is broken so I'm in favor of trying to not use the reader more than once per file.
hope it makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org