maven-shade-plugin icon indicating copy to clipboard operation
maven-shade-plugin copied to clipboard

basic dir handling in default shader

Open rmannibucau opened this issue 3 years ago • 3 comments

Follow up of https://github.com/apache/maven-shade-plugin/pull/104#issuecomment-883263433 to share the idea (likely better than discussing).

rmannibucau avatar Jul 20 '21 10:07 rmannibucau

Because the new PR has no description, I want to quickly describe it:

It solves MSHADE-366 in a more fundamental way than both #83 and #104, namely by adding the new feature of handling input directories and input JARs uniformly with regard to service provider minification, instead of simply avoiding to log irritating warnings when directories - most prominently, the own module's target/classes - are found on the classpath.

It addresses the then open question I raised in https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847452890, in my MSHADE-366 comment and again in https://github.com/apache/maven-shade-plugin/pull/104#issuecomment-873361152 as a reaction to @rmannibucau's corresponding question, making the warning a non-issue.

Therefore, this PR supersedes both of its predecessors. I am suggesting to finish it ASAP (if Romain thinks he still needs to change/improve anything), so we can review and merge it, in order to include it in the hopefully soon to be released Shade 3.3.0.

kriegaex avatar Jul 21 '21 01:07 kriegaex

Sorry, I was a bit too fast with my enthusiasm: In its current state, this PR improves DefaultShader, but MSHADE-366 is about minification and the previous two PRs addressed warnings in MinijarFilter. I.e., while Romain's changes point the way towards unified file handling for directories and JARs, it is by no means used during minification yet. The old warning persists.

To do:

  • Unify resource handling in MinijarFilter, similarly to DefaultShader.
  • Refactor method MinijarFilter.removeService in a way similar to how I did it in PR 104, commit https://github.com/apache/maven-shade-plugin/pull/104/commits/faa97dfb202f56bd5ddff7e0e717f695f956aeb1, extracting smaller methods from that big one.

kriegaex avatar Jul 21 '21 01:07 kriegaex

@kriegaex updated a bit the PR but think it can be worth importing/adding your test on top of it to harness the code, do you want to do it and adjust the code if needed?

rmannibucau avatar Jul 21 '21 06:07 rmannibucau