Skip to content
New issue

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

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

Already on GitHub? # to your account

[MJAR-310] - fixed toolchain version detection when toolchain paths contain white spaces #86

Conversation

jansohn
Copy link
Contributor

@jansohn jansohn commented May 17, 2024

Switched to ProcessBuilder for JDK toolchain version detection as it actually correctly handles white spaces in the executable path compared to the plexus-utils dependency.

@jansohn jansohn force-pushed the MJAR-310-Plugin-fails-to-handle-toolchain-paths-that-contain-spaces branch from c5ff576 to f07ad8f Compare May 17, 2024 13:36
@slawekjaranowski slawekjaranowski self-assigned this May 17, 2024
@slawekjaranowski
Copy link
Member

@jansohn - please look at comments in JIRA https://issues.apache.org/jira/browse/MJAR-310
It can be reasonable to fix a bug in plexus-utils

@slawekjaranowski
Copy link
Member

@jansohn can you try my proposition of fix: #87

@jansohn
Copy link
Contributor Author

jansohn commented May 18, 2024

@jansohn - please look at comments in JIRA https://issues.apache.org/jira/browse/MJAR-310
It can be reasonable to fix a bug in plexus-utils

Makes sense but there are already three year old PRs trying to revert the extra use of cmd.exe so I'm not keen on going down that route.

I think it makes more sense to just use ProcessBuilder directly for such a simple task.

@jansohn
Copy link
Contributor Author

jansohn commented May 18, 2024

@jansohn can you try my proposition of fix: #87

I can test it on Tuesday but I doubt it will fix the problem.

@jansohn
Copy link
Contributor Author

jansohn commented May 21, 2024

@jansohn can you try my proposition of fix: #87

I can test it on Tuesday but I doubt it will fix the problem.

I stand corrected and #87 actually does solve the issue with blanks in Windows paths.

Totally unintuitive that it works by setter and not by constructor argument in my opinion. While digging into the code I also saw that it actually dismisses the shell wrapping if it detects a non-Windows OS which explains why this only occurred on Windows systems.

Personally I would not rely on such an overly complex third library for such a simple use case but feel free to merge whatever PR you prefer.

@slawekjaranowski
Copy link
Member

Commandline used by constructor try to parse command .... so when we have a space it is wrong parsed ...

@michael-o - next issue with parsing command line 😄

@michael-o
Copy link
Member

Commandline used by constructor try to parse command .... so when we have a space it is wrong parsed ...

@michael-o - next issue with parsing command line 😄

It does not surprise me. I have been telling that this is broken for years.

@slawekjaranowski slawekjaranowski merged commit e9c98a4 into apache:master Jun 1, 2024
19 checks passed
@slawekjaranowski
Copy link
Member

@jansohn thanks

@jansohn jansohn deleted the MJAR-310-Plugin-fails-to-handle-toolchain-paths-that-contain-spaces branch June 1, 2024 18:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants