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

Setup cargo environment for cargo rustc --print #15026

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Jan 7, 2025

It turns out, running cargo rustc --print cfg -Zunstable-options (and the like, #9357) fail with .cargo/config.toml setups like

[build]
# custom target json that lives in `./targets/my-super-cool-target.json`
target = "my-super-cool-target"

[env]
RUST_TARGET_PATH = { value = "./targets", relative = true }

resulting in


❯ cargo rustc --print cfg -Zunstable-options
error: Error loading target specification: Could not find specification for target "my-super-cool-target". Run `rustc --print target-list` for a list of built-in targets

error: process didn't exit successfully: `C:\Users\lukas\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\bin\rustc.exe --target my-super-cool-target --print cfg` (exit code: 1)

The reason for that is that cargo recognizes the target from the .cargo/config and then implicitly passes that along to the spawned rustc process, but it does so without passing along the important environment that is required for the target tuple to make sense.

(can add a test if desired, just tell me where)

@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2025

r? @ehuss

rustbot has assigned @ehuss.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2025
Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

Dug around and not seeing other places where we miss apply_env_config

@@ -188,6 +188,7 @@ pub fn print<'a>(
}
let target_info = TargetInfo::new(gctx, &build_config.requested_kinds, &rustc, *kind)?;
let mut process = rustc.process();
apply_env_config(gctx, &mut process)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

(can add a test if desired, just tell me where)

See https://github.com/rust-lang/cargo/blob/master/tests/testsuite/rustc.rs

Looks like we have --print tests in there. Unsure about custom targets.

We generally ask that PRs are split into

  • One commit that adds the test, showing the currently broken behavior (ie it passes)
  • The fix commit that updates the test to pass with the change

This makes the diff between the two commits show the change in behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah makes sense, will split the commit

Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

Forgot about a test

@Veykril Veykril force-pushed the veykril/push-uyynntmvqqxs branch from 6d18296 to 88797f4 Compare January 7, 2025 17:48
@Veykril Veykril force-pushed the veykril/push-uyynntmvqqxs branch from 88797f4 to b7a0c9d Compare January 7, 2025 17:57
@Veykril Veykril requested a review from epage January 7, 2025 17:58
@epage epage added this pull request to the merge queue Jan 7, 2025
@epage
Copy link
Contributor

epage commented Jan 7, 2025

Thanks!

Merged via the queue into rust-lang:master with commit 83615cf Jan 7, 2025
21 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 11, 2025
Update cargo

18 commits in fd784878cfa843e3e29a6654ecf564c62fae6735..088d496082726091024f1689c124a0c3dccbd775
2025-01-03 20:06:26 +0000 to 2025-01-10 20:10:21 +0000
- docs(reference): Fix PkgIdSpec kind docs (rust-lang/cargo#15049)
- feat: Added warning when failing to update index cache (rust-lang/cargo#15014)
- docs(ref): Fix the inverted logic about MSRV (rust-lang/cargo#15044)
- chore(deps): update msrv (1 version) to v1.84 (rust-lang/cargo#15041)
- Remove unnecessary into conversions (rust-lang/cargo#15042)
- docs(contrib): Start guidelines for schema design (rust-lang/cargo#15037)
- fix: emit warnings as warnings when learning rust target info (rust-lang/cargo#15036)
- fix(schemas): Fix the `[lints]` JSON Schema (rust-lang/cargo#15035)
- fix(schemas): Fix 'metadata' JSON Schema (rust-lang/cargo#15033)
- shorten comment on Ord for SourceKind (rust-lang/cargo#15029)
- Make `"C"` explicit in `extern "C"`. (rust-lang/cargo#15034)
- simplify SourceID Ord/Eq (rust-lang/cargo#14980)
- Setup cargo environment for `cargo rustc --print` (rust-lang/cargo#15026)
- Avoid naming variables `str` (rust-lang/cargo#15025)
- Bump to 0.87.0; update changelog (rust-lang/cargo#15022)
- Update libgit2 to 1.9 (rust-lang/cargo#15018)
- Remove condition on RUSTUP_WINDOWS_PATH_ADD_BIN (rust-lang/cargo#15017)
- Fix https::self_signed_should_fail for macos (rust-lang/cargo#15016)
@rustbot rustbot added this to the 1.86.0 milestone Jan 11, 2025
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants