[MSHADE-382] - Add property to skip execution

[aalmiray]

Added a skip field to ShadeMojo to skip the execution.

Following this checklist to help us incorporate your
contribution quickly and easily:

• Make sure there is a JIRA issue 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 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
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

• In any other case, please file an Apache Individual Contributor License Agreement.

[rfscholte]

A 'no' for me on this request, see discussion on jira.

[aalmiray]

Following up at https://issues.apache.org/jira/browse/MSHADE-382

[rmannibucau]

+1, it is possible to skip maven jar plugin so not worse to skip shade, in particular when 50% of the usages are side artifacts and not primary ones (classifier set)

[aalmiray]

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.

[mkarg]

+1 - LGTM! - Thanks a lot for hacking this stuff with me in the live stream, it was such fun and pretty amazing that you not only added this contribution in just one hour, but also explained to a lot of people how easy and fun it is to contribute to Maven! #Amazing

[mkarg]

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?

[rmannibucau]

@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.

[aalmiray]

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 😉

[rfscholte]

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.

[rmannibucau]

@rfscholte does not providing a built-in property makes you happy (did I get it right)? If so I'm fine with that.

[rfscholte]

@rmannibucau that is indeed what i am saying

[mkarg]

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?

[rfscholte]

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.

[rmannibucau]

@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" ;).

[mkarg]

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).

[rfscholte]

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.

[rmannibucau]

@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.

[mkarg]

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).

[stephenc]

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

[rmannibucau]

@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).

[mkarg]

@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...

[mkarg]

@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.

[rmannibucau]

@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 - 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 ;).

[rmannibucau]

shade.skip should likely disappear as discussed and hightlighted by Robert

[rfscholte]

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.

[mkarg]

@aalmiray For the sake of getting progress, I give up to insist on the command line property. Andres, will you remove it?

[mkarg]

@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.

[mkarg]

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)?

[rmannibucau]

@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.

[mkarg]

@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.

[rmannibucau]

@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?

[mkarg]

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?

[rmannibucau]

@mkarg #90 (comment) or next message explaining/rephrasing it proposed an unifying option

[mkarg]

@mkarg #90 (comment) 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.

[rmannibucau]

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.

[aalmiray]

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.

[mkarg]

I second what @aalmiray said, and BTW I did not blame anybody for the stuck discussion. Stuck simply means, nobody responded. This includes Andres.

[rmannibucau]

Ok, if ok with Robert let's just drop the property value then

[rfscholte]

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.

[mkarg]

We dropped this meanwhile.

[rfscholte]

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())

[mkarg]

We replaced this BSF script by Groovy meanwhile.

[rfscholte]

The parameter should have the general skip name.

[aalmiray]

The parameter should have the general skip name.

@rfscholte do you mean the field must have @Parameter( property = "skip") ?

[rmannibucau]

No, this property would break builds too easily. Guess it was more against skipClassified and his previous point making skip acceptable as a compromise.

[aalmiray]

My thoughts exactly. A skip property without namespace would be very harmful.

[rfscholte]

It should be like this:

@Parameter 
private boolean skip;

no property, defaultValue is already false so can be omitted.

[eolivelli]

LGTM
very useful
I added a comment, PTAL

[eolivelli]

Can we log something ?

[aalmiray]

Done. Added a logging statement

[Tibor17]

@aalmiray Pls squash these commits in one. Thx

[Tibor17]

If there are no objections, we can close this PR with push.

[aalmiray]

@aalmiray Pls squash these commits in one. Thx

@Tibor17 I've squashed the commits as requested 😄

[Tibor17]

The CI is running...
Let's wait for the outcome.

[Tibor17]

@aalmiray Thx for contributing!

[aalmiray]

Lovely. Thank you @Tibor17 for merging and everyone else for your help and patience. Go Maven!