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

Revert usage of cmd on Windows #41

Closed
wants to merge 1 commit into from
Closed

Revert usage of cmd on Windows #41

wants to merge 1 commit into from

Conversation

axel3rd
Copy link
Contributor

@axel3rd axel3rd commented May 22, 2018

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

For history: This Windows shell specific is removed since plexus-utils 3.0.15, except for 3.1.0.

On Windows, the usage of cmd builtin (like echo) or .cmd / .bat on PATH should be implemented by the client.

@axel3rd
Copy link
Contributor Author

axel3rd commented May 22, 2018

Travis oraclejdk7 support seems removed (see here).
Could be done in // PR if not relevant here.

@michael-o
Copy link
Member

Please move the OpenJDK 7 commit to a separate PR.

@axel3rd
Copy link
Contributor Author

axel3rd commented May 23, 2018

About openjdk7: already in issue-fix branch (commit: 54f6ca0d9c7d0940f84610788f3e55c2548dc5c4).

@axel3rd
Copy link
Contributor Author

axel3rd commented May 23, 2018

Other PR #42, to propose an approach for cmd usage if wanted (consensus ? hardcode a cmd.exe /X /C for a simple echo disturb me 😜).

@mkarg
Copy link
Collaborator

mkarg commented Dec 21, 2020

@axel3rd Please fix conflicts. Thanks.

@axel3rd
Copy link
Contributor Author

axel3rd commented Dec 21, 2020

@axel3rd Please fix the conflicts. Thanks.

@mkarg : Sorry, not able todo that in this PR, because is >2 years old so I have deleted the fork since this time.

@michael-o
Copy link
Member

@axel3rd Please fix the conflicts. Thanks.

@mkarg : Sorry, not able todo that in this PR, because is >2 years old so I have deleted the fork since this time.

That's ok. @mkarg Honestly, I don't see a reason to have a shell wrapper at all, but use ProcessBuilder. This worked for me for years. Those wrappers only cause confusion and issues which I have to fix in Maven SCM.

@mkarg
Copy link
Collaborator

mkarg commented Dec 21, 2020

@axel3rd Please fix the conflicts. Thanks.
@mkarg : Sorry, not able todo that in this PR, because is >2 years old so I have deleted the fork since this time.
That's ok. @mkarg Honestly, I don't see a reason to have a shell wrapper at all, but use ProcessBuilder. This worked for me for years. Those wrappers only cause confusion and issues which I have to fix in Maven SCM.

My target is to cleaup the open PRs, so either we merge or drop them or turn them into drafts, but not keep them open for
review for more years in an unclear state. To reach that goal, first the original authors need to resolve conflicts to allow reviews ontop master. I am looking into the actual change once the code at least will compile again.

@axel3rd
Copy link
Contributor Author

axel3rd commented Dec 21, 2020

My target is to cleaup the open PRs, [...] not keep them open for review for more years in an unclear state

Fully agree with that 👍

I don't see a reason to have a shell wrapper at all, but use ProcessBuilder

I understand the content of this PR is not really relevant, so closing.

@axel3rd axel3rd closed this Dec 21, 2020
@michael-o
Copy link
Member

My target is to cleaup the open PRs, [...] not keep them open for review for more years in an unclear state

Fully agree with that 👍

I don't see a reason to have a shell wrapper at all, but use ProcessBuilder

I understand the content of this PR is not really relevant, so closing.

I didn't say that. What I am saying is that our concept is wrong from begin with.

@axel3rd
Copy link
Contributor Author

axel3rd commented Dec 21, 2020

I didn't say that. What I am saying is that our concept is wrong from begin with.

Ok, sorry, misunderstanding from my side.

I will provide some new compiling PR for #41 & #42 and not delete the fork until status, allowing review.

@michael-o
Copy link
Member

I didn't say that. What I am saying is that our concept is wrong from begin with.

Ok, sorry, misunderstanding from my side.

I will provide some new compiling PR for #41 & #42 and not delete the fork until status, allowing review.

Thanks! Take your time! No need to hurry.

@axel3rd
Copy link
Contributor Author

axel3rd commented Dec 21, 2020

See #109 for following.

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