-
Notifications
You must be signed in to change notification settings - Fork 3k
Conversion to SmallRye Common Process #48354
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
Conversation
/cc @Karm (awt), @brunobat (micrometer), @ebullient (micrometer), @galderz (awt) |
84bf53d
to
b6442d5
Compare
🎊 PR Preview e8c4b6e has been successfully built and deployed to https://quarkus-pr-main-48354-preview.surge.sh/version/main/guides/
|
core/deployment/src/main/java/io/quarkus/deployment/dev/DevModeMain.java
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/util/ExecUtil.java
Outdated
Show resolved
Hide resolved
independent-projects/tools/utilities/src/main/java/io/quarkus/utilities/OS.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice improvement!
e608f01
to
9cc465f
Compare
This comment has been minimized.
This comment has been minimized.
…ner` to new process API
…nerRunner` to new process API
…and` to new process API
…avoid losing output due to goofy race condition
Unfortunately, I have to push it again to resolve conflicts. But it should be OK to merge. |
👍🏽 Let's merge when CI goes green |
Status for workflow
|
Status for workflow
|
Any case where the process is normally writing to In this particular case, we had previously set the error stream to be redirected to output. I don't think that's the right thing here, so I'd say that this code should be changed to just log the I'll put up a PR for that. |
Created #49043. |
Convert most usages of the JDK
ProcessBuilder
andexec
over tosmallrye-common-process
.I've run across at least one class that can't be trivially converted because it is doing something complex/crazy/etc.; these cases should be refactored and cleaned up separately. I'll list these cases here as they appear, for future reference:
io.quarkus.test.common.DefaultDockerContainerLauncher
io.quarkus.test.common.ArtifactLauncher
paradigmPlease review not just the code changes themselves, but also give feedback on the ergonomics of the process API in these various situations.
The move is split up into one commit per class so that we can bisect any problems that arise.
See #48223 for discussion and smallrye/smallrye-common#418 for the upstream PR.