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

For Windows OS, revert the usage of cmd.exe by default (new option) #110

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

Conversation

axel3rd
Copy link
Contributor

@axel3rd axel3rd commented Dec 21, 2020

This PR is an update of old previous staling PR #42


cf. #17 (comments from May 2018): Revert the usage of cmd.exe by default (on Windows), because prevents the destroy/kill launched by this way when CTRL+C.

Allow this behavior by the usage of a new method:

Commandline cmd = new Commandline();
cmd.setForceShellOsSpecific( true );

This PR should be chosen or #109.

@axel3rd axel3rd marked this pull request as draft December 21, 2020 11:29
@axel3rd axel3rd force-pushed the update-42-For_Windows_revert_usage_of_cmd_by_default branch from c7a10b4 to 84109d4 Compare December 21, 2020 11:34
@axel3rd axel3rd force-pushed the update-42-For_Windows_revert_usage_of_cmd_by_default branch 3 times, most recently from 8f20b85 to 971980d Compare December 21, 2020 13:24
@axel3rd axel3rd force-pushed the update-42-For_Windows_revert_usage_of_cmd_by_default branch from 971980d to ff8e0e4 Compare December 21, 2020 13:28
@axel3rd
Copy link
Contributor Author

axel3rd commented Dec 21, 2020

Note: GitHub action verify problem with Java 16-ea is not part of this PR.

@axel3rd axel3rd marked this pull request as ready for review December 21, 2020 13:35
@axel3rd axel3rd requested a review from michael-o January 4, 2021 11:50
@axel3rd
Copy link
Contributor Author

axel3rd commented Jan 4, 2021

(@michael-o: Requested changes have been done)

@axel3rd
Copy link
Contributor Author

axel3rd commented Jan 4, 2021

Note: GitHub action verify problem with Java 16-ea is not part of this PR.

See #116

@axel3rd
Copy link
Contributor Author

axel3rd commented Feb 26, 2021

Is this PR (or #109) not relevant 😢 ?

@michael-o
Copy link
Member

I will try to pick this up next month.

@axel3rd
Copy link
Contributor Author

axel3rd commented Feb 26, 2021

Thank you !

@axel3rd
Copy link
Contributor Author

axel3rd commented Jul 28, 2021

I will try to pick this up next month.

😁 (#FriendlyBump)

@michael-o
Copy link
Member

Thanks for the note. I think both Bourne Shell and Cmd Shell must go. There is no use for that.

@axel3rd
Copy link
Contributor Author

axel3rd commented Jul 29, 2021

I think both Bourne Shell and Cmd Shell must go. There is no use for that

=> We abandon this PR, and perhaps #109 depending the relevance ?

@michael-o
Copy link
Member

I first need to understand why this was done at all. Maybe lack of knowledge. Currently, I don't see a reason why we should wrap these command into a shell execution at all.

@rfscholte
Copy link
Member

From what I've always understood only with a new shell you can isolate system properties and environment variables.

@michael-o
Copy link
Member

System properties are pure args to a process. There is no connection to the parent process. If want to isolate env vars, you can do that easily with the ProcessBuilder. It works just fine. Moreover, you cannot start with a nude environment because some processes expect default env vars and they won't be present. Just set those you need and that's it.

@michael-o michael-o removed their request for review October 23, 2022 08:59
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