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

fix definition of open I/O to Process #39544

Merged
merged 2 commits into from
Jun 7, 2021
Merged

fix definition of open I/O to Process #39544

merged 2 commits into from
Jun 7, 2021

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 5, 2021

It turns out that ArgTools exports a number of test scripts that depend on this change. This was causing many of the failures of the aarch64 buildbot. Hopefully nobody else was depending on the reverse, but I think that should be far less likely (and likely a bug elsewhere in their code already anyways).

@vtjnash vtjnash added io Involving the I/O subsystem: libuv, read, write, etc. ci Continuous integration backport 1.6 Change should be backported to release-1.6 labels Feb 5, 2021
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantics change is fine to me. Before we had this return Process this would close whichever stream the user requested rather than .in (which would correspond to whether you were expecting to read from or write to the process), so closing both seems fine.

@Keno Keno added the minor change Marginal behavior change acceptable for a minor release label Feb 5, 2021
@vtjnash
Copy link
Member Author

vtjnash commented Feb 6, 2021

Yeah, the problem apparently will be this is a hard-break for ArgTools / Tar tests, which rely on the existence of SIGPIPE being produced by cat on Windows—none of which actually exist.

@StefanKarpinski
Copy link
Member

Should be ok to merge after #41059.

@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed backport 1.6 Change should be backported to release-1.6 labels Jun 2, 2021
It seems this behavior was relied upon in ArgTools, so we change the
behavior here to match. This is probably more in line with what people
expect (and if not, this makes the `success` check now fail noisily,
instead of hanging the process)!

Refs #38996
@vtjnash vtjnash merged commit 0a34309 into master Jun 7, 2021
@vtjnash vtjnash deleted the jn/22832 branch June 7, 2021 21:31
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration io Involving the I/O subsystem: libuv, read, write, etc. minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants