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

bootstrap: use nix crate to get rusage on unix #128290

Closed
wants to merge 1 commit into from

Conversation

GrigorenkoPV
Copy link
Contributor

Part of #109859

@GrigorenkoPV GrigorenkoPV marked this pull request as ready for review July 27, 2024 22:52
@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2024

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 27, 2024
src/bootstrap/Cargo.toml Outdated Show resolved Hide resolved
@Kobzol
Copy link
Contributor

Kobzol commented Jul 28, 2024

Hi, thanks for the PR! While removing unsafe from bootstrap in general is a good idea, this PR adds 4 more dependencies that need to be compiled on every bootstrap rebuild on Linux. I'm not sure if that's a good trade-off, especially when RUSTC_PRINT_STEP_RUSAGE seems kind of niche. I don't know about others, but I haven't personally ever used it, and it also doesn't seem to be used on CI by default.

The dependency cost could be amortized if we used it for more things, of course. I checked #109859 and it seems like nix could also help with getuid, but not with setpriority (based on what is currently offered by the nix crate). I'm still unsure whether it's a good enough value proposition for rusage and getuid.

@onur-ozkan What do you think?

@Kobzol
Copy link
Contributor

Kobzol commented Jul 28, 2024

For context, prior discussion: #116476

@onur-ozkan
Copy link
Member

onur-ozkan commented Jul 28, 2024

I am not afraid of having unsafe blocks in bootstrap, but it's definitely an improvement if we can replace them with safe blocks. However, adding dependencies to achieve this brings more problems than solutions. Also, many of these dependencies follow the logic we already do in bootstrap, which is why we decided not to merge #116476.

It seems we should update some items of #109859 as it is leading to incorrect directions that we don't want to follow on bootstrap.

Thanks for the ping! @Kobzol

@GrigorenkoPV
Copy link
Contributor Author

The dependency cost could be amortized if we used it for more things, of course. I checked #109859 and it seems like nix could also help with getuid, but not with setpriority (based on what is currently offered by the nix crate). I'm still unsure whether it's a good enough value proposition for rusage and getuid.

@onur-ozkan, do I get it right that it's a no from you even in this case?

It seems we should update some items of #109859 as it is leading to incorrect directions that we don't want to follow on bootstrap.

That's what I've been trying to do:
#109859 (comment)

In all honesty, I did not really expect this PR get accepted, I mostly wanted a confirmation that unsafe for rusage on unix is a non-issue and will not be addressed.

@JohnCSimon
Copy link
Member

Triage:

I did not really expect this PR get accepted

@GrigorenkoPV Maybe close this PR?

@GrigorenkoPV
Copy link
Contributor Author

@GrigorenkoPV Maybe close this PR?

I would still love to get a reply to my question above first.

The dependency cost could be amortized if we used it for more things, of course. I checked #109859 and it seems like nix could also help with getuid, but not with setpriority (based on what is currently offered by the nix crate). I'm still unsure whether it's a good enough value proposition for rusage and getuid.

@onur-ozkan, do I get it right that it's a no from you even in this case?

@onur-ozkan
Copy link
Member

@onur-ozkan, do I get it right that it's a no from you even in this case?

Yeah:

// SAFETY: getuid() system call is always successful and no return value is reserved
// to indicate an error.
//
// For more context, see https://man7.org/linux/man-pages/man2/geteuid.2.html
let uid = unsafe { libc::getuid() };

@GrigorenkoPV GrigorenkoPV deleted the bootstrap-rusage branch August 18, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants