[MSHADE-366] - "Access denied" during 'minimizeJar'

[JanMosigItemis]

Now ignoring directories when scanning the classpath for services. Just scan through files.

Not sure though, if directories should be traversed somehow. However, if so, than this might be a different bug, bc directories are in fact not being traversed in the current release version (3.2.4)

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

[rmannibucau]

Hi @JanMosigItemis , did you check why you hit a directory? too early custom execution?
Note that it needs a test that the directory classpath element service is still listed in services in the minified jar even if not in the classpath (ie only in META-INF/services/foo) - guess it is the functional expectation.

side note: seems module-info services are skipped for now but should end up in the expected classes so can need another ticket about it - mentionning it since I just saw it while reviewing your PR.

[JanMosigItemis]

@rmannibucau yes I checked. It is bc org.apache.maven.plugins.shade.filter.MinijarFilter.removeServices(MavenProject, Clazzpath) calls project.getRuntimeClasspathElements which puts the output directory on the resulting list. See https://github.com/apache/maven/blob/bb916d0784c7631866167928e4d0615df3317567/maven-core/src/main/java/org/apache/maven/project/MavenProject.java#L412

I could provide the test you wrote about, but I am not sure it leaves the scope of this PR, bc such a test was already missing before I started working on the issue. Please let me know if such a test is required here.

[TheDGOfficial]

I'm also getting this issue. The warning is different in Ubuntu and Windows.

Both are with Maven Shade Plugin 3.2.4.

on Windows (Windows 10, my computer):
[WARNING] C:\Users\<my username>\<path to project-root>\target\classes (Erişim engellendi)

"Erişim engellendi" means "Access denied" in my language (Turkish).

on Ubuntu (20.04, GitHub Actions):
Warning: /home/runner/work<path to project root>/target/classes (Is a directory)

I first found this unanswered question on StackOverflow from Google, then decided to check GitHub PRs and JIRA issues if an issue/PR already exists, and found this.

Hope it will be fixed/merged soon (It is the only warning on my build and I'm a bit nitpicky, I know it doesn't cause any issues).

[JanMosigItemis]

The different warning messages are caused by different FileSystem implementations in use. Java's IO code eventually maps to OS native implementations of course. So I guess it is still the same bug on any OS.

[kriegaex]

As MSHADE-366 was originally created and is still bugging me and because lately @rmannibucau collaborated with me in another PR, I want to restart this one here in order to finally get it off the table. When I checked out @JanMosigItemis's fork and rebased it on master, I noticed that the diff looks bigger than it is due to whitespace changes caused by the if wrapped around a big chunk of code. Actually, in we ignore the whitespace changes, it is only this:

As suggested similarly in the Jira issue, I would rather do it like this:

If would both avoid whitespace changes and make the code more readable by avoiding nesting. It would also avoid two new imports.

I did not look at the test in detail yet, it seems that there was refactored more than necessary in order to test this case, but firstly I could be wrong (going to take a second look) and secondly fixing a bug does not mean we have to avoid refactoring in the surrounding code. Maybe it should just be a separate commit.

The more important question I have is: Is it necessary to do the same analysis and exclusion for used services during minification for files in a target directory on the classpath, which currently happens for JAR files, or is it the right thing to just ignore it? Is a classpath directory a valid use case here? It could very well be the case, because the current module's class files are usually part of the uber JAR if not explicitly excluded by a filter. So we would have to mimic the same logic here. I can try doing that, starting a fresh PR superseding this one. But I want a maintainer's qualified answer before I start potentially wasting time with a superfluous feature.

[JanMosigItemis]

Hi @kriegaex and thanks for your input. I totally understand your remark on code readability but I've got a totally different opinion which is why I provided the PR as I did. This does also hold for usage of new File.. vs. Files...

Also I once was taught to avoid continue (or break for that matter) in loops bc it makes the execution path harder to be traced.

Anyway, I'd oblige to any code style the maintainers see fit and would also happily alter (and rebase) the PR if need be. A new PR would also be ok with me, I really do only care about this nasty warning going away.

[kriegaex]

Yes, continue might not look so nice to some people, but is is clearly readable. Code refactoring is always a trade-off. I was taught to avoid nesting control structures as much as reasonably possible. Actually, this could all be factored out into smaller methods, each of which would be more readable. I also do not understand why you prefer to use two tool classes with static methods instead of my suggestion. Static methods are handy sometimes, but usually difficult to test, especially to mock-test. Here, that might not be a big issue, but basically I am not a fan of utility classes with lots of static methods.

In this case, my reasoning was much simpler, though: I was simply aiming for as much a minimal invasive change as reasonable here, in order to avoid lengthy code reviews and enable this thing to be merged ASAP. It is easy enough to fix and has been remained unresolved for too long already.

So even if we factor out the possible extension I talked about above - handling classpath directories the same way as JARs with regard to analysing and eliminating/keeping services - into a new issue, this little fix provides customer value already because the warning message is just... suboptimal.

[kriegaex]

@JanMosigItemis, maybe you want to reconsider your opinion and simplify the PR the way I suggested. I guess, the smaller the change, the less nested the control structures, the fewer new imports you pull in without any obvious benefit, the higher the chances that someone merges this quickly. You may also want to minimise your test change and factor out the actual test refactoring (if necessary at all) into either a new PR or at least into a separate commit, so we can clearly differentiate the new test case from the refactoring. It helps nobody if the PR is just sitting here, rotting.

Having said that, I do not really understand why nobody with the right to actually merge this thing has collaborated with you in order to get this off the table for so long. I would, if I could, but I am a user and contributor, just like you. I have no privileges here. I suggested this fix in January already, so I guess the maintainers must be super busy and this plugin does not have a high priority for anyone. This surprises me to some extent, because I believe that tens of thousands of developers - I want to avoid the hyperbole of saying "millions" - probably use Maven Shade.

Update: I wanted to mention one more thing:

I once was taught to avoid continue (or break for that matter) in loops bc it makes the execution path harder to be traced.

I disagree. It helps avoid nesting and also helps getting exceptional conditions out of the way before applying the "happy path" logic. When I see break or continue, I immediately know that I do not have to read on when trying to understand or trace in my mind the execution path of a method. I can either scroll to the end of the control structure (break) or back to the beginning of it (continue) and simulate the next iteration in my mind. Furthermore, in this case the style of using continue is already used 4 more times in the very same method, so changing it for this one case is like applying two paradigms to the same piece of code.

[kriegaex]

Code refactoring is always a trade-off. I was taught to avoid nesting control structures as much as reasonably possible. Actually, this could all be factored out into smaller methods, each of which would be more readable.

I just did that on top of your PR, factoring out the big method into 3 smaller ones. I kept the pattern to use continue, though - sorry. I also refactored your tests into one, because they were 90% duplicates, also renaming the test method from this_naming_pattern to thatNamingPattern like in the rest of the test class (and also according to Java standards). I also reordered the imports so as to minimise changes compared to the main branch.

[JanMosigItemis]

Thx again for the input. I guess, this means, PR #83 may be closed without merge?

[kriegaex]

If somebody finally decides to merge #104, yes. It is still pending, I am not sure why.

[rmannibucau]

@kriegaex I guess the expectation is to fix the cause instead of ignoring it so can be a thing slowing down merge button push maybe - at least for me.

[kriegaex]

Why are you reacting here instead of in the new PR? I was waiting for feedback there, you did not react to my latest comment from 9 days ago.

To answer your question: No, the expectation was not add a new feature. Just read the Jira issue again, please. Instead, it was not to see the meaningless warning anymore. I implemented that and on top of it factored out some methods in order to make the code more readable. I also suggested to extend the functionality in a separate issue and PR, because it is a new feature. Now the feature to handle directories like JARs with regard to handling services simply does not exist, so there should not be any irritating warning. Instead, there is a debug message now, explaining what is going on. You approved of that.

So why are you blocking a clear improvement which can be the basis for the next PR, instead of not merging, waiting for a related feature? I understand why you and I both would want that feature. I was the first one to suggest it anyway! But that idea should not block incremental improvement. I think smaller PRs are a nice thing.

[rmannibucau]

@kriegaex I did 9 days ago: #104 (review)
I'm clearly not blocking, just pointing out that as written this PR does not solve the mentionned issue so, as written, I'm fine if somebody wants to merge it since it does not hurt but I don't see it as needing to be merged since there are solution to this particular issue (fixing the classpath etc), it does not solve the issue which ignored folders anyways - without this PR, and it does not solve the issue mentionned in the ticket. I guess it answers your question why it is not yet merged.

[kriegaex]

@kriegaex I did 9 days ago: #104 (review)

No, I said my latest comment in which I reacted to yours, explaining why I did what.

I'm clearly not blocking, just pointing out that as written this PR does not solve the mentionned issue so, as written

https://issues.apache.org/jira/browse/MSHADE-366 talks about the irritating warning, not about adding a new feature. The title is "Access denied" during 'minimizeJar'", not Handle exclusion for used services during minification for files in a target directory. I.e., adding a new feature is out of scope and was not requested there. I agree with Jan that this should go into a new Jira issue. I did, however, fix the root cause of the problem, not mistaking directories for JARs anymore. This is a real fix for the problem described in the Jira issue. Nothing is hidden. Corresponding information is still logged in debug mode, i.e. whoever of the maintainers looks at the corresponding piece of code must notice that there is a TODO for the future, even if there is no separate Jira issue yet. Before we finally agree to merge my PR without the new issue, it does not make sense to create it, though, because if you and other maintainers disagree, maybe you want to contribute the new feature you seem so intent to have in my PR:

[rmannibucau]

@kriegaex ok so let say I disagree the warning is heritant and think it reveals a real bug we can fix. Also logging in debug is hiding by nature since it is rarely ran in debug, in particular on CI - but this is another topic probably. So long story short, I think we should not have a quick and dirty fix for this issue but fix the actual cause which is the folder handling. It needs to 1. know why there is a folder in the cp at that point (maybe it is a wrong setup) and 2. depending on 1 either support folders or reject the issue as being irrelevant (in which case this PR is fine). But without 1 we can't decide if the solution is to handle folders or fix the logging.

[kriegaex]

My fix is not dirty! It cleans up a dirty situation. It just does not add a new feature that you suddenly want to define into MSHADE-366. But I am used to this project being slow, so for a long time I have used my own, self-built plugin version in my projects, one of which prominently is AspectJ. I simply deployed it under another group ID, because we cannot afford being blocked by Maven Shade. My version already contains fixes for:

• MSHADE-252 - my PR [MSHADE-252] - Make source shading respect package/class name excludes #88 was merged after a long time, but is still unreleased, and I already added an improvement in [MSHADE-396] Improve SourceContent Shading #105, reacting on MSHADE-396.
• MSHADE-391 - my PR [MSHADE-391] Do not write modified class files for no-op relocations #95 is still unmerged. You know it, you reviewed it yourself.

So I just build a new version with this fix also merged in, scratching my own itch and enjoying the improvements, until finally the people who are allegedly not blocking PRs decide to either contribute and improve them by themselves or merge them and create new ones for further improvements. Maven Shade is simply a slow-moving project. I wish that other users could also profit from my PRs, and maybe at some point they will. For now, I am simply using them myself, unblocking myself.

[rmannibucau]

My fix is not dirty! It cleans up a dirty situation.

As explained, I can agree with that if you find a proof the fact the folder is in the CP is expected, in the other case (leak from an assembly plugin or alike) it is a quick and dirty fix as explained.
I didn't see such explanation so until we get this input we don't know what is needed to fix the issue.
Note that it can just hide that the target/classes SPI are broken which is why I'm saying it can NOT be a fix but a quick and dirty way to hide a bug.
This is why I'm asking for more inputs before being able to say it is a good/valid fix or just a quick and dirty workaround.
Once again, no goal to block anything but ensure this fix solves an issue and does not hide it.

[kriegaex]

It is not rocket science to understand that it is perfectly normal for directories to be on the classpath. The world is not all JARs. The problem is not that a directory is on the classpath, but that MShade assumes, every classpath member must be a JAR. Having said that, why would I have to explain that to you? I am a simple user, trying to contribute something. If you have the power to merge something, you are in the role of a maintainer. Maybe you are not the lead developer for this particular plugin, but at least you have contributes 5 commits to it. Either way, why don't you verify this, as part of being a reviewer? Talk is cheap, so I can say whatever I want. Just read the code and see for yourself. If I made a mistake, which is perfectly possible, suggest a fix. And if you don't - whatever. I am used to OSS maintainers blocking PRs, simultaneously saying they are not blocking anything. It would actually be quite funny, if it was not so sad. Do whatever you want with my PR - merge it, close it, improve it. I do not care anymore. I wasted enough time on this.

[rmannibucau]

It is not rocket science to understand that it is perfectly normal for directories to be on the classpath.

In a classpath yes, in a shade build it is abnormal as of today (you can check the code you modified to confirm it).
If you are saying it is normal and we must support folders then i'm -1 for this PR and expect a folder handling as a fix - but once again, until we have the information of the original project why a folder is in the classpath then we can't judge.
Please don't be picky and try to just be pragmatic, there is an issue, we identify the cause (still not done) then we decide on a fix. It is the process i'm expecting there or we'll end up in endless debates cause we'll try to guess the cause.

[JanMosigItemis]

@rmannibucau in my case, as documented here: #83 (comment), the directory on the cp is the target directory itself, which is returned to shade by maven's own project implementation. I guess in this special case, ignoring that particular entry is ok. Would you agree?

[rmannibucau]

@JanMosigItemis hmm, not sure I fully got it but if target/ it is unexpected and it should be stripped early (instead of lately ignored), if target/classes it must not be ignored but it must be handled until target/artifact-version.jar is there in which case the folder shouldnt have been considered - if it is, it can be due to a too early execution/phase binding. I maybe miss some cases where it can happen but the ones I'm thinking about tend to go with an fix located way earlier than current PR.
Hope it makes sense.