Skip to content

//@ {unset-,}{rustc,exec}-env parsing is a footgun #132990

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
Closed
jieyouxu opened this issue Nov 13, 2024 · 5 comments · Fixed by #139507
Closed

//@ {unset-,}{rustc,exec}-env parsing is a footgun #132990

jieyouxu opened this issue Nov 13, 2024 · 5 comments · Fixed by #139507
Labels
A-compiletest Area: The compiletest test runner C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jieyouxu
Copy link
Member

Apparently

//@ rustc-env: RUSTC_BOOTSTRAP=1

is not the same as

//@ rustc-env:RUSTC_BOOTSTRAP=1

compiletest will parse the former as an env var called ⌴RUSTC_BOOTSTRAP (incl. the whitespace).
@jieyouxu jieyouxu added A-compiletest Area: The compiletest test runner C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Nov 13, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 13, 2024
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 13, 2024
@Shunpoco
Copy link
Contributor

Shunpoco commented Nov 16, 2024 •
edited
Loading

Hi, I'd like to take this ticket, but what is the best solution?

  1. Accept such whitespace: I think we will modify Config::parse_env to remove such whitespaces,
  2. Fix all test codes which put a whitespace, or
  3. Panic when parse_env detects such a whitespace

@jieyouxu
Copy link
Member Author

I haven't quite determined what's the best solution to this yet.

@jieyouxu jieyouxu added E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it labels Nov 17, 2024
@jieyouxu
Copy link
Member Author

Tagging as E-hard and E-needs-design because it needs to not be a footgun on its own but also it needs to be homogenous with other directive syntaxes.

@Shunpoco
Copy link
Contributor

Got it, thanks. Until the design is done, whitespace is clearly misconfiguration. Can I fix them?
(For example, I think we should remove whitespace from https://github.com/rust-lang/rust/blob/master/tests/ui/proc-macro/env.rs#L3)

@jieyouxu
Copy link
Member Author

Yes, for the time being that sounds like a wise plan, I think the ones with whitespace in tests are probably wrong.

@Shunpoco Shunpoco mentioned this issue Nov 17, 2024
Closed
@jieyouxu jieyouxu mentioned this issue Apr 8, 2025
Merged
Zalathar added a commit to Zalathar/rust that referenced this issue Apr 8, 2025
@Zalathar
Rollup merge of rust-lang#139507 - Zalathar:trim-env-name, r=jieyouxu
7d32832 
compiletest: Trim whitespace from environment variable names

When a test contains a directive like `//@ exec-env: FOO=bar`, compiletest currently includes that leading space in the name of the environment variable, so it is defined as ` FOO` instead of `FOO`.

This is an annoying footgun that is pretty much never intended, especially since most other directives *do* trim whitespace. So let's get rid of it by trimming the environment variable name.

Values remain untrimmed, since there could conceivably be a use-case for values with leading space, but perhaps we'll end up trimming values too in the future.

Recently observed in rust-lang#138603 (comment).

Fixes rust-lang#132990.
Supersedes rust-lang#133148.

r? jieyouxu
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 8, 2025
@bors
Auto merge of rust-lang#139507 - Zalathar:trim-env-name, r=<try>
6b40b1d 
compiletest: Trim whitespace from environment variable names

When a test contains a directive like `//@ exec-env: FOO=bar`, compiletest currently includes that leading space in the name of the environment variable, so it is defined as ` FOO` instead of `FOO`.

This is an annoying footgun that is pretty much never intended, especially since most other directives *do* trim whitespace. So let's get rid of it by trimming the environment variable name.

Values remain untrimmed, since there could conceivably be a use-case for values with leading space, but perhaps we'll end up trimming values too in the future.

Recently observed in rust-lang#138603 (comment).

Fixes rust-lang#132990.
Supersedes rust-lang#133148.

---

try-job: test-various
Zalathar added a commit to Zalathar/rust that referenced this issue Apr 10, 2025
@Zalathar
Rollup merge of rust-lang#139507 - Zalathar:trim-env-name, r=jieyouxu
f5b6e88 
compiletest: Trim whitespace from environment variable names

When a test contains a directive like `//@ exec-env: FOO=bar`, compiletest currently includes that leading space in the name of the environment variable, so it is defined as ` FOO` instead of `FOO`.

This is an annoying footgun that is pretty much never intended, especially since most other directives *do* trim whitespace. So let's get rid of it by trimming the environment variable name.

Values remain untrimmed, since there could conceivably be a use-case for values with leading space, but perhaps we'll end up trimming values too in the future.

Recently observed in rust-lang#138603 (comment).

Fixes rust-lang#132990.
Supersedes rust-lang#133148.

---

try-job: test-various
Zalathar added a commit to Zalathar/rust that referenced this issue Apr 10, 2025
@Zalathar
Rollup merge of rust-lang#139507 - Zalathar:trim-env-name, r=jieyouxu
8eb9358 
compiletest: Trim whitespace from environment variable names

When a test contains a directive like `//@ exec-env: FOO=bar`, compiletest currently includes that leading space in the name of the environment variable, so it is defined as ` FOO` instead of `FOO`.

This is an annoying footgun that is pretty much never intended, especially since most other directives *do* trim whitespace. So let's get rid of it by trimming the environment variable name.

Values remain untrimmed, since there could conceivably be a use-case for values with leading space, but perhaps we'll end up trimming values too in the future.

Recently observed in rust-lang#138603 (comment).

Fixes rust-lang#132990.
Supersedes rust-lang#133148.

---

try-job: test-various
@bors bors closed this as completed in 2b0e47e Apr 10, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 10, 2025
@rust-timer
Unrolled build for rust-lang#139507
18a4052 
Rollup merge of rust-lang#139507 - Zalathar:trim-env-name, r=jieyouxu

compiletest: Trim whitespace from environment variable names

When a test contains a directive like `//@ exec-env: FOO=bar`, compiletest currently includes that leading space in the name of the environment variable, so it is defined as ` FOO` instead of `FOO`.

This is an annoying footgun that is pretty much never intended, especially since most other directives *do* trim whitespace. So let's get rid of it by trimming the environment variable name.

Values remain untrimmed, since there could conceivably be a use-case for values with leading space, but perhaps we'll end up trimming values too in the future.

Recently observed in rust-lang#138603 (comment).

Fixes rust-lang#132990.
Supersedes rust-lang#133148.

---

try-job: test-various
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-compiletest Area: The compiletest test runner C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
compiletest maintenance and improvements
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

compiletest: Trim whitespace from environment variable names Zalathar/rust
3 participants
@Shunpoco @jieyouxu @rustbot