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/01/28 17:28:57 UTC
[GitHub] [maven-shade-plugin] JanMosigItemis opened a new pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
JanMosigItemis opened a new pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83
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)
- [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)
----------------------------------------------------------------
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-873338678
@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.
--
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] kriegaex commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-878143781
--
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] kriegaex commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-873338678
@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 fix and factor out the actual test refactoring into either a new PR or at least into a separate commit, so we can clearly differentiate the fix from the refactoring. It helps nobody if the PR is just sitting here, rotting.
--
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-873338678
@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 extens, because I believe that tens of thousands of developers - I want to avoid the hyperbole of saying "millions" - probably use Maven Shade.
--
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] kriegaex commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-873350704
> 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 (and also according to Java standards). I also reordered the imports so as to minimise changes compared to the main branch.
--
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] JanMosigItemis commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
JanMosigItemis commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-878315256
@rmannibucau in my case, as documented here: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-769668875, 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?
--
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847683826
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 here, 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.
--
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-873338678
@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.
--
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847683826
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.
--
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-873350704
> 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.
--
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 #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-878118199
--
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] kriegaex commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-878045403
If somebody finally decides to merge #104, yes. It is still pending, I am not sure why.
--
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] JanMosigItemis commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
JanMosigItemis commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-873919166
Thx again for the input. I guess, this means, PR #83 may be closed without merge?
--
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] kriegaex commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847452890
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, 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.
--
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847683826
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 big fan of utility classes with lots of static methods.
In this case here, 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.
--
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] JanMosigItemis commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
JanMosigItemis commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-776571029
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.
----------------------------------------------------------------
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847452890
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.
--
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 #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-769255809
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.
----------------------------------------------------------------
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] TheDGOfficial commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
TheDGOfficial commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-776253880
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](https://stackoverflow.com/questions/63893042/warning-myproject-target-classes-is-a-directory-when-minimizejartrue-mi) 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).
----------------------------------------------------------------
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 #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-878089031
@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.
--
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] kriegaex commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847683826
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. But I was just 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.
--
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] JanMosigItemis commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
JanMosigItemis commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-769668875
@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.
----------------------------------------------------------------
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] JanMosigItemis commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
JanMosigItemis commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847642859
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.
--
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] JanMosigItemis commented on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
JanMosigItemis commented on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-873919166
Thx again for the input. I guess, this means, PR #83 may be closed without merge?
--
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-873338678
@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.
--
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] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-878143781
--
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