Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MSHADE-307] adding useDefaultConfiguration flag in ShadeMojo #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rmannibucau
Copy link
Contributor

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.

Copy link

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very useful!

I am sure about the name. A 'default' configuration which is not active by default sounds weird.

What about about 'safe', 'common', 'standard'....

@rmannibucau
Copy link
Contributor Author

Common sounds a good alternative but too generic, I thought about "auto" but was not that happy too. Maybe "autoConfigure" + adding a spi on transformers and filters would make more sense?

@eolivelli
Copy link

autoConfigure + SPI will be great, but it a more complex change.

The other way would be to have such "useDefaultConfiguration", turned on by default, but with a bump in the major version for the plugin.

@rfscholte what do you think ?

@rfscholte
Copy link
Contributor

In general I'd like prefer to keep the number of parameters to a minimum. So if we want to do this, why not set these as defaults. When there are explicit filters/transformers, these will be used. Any empty list would mean: no filters or transformers. And this implies there should be an explicit filter for signatures.

@eolivelli
Copy link

Changing defaults needs a bump in major version ?

These new defaults are safe but not obvious.

I am +1 in changing defaults

@rfscholte
Copy link
Contributor

The new version will be 3.n+1.0. The first 3 means that is requires at least Maven 3. We discovered that this pattern makes is very clear when certain plugins can be used.
We should add this change to the index.html file too, just to be clear. (in the end the result should be the same, because if filters and transformers are specified, nothing will change)

@rmannibucau
Copy link
Contributor Author

Hmm, i thought of that - active by default and ignored if set by the user - but it is rare it is sufficient and does need a log4j, openwebbeans or other transformer so i preferred the composition. Should we have it with 3 values: default = use, skip and merge?

@eolivelli
Copy link

Okay for versioning and index.html
I can take of take (with a little guidance).

For use/skip/merge....things are getting complicated and complicated.

We should default to 'merge', but will 'use' be useful? As 'merge' is a super set of 'use'

@rfscholte
Copy link
Contributor

For use/skip/merge....things are getting complicated and complicated.

I totally agree here. Like all Maven plugin parameters it is: use the default or override it. Those are the only 2 modes, clear and simple.

@rmannibucau
Copy link
Contributor Author

Hmm it must be combinable or it is useless and config stays complex.

Merge (default), override, skip?

@rfscholte
Copy link
Contributor

Sorry, but it will be default or override: easy to understand and maximum control. If you want full merge/override-control, you need to do that with the magic attributes as described at https://blog.sonatype.com/2011/01/maven-how-to-merging-plugin-configuration-in-complex-projects/

@rmannibucau
Copy link
Contributor Author

@rfscholte this is my intention but here it does not work cause the config is implicit, this is why i proposed 3 states. Now if you say the impl is ok to turn on by default it can be enough for the next release (and we enhance later if needed). If so, do you have a name preference?

@rfscholte
Copy link
Contributor

If so, do you have a name preference?

for what?

@rmannibucau
Copy link
Contributor Author

@rfscholte "useDefaultConfiguration" was not liked by @eolivelli and I have to admit I'm not fan of "common" usage so asking for options here

I will try to PoC the SPI idea now to let us "see it" before going with your other options which seems to kill the idea to simplify the config of the plugin.

@rfscholte
Copy link
Contributor

there's no need for this parameter. Just remove it

@rmannibucau
Copy link
Contributor Author

Guys, I added the SPI and removed the toggle, can you have a look and let me know if it looks better this way? I like this solution cause we can plug the SPI in log4j, openwebbeans, and other extensions

if ( transformers == null )
{
return Collections.emptyList();
return resourceTransformers;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're still merging. Here you should replace Collections.emptyList() with getDefaultResourceTransformers()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the case, resourceTransformers is initialized with getDefaultResourceTransformers. If the else skips the default transformers the feature is pointless since in 80% of the case you need to add a custom transformer or customize a config (like mainClass for the manifest)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's agree to disagree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine to disagree but I'd like to solve the issue which aims to reduce the entry cost of the plugin and save hours of debug time due to a bad packaging which is hard to identify - and even more with jpms since module-info.java drops SPI registration :s.

any alternative proposal to have a trivial to enable good auto configuration compatible with user customizations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't think of a clean, clear and simple solution yet. So let's split it into 2 issues: 1. have good default transformers; 2. combine default+custom transfomers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's split. Just to ensure I get it and not do work for nothing, it means 3 PR - no order:

  1. defaults (transformers and filters)
  2. SPI
  3. combine
    ?

Copy link
Contributor Author

@rmannibucau rmannibucau Dec 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 done at #12
2 done at #13

will let 3 to a bit later to see if we find a good idea


return Arrays.asList( transformers );
private List<ResourceTransformer> getDefaultResourceTransformers()
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're not merging, so you can return a list with these 2 transformers.
Also document this on the transfomers-parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guess it is addressed by previous comment, we want to merge ;)

@@ -823,6 +844,50 @@ private File resolveArtifactSources( Artifact artifact )
}
}

// first check for backward compatibility this is not already configured explicitly
boolean addExclusion = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a similar trick here:

public Collection<Filter> getFilters()
{
  if ( this.filters == null )
  {
    return defaultFilters();
  }
  else
  {
    return this.filters;
  }
} 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likely the same

new HashSet<String>( Arrays.asList( "META-INF/*.SF", "META-INF/*.DSA", "META-INF/*.RSA" ) ) ) );
}

for ( final Filter filter : ServiceLoader.load( Filter.class ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ServiceLoader requires a separate JIRA issue, and I wonder if this is going to work. See https://issues.apache.org/jira/browse/MNG-6275

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It must work if the extension is added in plugin dependencies as commonly done for transformers - http://openwebbeans.apache.org/meecrowave/meecrowave-maven/index.html#_shading for example. About the other issue: it can be the case but the issue is to make the configuration easier, if we don't want of a toggle then the SPI is the answer so it is the same issue, no?

@rmannibucau rmannibucau force-pushed the adding-useDefaultConfiguration-toggle branch from ad66d55 to 6300425 Compare August 8, 2019 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants