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

5: Remove pipeline support from process #2

Closed
wants to merge 1 commit into from

Conversation

@jeyvison
Copy link
Contributor

jeyvison commented Jun 25, 2019

Hey Folks :) Is that what you meant with removing pipeline support? If it's not, let me know. Also, I think something must be changed in ProcessBuilder, am i right? And this PR has a test problem i'm not able to fix but if any of you give me some directions i can fix it and resend :)

Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Approvers

@openjdk openjdk bot added the rfr label Jun 25, 2019
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 25, 2019

Hi jeyvison, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jeyvison as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@bridgekeeper bridgekeeper bot added the oca label Jun 25, 2019
@openjdk
Copy link

openjdk bot commented Jun 25, 2019

Webrevs

@jeyvison
Copy link
Contributor Author

jeyvison commented Jun 25, 2019

/signed

@bridgekeeper bridgekeeper bot added the oca-verify label Jun 25, 2019
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 25, 2019

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days.

@openjdk openjdk bot removed the rfr label Jun 25, 2019
@openjdk openjdk bot added the rfr label Jun 25, 2019
@rwestberg
Copy link
Member

rwestberg commented Jun 26, 2019

Hey Folks :) Is that what you meant with removing pipeline support? If it's not, let me know.

Hi! Thanks for the PR! Yes, this is indeed what we meant. (There's actually even a bit more complexity that can be removed, I'll add a more detailed review).

Also, I think something must be changed in ProcessBuilder, am i right?

Do you mean java.lang.ProcessBuilder? That should not need any changes, perhaps I'm misunderstanding the question..

And this PR has a test problem i'm not able to fix but if any of you give me some directions i can fix it and resend :)

All tests pass when I run them in your branch, what kind of problem did you see?

Copy link
Member

rwestberg left a comment

Overall this looks good, but it would be nice if the now-unnecessary lists could be removed as well, for example:

private final List<ProcessBuilder> processBuilders;

could just be a

private final ProcessBuilder processBuilder.
start = Instant.now();
}
}

private void startProcess() throws IOException {
cmd = String.join(" ", getLastProcessBuilder().command());

This comment has been minimized.

@rwestberg

rwestberg Jun 26, 2019 Member

The getLastProcessBuilder method could be removed as there is never more than one now.

@openjdk
Copy link

openjdk bot commented Jun 26, 2019

This PR has been reviewed by Robin Westberg (rwestberg - Reviewer) - comment added.

@mlbridge
Copy link

mlbridge bot commented Jun 26, 2019

@jeyvison
Copy link
Contributor Author

jeyvison commented Jun 26, 2019

@rwestberg Done :) Hope it's better now.

Copy link
Member

edvbld left a comment

Hi @jeyvison, thanks for contributing!

You are definitely on the right track here, but I think there are a few more places that can be simpler when the pipeline supports is removed (see comments in the files).

Thanks!
Erik

@@ -43,7 +42,6 @@
private File stdoutFile;
private List<File> stderrFiles;

This comment has been minimized.

@edvbld

edvbld Jun 26, 2019 Member

This can also be made into a single File stderrFile, we no longer need the a list here (that was only used to support pipelines)

remainingTimeout = remainingTimeout.minus(Duration.between(start, Instant.now()));
start = Instant.now();
if (outputOption == Process.OutputOption.CAPTURE) {
var stderrFile = File.createTempFile("stderr", ".txt");

This comment has been minimized.

@edvbld

edvbld Jun 26, 2019 Member

So this would become essentially like the standard output case, stderrFile = File.createTempFile("stderr", ".txt")


processes = new LinkedList<>();
processes.add(getLastProcessBuilder().start());
processes.add(processBuilder.start());

This comment has been minimized.

@edvbld

edvbld Jun 26, 2019 Member

We can also remove the field private List<java.lang.Process> processes now, that can just be turned into private java.lang.Process process (the list was only there to support pipelines).

var command = processBuilders.stream()
.map(pb -> String.join(" ", pb.command()))
.reduce("", (res, cmd) -> res.isEmpty() ? cmd : res + " | " + cmd);
var command = String.join(" ", processBuilder.command());

This comment has been minimized.

@edvbld

edvbld Jun 26, 2019 Member

Can't you just the field cmd here instead that you updated on line 134?

@openjdk
Copy link

openjdk bot commented Jun 27, 2019

This PR has been reviewed by Erik Helin (ehelin - Reviewer) - comment added.

@jeyvison
Copy link
Contributor Author

jeyvison commented Jun 27, 2019

@edvbld Third time is the luck one. Done :)

@jeyvison
Copy link
Contributor Author

jeyvison commented Jun 27, 2019

Should

be just a single command too?

Copy link
Member

rwestberg left a comment

Thank you, looks good to me now!

@openjdk openjdk bot removed the rfr label Jun 27, 2019
@openjdk
Copy link

openjdk bot commented Jun 27, 2019

The PR review by Robin Westberg (rwestberg - Reviewer) has been updated - changes are approved.

@openjdk
Copy link

openjdk bot commented Jun 27, 2019

@jeyvison This change can now be integrated. The commit message will be:

5: Remove pipeline support from process

Reviewed-by: rwestberg, ehelin
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there have been 5 commits pushed to the master branch:

  • 561d715: 19: Allow GitHub bots to use a PAT for authentication as well
  • a185823: 16: Allow regular expression replacement for web urls
  • 72627b9: 14: Only use git.openjdk.java.net links in emails
  • 934cb9c: 15: Allow a configurable set of labels to block ready for review status
  • 9371b5a: 12: Make the notification sender configurable

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@rwestberg, @edvbld) but any other Committer may sponsor as well.

  • To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@openjdk openjdk bot added the ready label Jun 27, 2019
@rwestberg rwestberg changed the title SKARA-5 Remove pipeline support from process 5: Remove pipeline support from process Jun 27, 2019
@rwestberg
Copy link
Member

rwestberg commented Jun 27, 2019

Btw, I changed the title of the PR to conform to our preferred format (this will be automated in the future).

@rwestberg
Copy link
Member

rwestberg commented Jun 27, 2019

Should

be just a single command too?

And no, that one is fine, commands (executable and its arguments) are passed to ProcessBuilder as a list:

var builder = new ProcessBuilder(processBuilderSetup.command.toArray(new String[0]));

@jeyvison
Copy link
Contributor Author

jeyvison commented Jun 27, 2019

/integrate

@openjdk
Copy link

openjdk bot commented Jun 27, 2019

@jeyvison
Your change (at version d76aaa2) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Jun 27, 2019
@edvbld
edvbld approved these changes Jun 27, 2019
Copy link
Member

edvbld left a comment

Thanks @jeyvison, this PR looks good now 👍

@openjdk
Copy link

openjdk bot commented Jun 27, 2019

The PR review by Erik Helin (ehelin - Reviewer) has been updated - changes are approved.

@rwestberg
Copy link
Member

rwestberg commented Jun 27, 2019

/sponsor

@openjdk openjdk bot closed this Jun 27, 2019
@openjdk
Copy link

openjdk bot commented Jun 27, 2019

@rwestberg @jeyvison The following commits have been pushed to master since your change was applied:

Your commit was automatically rebased without conflicts.
Pushed as commit c4cc930.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.