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.

Sign up for GitHub

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

Jump to bottom

Stablize anonymous pipe #137793

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Stablize anonymous pipe #137793

wants to merge 2 commits into from
+249 −225

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented Feb 28, 2025 •
edited
Loading

Since #135822 is staled, I create this PR to stablise anonymous pipe

Closes #127154

@rustbot

This comment was marked as resolved.

Sign in to view
@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 28, 2025

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

The Miri subtree was changed

cc @rust-lang/miri

@jieyouxu
Copy link
Member

Rolling the original libs reviewer for the other stabilization attempt.
r? @joboet

@rustbot rustbot assigned joboet and unassigned ibraheemdev Feb 28, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Feb 28, 2025

Please squash the commits

@NobodyXu NobodyXu force-pushed the stablise-annoymous-pipe branch from 791a3fd to 57bdbe6 Compare February 28, 2025 14:37
@rustbot
Copy link
Collaborator

rustbot commented Feb 28, 2025

Some changes occurred in src/tools/cargo

cc @ehuss

@jieyouxu
Copy link
Member

(Some unintentional submodule changes I think)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2025
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 1, 2025

The CI error is not related to any changes in this PR:

tidy error: could not find exception package `foldhash`

@NobodyXu NobodyXu changed the title Stablise anonymous pipe Stablize anonymous pipe Mar 1, 2025
@tgross35
Copy link
Contributor

tgross35 commented Mar 1, 2025

#135822 also did some refactoring, are you also planning to put that up? If you do it in a separate PR feel free to request a review from me and we can probably merge it pretty quick.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 1, 2025

If you do it in a separate PR feel free to request a review from me and we can probably merge it pretty quick.

Yes I want to put it in separate PR, I hope this PR can merge quick and hope that debian could pick it up, so that jobserver/etc can use it to reduce their burden

@NobodyXu NobodyXu force-pushed the stablise-annoymous-pipe branch from 57bdbe6 to 8acff46 Compare March 1, 2025 13:13
@NobodyXu NobodyXu force-pushed the stablise-annoymous-pipe branch from 8acff46 to 7acb0e3 Compare March 1, 2025 13:35
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 1, 2025

Reverted submodules changes, and tidy passed locally, so CI should be fixed now

@jieyouxu jieyouxu mentioned this pull request Mar 1, 2025
Open
6 tasks
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 1, 2025

cc @joboet this pr is ready for review

@joshtriplett
Copy link
Member

LGTM

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 4, 2025

Can we get this PR merged please?

joboet
joboet requested changes Mar 5, 2025
View reviewed changes
Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

I still have the same request as in the original PR:

The changes here look good to me, though I'd like to see another change before stabilization:

Currently, things like the Stdio: From<PipeWriter> implementations live in sys:: which IMHO is really confusing and dangerous (because people will forget to add these implementations when porting std). Could you move all the public trait implementations to io::pipe (and os::_::pipe for the platform-specific implementations)?

@joboet
Copy link
Member

joboet commented Mar 5, 2025

cc @joboet this pr is ready for review

Please make sure to set the PR label (S-waiting-on-author) to the current state, using e.g. @rustbot ready. I have quite a high number of assigned PRs recently – and also a life, so I'd rather not invest my time into continually observing the state of each one.

Can we get this PR merged please?

Yes, but I don't think five days are an unreasonable long wait period. I have been rather busy over the last few weeks, so please excuse me if I'm taking a bit longer to respond.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 6, 2025 •
edited
Loading

The changes here look good to me, though I'd like to see another change before stabilization:

I think this can be done in a separate PR after this one is merged?

I don't think it should block this PR and it is outside the scope of this PR.
I can submit another PR after this PR is merged to fix it.

@tgross35
Copy link
Contributor

tgross35 commented Mar 6, 2025

(there are three weeks until the beta branch, we don't have much of a rush)

@rustbot rustbot added A-tidy Area: The tidy tool T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 7, 2025
@NobodyXu NobodyXu force-pushed the stablise-annoymous-pipe branch from 22af408 to 4c842c0 Compare March 7, 2025 15:05
@rustbot rustbot added O-unix Operating system: Unix-like O-windows Operating system: Windows labels Mar 7, 2025
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 7, 2025

I have moved all the OS-specific trait implementations to std::os::*, once the CI is green it should be ready for review

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 7, 2025

A-tidy and T-bootstrap should be removed, as changes touching that is reverted

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 8, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2025
@joshtriplett
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 9, 2025

📌 Commit 4c842c0 has been approved by joshtriplett

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 9, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 9, 2025
@matthiaskrgr
Rollup merge of rust-lang#137793 - NobodyXu:stablise-annoymous-pipe, …
78c3d99 
…r=joshtriplett

Stablize anonymous pipe

Since rust-lang#135822 is staled, I create this PR to stablise anonymous pipe

Closes rust-lang#127154
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 9, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2025
@bors
Auto merge of rust-lang#138289 - matthiaskrgr:rollup-8wl01e5, r=matth…
5c4b604 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#137279 (Make some invalid codegen attr errors structured/translatable)
 - rust-lang#137793 (Stablize anonymous pipe)
 - rust-lang#138238 (Fix dyn -> param suggestion in struct ICEs)
 - rust-lang#138270 (chore: Fix some comments)
 - rust-lang#138280 (fix ICE in pretty-printing `global_asm!`)
 - rust-lang#138286 (triagebot.toml: Don't label `test/rustdoc-json` as A-rustdoc-search (…)

r? `@ghost`
`@rustbot` modify labels: rollup
@compiler-errors
Copy link
Member

#138289 (comment) failed in a rollup

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu left review comments

@joboet joboet joboet requested changes

Assignees

@joboet joboet

Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-tidy Area: The tidy tool O-unix Operating system: Unix-like O-windows Operating system: Windows S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tracking Issue for anonymous pipe API