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

jobserver updates jobserver-auth from old style R,W to new style fifo:PATH for children though children may not be compatible with fifo:PATH style auth #99

Closed
IceTDrinker opened this issue Jun 28, 2024 · 2 comments · Fixed by #100

Comments

@IceTDrinker
Copy link

IceTDrinker commented Jun 28, 2024

Hello,

As discussed in this zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/does.20Cargo.20inject.20fifo.3APATH.20style.20jobserver-auth.20in.20MAKEFLAG/near/447522824

We have a build case with rust that started failing on newer nightly toolchains, the crash stemmed from a fifo:PATH style jobserver-auth being forwarded in a build.rs to a make command (with version < 4.4) that is not compatible with the fifo:PATH style auth, when the make command tried to run it crashed indicating it did not support the fifo:PATH auth.

@the8472 pointed out this piece of code

jobserver-rs/src/unix.rs

Lines 168 to 181 in c0c2898

// Optimization: Try converting it to a fifo by using /dev/fd
//
// On linux, opening `/dev/fd/$fd` returns a fd with a new file description,
// so we can set `O_NONBLOCK` on it without affecting other processes.
//
// On macOS, opening `/dev/fd/$fd` seems to be the same as `File::try_clone`.
//
// I tested this on macOS 14 and Linux 6.5.13
#[cfg(target_os = "linux")]
if let Ok(Some(jobserver)) =
Self::from_fifo(&format!("fifo:/dev/fd/{}", read.as_raw_fd()))
{
return Ok(Some(jobserver));
}

which breaks compatibility if the new fifo:PATH auth is forwarded to incompatible children processes.

Thanks a lot for the swift support!

Cheers

@NobodyXu
Copy link
Contributor

I think we can fix this, by pass the fd + the path for fifo?

@the8472
Copy link
Member

the8472 commented Jun 28, 2024

We should pass-through whatever the parent gave us and as an optimization open a separate set of FDs for internal use on linux and not pass.

NobodyXu added a commit to NobodyXu/jobserver-rs that referenced this issue Jun 29, 2024
weihanglo added a commit to weihanglo/cargo that referenced this issue Jul 15, 2024
See rust-lang/jobserver-rs#99

While there is a fix waiting for review,
we might miss the release window for 1.80,
so propose we beta-backport this
bors added a commit to rust-lang/cargo that referenced this issue Jul 15, 2024
chore: downgrade to jobserver@0.1.28

See rust-lang/jobserver-rs#99

While there is a fix waiting for review,
we might miss the release window for 1.80,
so propose we beta-backport this
weihanglo added a commit to weihanglo/cargo that referenced this issue Jul 15, 2024
See rust-lang/jobserver-rs#99

While there is a fix waiting for review,
we might miss the release window for 1.80,
so propose we beta-backport this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants