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

Build script regression prevents some builds from terminating #11196

Closed
Kirens opened this issue Oct 8, 2022 · 1 comment · Fixed by #11205
Closed

Build script regression prevents some builds from terminating #11196

Kirens opened this issue Oct 8, 2022 · 1 comment · Fixed by #11205
Labels
A-build-scripts Area: build.rs scripts C-bug Category: bug regression-from-stable-to-stable Regression in stable that worked in a previous stable release.

Comments

@Kirens
Copy link

Kirens commented Oct 8, 2022

Problem

It seems build scripts previously were executed with a terminated stdin, but not since 1.63.0.

The build script of a package that I am using checks for an optional build-time dependency with Command::new("exe").spawn().is_ok(). It is expected to terminate immediately, but in newer versions of cargo it's blocking the script from completing while awaiting input from stdin. A solution for this package is to add .stdin(Stdio::null()) to quickly resolve the issue.

I'm not sure if this is a deliberate change. But I can't seem to find any notes regarding this in the changelogs.

Steps

  1. Create a build.rs that Command::new(...).spawn()s a process that only terminates once stdin does (e.g. cat)
  2. cargo build with 1.62. Notice how it completes
  3. cargo build with 1.63. Notice how it freezes while building.

I wrote a simple proof-of-concept

Possible Solution(s)

I was unable to find the culprit. If it's a desired change, I'd suggest adding a note about it in the changelog.

Notes

No response

Version

cargo 1.62.0
release: 1.62.0
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.84.0 (sys:0.4.51+curl-7.80.0 system ssl:OpenSSL/1.1.1q)
os: NixOS 21.11.0 [64-bit]

------------------------------------------------------------------

cargo 1.63.0
release: 1.63.0
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.84.0 (sys:0.4.55+curl-7.83.1 system ssl:OpenSSL/1.1.1q)
os: NixOS 21.11.0 [64-bit]
@Kirens Kirens added the C-bug Category: bug label Oct 8, 2022
@ehuss
Copy link
Contributor

ehuss commented Oct 8, 2022

Thanks for the report! I believe this was an unintentional change by #10738. @arlosi, can you maybe take a look at fixing this? I think it should probably retain the old behavior. I'm not sure what the best solution might be, but I think perhaps ProcessBuilder should not pipe stdin unless explicitly instructed.

@ehuss ehuss added A-build-scripts Area: build.rs scripts regression-from-stable-to-stable Regression in stable that worked in a previous stable release. labels Oct 8, 2022
@bors bors closed this as completed in 642a0e6 Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts C-bug Category: bug regression-from-stable-to-stable Regression in stable that worked in a previous stable release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants