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 2021/04/16 19:35:40 UTC
[GitHub] [maven-shade-plugin] aalmiray opened a new pull request #90: [MSHADE-382] - Add property to skip execution
aalmiray opened a new pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90
Added a skip field to ShadeMojo to skip the execution.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] 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.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] 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.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] 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.
- [x] 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826120055
@mkarg if you need to skip once as a test you dont need skip in general, common case tend to be a faster "dev" build (skip fatjars, skip shades/relocations, skip assembly, skip docker, ...) otherwise you can just comment the plugin doing investigations so not sure this example is the most relevant one (at least for me). Robert's point is only about 20% of the plugin usage or so but quite relevant and his proposal still enables 100% of the cases so my humble proposal would be to go with it for next release or expect this issue to stay hanging and only solvable by profiles (unsatisfying for mentionned cases) or extensions (external or custom so not always good). Indeed just my 2cts.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-869042366
> The parameter should have the general `skip` name.
@rfscholte do you mean the filed must have `@Parameter( property = "skip")` ?
--
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] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-869071947
It should be like this:
@Parameter
private boolean skip;
no property, defaultValue is already `false` so can be omitted.
--
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] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826118368
`maven.main.skip `is on my list to be removed. These decisions from the past are very hard to revert and feed people to say that every goal should need a skip parameter, because they are used everywhere.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841656807
Switching to `skipClassified` would help only when the shaded artifact has a classifier. Given that the default behavior of the shade plugin is to override the original artifact there can not be a classifier; the use case I presented also does not configure a classifier (matter of fact if it did then skip might not even be needed as what _I_ need is the original artifact to begin with, then again it's standard procedure to **not** provide a classifier (and I understand it's not mandated, it's a preference)).
Thus `skipClassifier` would be limited in this regard en provide not solution to the use case at hand. I think sticking with just `<skip>` is OK for now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826277638
@rmannibucau I don't get that - your vote on Github still is +1 for this PR. So why didn't you change it to -1 clearly requesting the change Robert proposed?
About your side note, this is unrealistic. *Any* change of *any* POM will break a build, and in reality least people like to put tests *ontop* that check that the POM itself wasn't incorrect. The POM *is* the single source of truth, so it *must* be relied upon, hence it *must never* be temporarily fiddled with. Adding such constraints intop to proof the consistency of the POM make the POM simply bloated and unreadable, cost a lot of additional development time, and are everything but CoC anymore. What users of Maven expect is smallest possible POMs and allowing to temporarily override POM settings at the command line, without chaning POMs all the time. This might not what you like to hear, but it is what the average Joe simply does and expects from this tool. That is why I think, we should listen more carefully to users instead of always concentrating on the inner circle's opinions.
Having said that, if the majority has such big trouble with our PR, maybe it is best if Andres and me simply change it, even if we disagree/dislike, just to have *any* progress here. But really, this makes me totally doubt about the way this community applies votes...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-840676516
@aalmiray
> I'm afraid I do not understand what's being proposed here. The fact that shade replaces the original artifact by_default is the root of the problem. IMHO that's the wrong thing to do however I'm aware that the current behavior has been like this for, well, since forever, and changing it would be more catastrophic than adding a skip field with a default property name.
Not really, the issue is you have multiple use cases and some are valid (replacing the root artifact because you relocate or want to do a fatjar), others are not (because you want a convenient binary).
In this last case you add a classifier and it is the case we want to be able to skip.
Therefore I proposed to not do a "skip" but "skipClassified" toggle which would not skip cases where root artifact is replaced but skip all other cases (artifacts with classifiers).
Guess it solves concerns of everybody.
@mkarg official way is to find a compromise and not try to force in any direction ;). Discussions are only stucked when people don't want to evolve in any direction which was not the case there AFAIK.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826079568
re: logging. We thought about it but decided to leave it out for the time being. We were sure this topic would come up during the review 😉
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826253029
@mkarg it is not only about Robert's -1 now he explained and proposed a solution but the proposed solution got the preference since it fixes properly both point so maybe just fix the PR ;). Side note: if modifying the pom can break your build, ensure you test what you deliver and make it xonditional with the *same* property (kind of confirms Robert's point is more than accurate).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841655691
Ok, i'll rephrase a last time if it is still ambiguous: since removing the default still has the pitfall to enable to skip the module main artifact, proposal was to skip artifacts with a classifier only (therefore the `skipClassified` toggle instead of `skip`). Solves the "can break the default delivery" point and enables to skip sibling deliveries in dev.
Hope it is clearer phrased this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Tibor17 commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882882099
@aalmiray Thx for contributing!
--
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 pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826077229
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841301304
I don't understand what you mean with the last line. I explicitly said I am ok with the proposal to remove default property, and asked Andres for his opinion. What else shall be my homework?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841654686
> @mkarg [#90 (comment)](https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-828131563) or next message explaining/rephrasing it proposed an unifying option
Still don't get what *I* could have done here, as that is @aalmiray 's PR, so it is up to him to comment on that proposals. I already wrote that I am fine with removing the default.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on a change in pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on a change in pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#discussion_r659273897
##########
File path: src/test/java/org/apache/maven/plugins/shade/mojo/ShadeMojoTest.java
##########
@@ -62,6 +62,43 @@
public class ShadeMojoTest
extends PlexusTestCase
{
+ public void testSkipShading() throws Exception
+ {
+ final ShadeMojo mojo = new ShadeMojo();
Review comment:
We dropped this meanwhile.
--
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 pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826105382
@mkarg it solves your use case more cleanly since you intentionally enable to skip it because your project can so guess it is not a bad compromise: enable without breaking other cases by default, more "maven spirit" ;).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882482799
--
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 pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826077229
@rfscholte do you have an alternate solution to the recurrent mentionned issue or do we move forward with this obvious fix which seems to make everyone happy?
Side question: should skipping be logged at info/warning level? Got surprised to not see it testing it on some woek project earlier this week.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841788833
The parameter should have the general `skip` name.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] stephenc commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
stephenc commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826128207
If @rfscholte is coming from the view that command line arguments shouldn't affect the built artifact... (which I agree with) and if robert intends to put energy behind making that happen... then I'm against adding technical debt (which this parameter would be introducing)
If robert is not prepared to put energy behind removing the other skip properties and starting to flag warnings if your pom dependency versions are controlled by a property... then from a pragmatist view let's add the tech debt.
Really a question of where Robert is preparing to spend his energy as to whether I think we should merge or not
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg edited a comment on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg edited a comment on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826286867
@aalmiray For the sake of getting progress, I give up to insist on the command line property. Andres, will you remove 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826086792
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826125941
In fact I *hate* modifying the POM just to skip things *temporarily*. This bears the risk to inadvertently add POM changes to a commit. Having a command-line-only way to *temporarily* override settings is a real bonus for *quick* work with Maven. Actually I would love to have a generic solution that allows me to override *anything* in the POM using a syntax like `-override project.build.plugins.plugin... etc ...`. But this is a different story. ;-)
So to sum up, if we all are +1 for the PR as it is, but just Robert is -1, then this effectively leads to rejection of this PR?
If *that* is how this community works, then certainly I will take Robert's proposal (but I would be rather sad as it effectively means that Robert's opinion is worth more than the voices of all those people out there that keep telling him that *they* want to have skip for each goal). Don't get me wrong, if this is how Apache works, I am OK with. But I always thought that it would be a good thing to listen to the users's requests. And as Robert said himself: They *keep* asking for skip (and IMHO for good reason).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-821790532
True. Except my use case requires the plain JAR or the shaded JAR depending on a condition. Skipping jar altogether would be bad. In other words, this is not to stop shading altogether but to have a choice to stop it when needed, even when the parent pom (or a custom lifecycle which is exactly the case mentioned in the JIRA ticket) activates shade by default.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rfscholte commented on a change in pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#discussion_r633064165
##########
File path: src/test/java/org/apache/maven/plugins/shade/mojo/ShadeMojoTest.java
##########
@@ -62,6 +62,43 @@
public class ShadeMojoTest
extends PlexusTestCase
{
+ public void testSkipShading() throws Exception
+ {
+ final ShadeMojo mojo = new ShadeMojo();
Review comment:
This is not a not a PlexusTest, but a unittest using reflection. And I understand it follows other test examples, but this keeps increasing the technical dept.
I suggest to drop this as the non-default value is already covered by the integration test, the default value with all the other tests.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841685349
Ok, if ok with Robert let's just drop the property value then
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882883233
Lovely. Thank you @Tibor17 for merging and everyone else for your help and patience. Go Maven!
--
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] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826092118
@rmannibucau that is indeed what i am saying
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Tibor17 commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882876324
--
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] Tibor17 commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882132478
@aalmiray Pls squash these commits in one. Thx
--
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] aalmiray commented on a change in pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on a change in pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#discussion_r671859020
##########
File path: src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java
##########
@@ -385,12 +385,23 @@
@Inject
private Map<String, Shader> shaders;
+ /**
+ * When true, skips the execution of this MOJO.
+ * @since 3.3.0
+ */
+ @Parameter( defaultValue = "false" )
+ private boolean skip;
+
/**
* @throws MojoExecutionException in case of an error.
*/
public void execute()
throws MojoExecutionException
{
+ if ( skip )
+ {
+ return;
Review comment:
Done. Added a logging statement
--
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] aalmiray edited a comment on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray edited a comment on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841656807
Switching to `skipClassified` would help only when the shaded artifact has a classifier. Given that the default behavior of the shade plugin is to override the original artifact there can not be a classifier; the use case I presented also does not configure a classifier (matter of fact if it did then skip might not even be needed as what _I_ need is the original artifact to begin with, then again it's standard procedure to **not** provide a classifier (and I understand it's not mandated, it's a preference)).
Thus `skipClassified` would be limited in this regard en provide not solution to the use case at hand. I think sticking with just `<skip>` is OK for now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826283496
> If @rfscholte is coming from the view that command line arguments shouldn't affect the built artifact... (which I agree with) and if robert intends to put energy behind making that happen...
Yes, I want to put energy into more reliable builds wrt commandline arguments as it is highly underrated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826072860
Added more +1 arguments at https://issues.apache.org/jira/browse/MSHADE-382.
Looking at the votings there is a rather clear majority pro this feature, but just Robert is against it. How to proceed?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-828211073
> > removing the @parameter annotation is the compromise that will let this PR be merged.
>
> Just the property value (you can keep it to get it configurable in the pom and have a properly documented default.
OK
> > Thus removing the parameter helps newbies but irks experts.
>
> As explained before it is not really true.
Alright, I wouldn't call myself a Maven expert but I'm definitely not a newbie anymore. Still this irks me, so ...
> What about not adding a skip parameter but a skipClassified parameter which can have a property (hopefully you find a shorter name ;))? Issue is only about the default artifact, not all other executions.
> Would it make everyone happy?
I'm afraid I do not understand what's being proposed here. The fact that shade replaces the original artifact _by_default_ is the root of the problem. IMHO that's the wrong thing to do however I'm aware that the current behavior has been like this for, well, since forever, and changing it would be more catastrophic than adding a skip field with a default property name.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Tibor17 merged pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
Tibor17 merged pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90
--
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] eolivelli commented on a change in pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#discussion_r671332232
##########
File path: src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java
##########
@@ -385,12 +385,23 @@
@Inject
private Map<String, Shader> shaders;
+ /**
+ * When true, skips the execution of this MOJO.
+ * @since 3.3.0
+ */
+ @Parameter( defaultValue = "false" )
+ private boolean skip;
+
/**
* @throws MojoExecutionException in case of an error.
*/
public void execute()
throws MojoExecutionException
{
+ if ( skip )
+ {
+ return;
Review comment:
Can we log something ?
--
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] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-840705795
@aalmiray I would be fine with dropping just the *default* property name if this really is the sole point which makes this discussion stuck. WDYT?
I also would love to hear from other committers what their proposed compromise would be.
> @mkarg official way is to find a compromise and not try to force in any direction ;). Discussions are only stucked when people don't want to evolve in any direction which was not the case there AFAIK.
I am always willing to learn the rules of a community. Where can I find the official ruleset which contains the request to find a *compromise*? I am asking because in other communities simply the majority wins.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826086792
I'm afraid I can't convince most of you that this a bad proposal, and that it is a lazy workaround for an issue somewhere else. Probably POM model 4.0.0 is missing an option to better control this so for only for that reason I'm willing to allow this skip parameter.
However, as said before, having the skip parameter exposed as a property is too dangerous.
I know a lot of people don't fully understand properties, so they are not aware of the consequences.
For people that want to be able to control it from commandline could add `<skip>${shade.skip}</skip>`, as can be done with any other parameter.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882482799
> @aalmiray Pls squash these commits in one. Thx
@Tibor17 I've squashed the commits as requested 😄
--
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 pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826088147
@rfscholte does not providing a *built-in* property makes you happy (did I get it right)? If so I'm fine with that.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-840673415
This discussion seems to be stuck. What is the official Apache way to resolve a situation like this, where three committers are +1 but two commiters are -1? Just not respond anymore (I doubt that)?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826072860
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826292651
@all: Would be great to have a public list of all the "unspoken design targets" like "getting rid of properties that affect build result". Can you please set this up and link here, so contributors know *upfront* what existing patterns they shall not further copy? Having said that, it would be nice to cast your vetos rather soon after someone filed a *JIRA*, not waiting him to actually invest time into code that will be rejected, as it happened here. I thought that actually is why we are asked to file JIRAs *upfront*.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826079568
re: logging. We thought about it but decided to leave it out for the time being. We were sure this topic would come up during the review 😉
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-840722813
@mkarg most rules are available at https://community.apache.org/. You can find some way to "pass in force" but it is against the spirit of ASF so please don't biase what is written (and makes sense in some cases), in particular for trivial case like this one - was intended for more complex cases. Generally speaking if you can't find a technical compromise for something technical, the issue is not in code if you follow the reasoning ;). Last note is that from my experience you shouldn't await too much from others - until you create a thread on the dev list to highlight you are awaiting from people.
BTW you didn't pronounced yourself on the last proposal and stated the thread was stucked so maybe some homework to do too?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] stephenc commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
stephenc commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826128207
If @rfscholte is coming from the view that command line arguments shouldn't affect the built artifact... (which I agree with) and if robert intends to put energy behind making that happen... then I'm against adding technical debt (which this parameter would be introducing)
If robert is not prepared to put energy behind removing the other skip properties and starting to flag warnings if your pom dependency versions are controlled by a property... then from a pragmatist view let's add the tech debt.
Really a question of where Robert is preparing to spend his energy as to whether I think we should merge or not
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826101756
No commandline argument should have impact on the outcome of the artifact. Once there are issues it will be hard to reproduce the original artifact and to do the correct analysis. Did you know that with Spring Boot is it very easy to adjust dependency versions from commandline? The uploaded pom will be the same, but the code might have been compiled against a different set of dependencies.
Hence I really want to protect the average Maven users against unintended mistakes.
If you do understand Maven, you know how to control parameters and you may decide to introduce a property.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rfscholte commented on a change in pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#discussion_r633064165
##########
File path: src/test/java/org/apache/maven/plugins/shade/mojo/ShadeMojoTest.java
##########
@@ -62,6 +62,43 @@
public class ShadeMojoTest
extends PlexusTestCase
{
+ public void testSkipShading() throws Exception
+ {
+ final ShadeMojo mojo = new ShadeMojo();
Review comment:
This is not a not a PlexusTest, but a unittest using reflection. And I understand it follows other test examples, but this keeps increasing the technical dept.
I suggest to drop this as the non-default value is already covered by the integration test, the default value with all the other tests.
##########
File path: src/it/projects/MSHADE-382_skip_execution/verify.bsh
##########
@@ -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.
+ */
+import java.io.*;
+
+File originalJarFile = new File( basedir, "target/original-skip-execution-1.0.jar" );
Review comment:
Groovy is the standard nowadays, which would probably make this a oneliner, something like:
`assert !(new File( basedir, "target/original-skip-execution-1.0.jar" ).exists())`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Tibor17 commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882876324
The CI is running...
Let's wait for the outcome.
--
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] Tibor17 commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882876324
--
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] Tibor17 commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882132625
If there are no objections, we can close this PR with push.
--
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] Tibor17 merged pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
Tibor17 merged pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90
--
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] aalmiray edited a comment on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray edited a comment on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841656807
Switching to `skipClassified` would help only when the shaded artifact has a classifier. Given that the default behavior of the shade plugin is to override the original artifact there can be no classifier; the use case I presented also does not configure a classifier (matter of fact if it did then skip might not even be needed as what _I_ need is the original artifact to begin with, then again it's standard procedure to **not** provide a classifier (and I understand it's not mandated, it's a preference)).
Thus `skipClassified` would be limited in this regard and provide no solution to the use case at hand. I think sticking with just `<skip>` is OK for now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Tibor17 merged pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
Tibor17 merged pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90
--
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 pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-828131563
> removing the @Parameter annotation is the compromise that will let this PR be merged.
Just the property value (you can keep it to get it configurable in the pom and have a properly documented default.
> Thus removing the parameter helps newbies but irks experts.
As explained before it is not really true.
What about not adding a skip parameter but a skipClassified parameter which can have a property (hopefully you find a shorter name ;))? Issue is only about the default artifact, not all other executions.
Would it make everyone happy?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-869047353
No, this property would break builds too easily. Guess it was more against skipClassified and his previous point making skip acceptable as a compromise.
--
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] aalmiray edited a comment on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray edited a comment on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-869042366
> The parameter should have the general `skip` name.
@rfscholte do you mean the field must have `@Parameter( property = "skip")` ?
--
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] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826278577
@stephenc I share your opinion and think that if we want to really *enforce* Robert's wish, then he should not only discard `maven.main.skip`, but in fact *all* existing properties of *all* existing plugins, as I assume there are *many* that have impact on the build. So unless he wants to do *that*, I feel treated in an unfair way that *all that* stuff will be kept for possibly ever but just *our* PR is rejected.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826292057
@rfscholte Can you please open an issue that tracks the progress of your crusade, hence listing all the properties that must go away? So the public can follow your success? Thanks.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826096259
> For people that want to be able to control it from commandline could add `<skip>${shade.skip}</skip>`, as can be done with any other parameter.
Actually this simply makes my use case (temporarily but *unplanned*, hence *without* upfront POM-change) much harder, and I do not understand what risk you *actually* do see from this property. Can you please elaborate what bad thing shall happen?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rfscholte commented on a change in pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#discussion_r633064207
##########
File path: src/it/projects/MSHADE-382_skip_execution/verify.bsh
##########
@@ -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.
+ */
+import java.io.*;
+
+File originalJarFile = new File( basedir, "target/original-skip-execution-1.0.jar" );
Review comment:
Groovy is the standard nowadays, which would probably make this a oneliner, something like:
`assert !(new File( basedir, "target/original-skip-execution-1.0.jar" ).exists())`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826113453
> No commandline argument should have impact on the outcome of the artifact.
That is just a personal opinion; others think differently and nobody besides you had a problem so far with it (they all approved without requesting a change). The compiler plugin supports e. g. `-Dmaven.main.skip`, and it clearly *has* an impact on the outcome of the artifact and nobody requested a change to remove it.
> @mkarg it solves your use case more cleanly since you intentionally enable to skip it because your project can so guess it is not a bad compromise: enable without breaking other cases by default, more "maven spirit" ;).
In fact it is *not* intentionally and I don't want to "plan upfront" the use of it: I need to skip *just once* to see if the problem comes from the shading plugin or not. I don't want to have to prepare the POM of all my projects "just in case" I need it *just once* some day (which is the most common case for me to skip shading).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-821526317
A 'no' for me on this request, see discussion on jira.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-869050462
My thoughts exactly. A `skip` property without namespace would be very harmful.
--
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] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-882482799
--
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] rfscholte commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841788833
The parameter should have the general `skip` name.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-821530011
Following up at https://issues.apache.org/jira/browse/MSHADE-382
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841662916
I second what @aalmiray said, and BTW I did not blame anybody for the stuck discussion. Stuck simply means, nobody responded. This includes Andres.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] aalmiray commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
aalmiray commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-827953865
Alright. If I understood correctly, removing the `@Parameter` annotation is the compromise that will let this PR be merged.
However, what we accomplish with that is:
- making it harder for not Maven savvy developers to "break" a build by "inadvertently" (by ignorance) or "incorrectly" (by mistake) if they happen to set a property exposed by the plugin (`shade.skip` in this case). In this regard the compromise would suffice.
- making it harder for Maven savvy people to accomplish the original use case, as now they have to explicitly add a configuration block with a property of their choosing (such as `shade.skip`, `my.shade.skip`, `why.is.this.not.the.default.shade.skip`, etc), that is, if they want to switch between shade and none-shade with a command property or a pom property. This implies adding a configuration block. As the original use case states that's exactly the thing I'd like to avoid, as the shade plugin is defined by a parent POM outside of my control. If I have to redefine the plugin section to add one additional explicit property I might as well redefine everything and put it in a profile or whatever.
Thus removing the parameter helps newbies but irks experts.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg commented on a change in pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on a change in pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#discussion_r659273932
##########
File path: src/it/projects/MSHADE-382_skip_execution/verify.bsh
##########
@@ -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.
+ */
+import java.io.*;
+
+File originalJarFile = new File( basedir, "target/original-skip-execution-1.0.jar" );
Review comment:
We replaced this BSF script by Groovy meanwhile.
--
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] mkarg commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826286867
@aalmiray For the sake of getting progress, I refuse to insist on the command line property. Andres, will you remove 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] mkarg edited a comment on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
mkarg edited a comment on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826286867
@aalmiray For the sake of getting progress, I give up to insist on the command line property. Andres, will you remove 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-841392211
@mkarg https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-828131563 or next message explaining/rephrasing it proposed an unifying option
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] rmannibucau commented on pull request #90: [MSHADE-382] - Add property to skip execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #90:
URL: https://github.com/apache/maven-shade-plugin/pull/90#issuecomment-826282136
@mkarg it is quite simple, you have 2 setup use cases with shade: 1. main artifact (no real point to skip it), 2. classified (as "with a classifier") artifact (fatjar, relocation, ...) and here it makes sense to skip it since it can be slow (redoing a zip is, this is why i mentionned assemblies as equivalent use case). 2 is a very common case we want to skip in "dev" and we must enabled. 1 is only about skipping the module - all other reasons are solvable through another way than modifying the plugin and they also need more than just a skip flag like a test flag consistent with the shade flag so this PR does not solve accurately this case anyway.
What you describe does not look like a shade skip flag but the support of -<maven mojo execution> since you can already bypass shade execution bypassing the lifecycle (mvn compiler:compile jar:jar for example) but it misses the opposite (use lifecycle minus E1, E2 executions) but this is orthogonal to this PR which only solves the common convenient binaries deliveries bypassing in dev IMHO.
Will change my +1 to reflect that - just thought it would be trivial and fast to just drop the property but seems not ;).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org