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.

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

feat: Add CARGO_RUSTC_CURRENT_DIR (unstable) #12996

Merged
merged 3 commits into from
Nov 24, 2023
Merged

feat: Add CARGO_RUSTC_CURRENT_DIR (unstable) #12996

merged 3 commits into from
Nov 24, 2023

Conversation

epage
Copy link
Contributor

@epage epage commented Nov 17, 2023

What does this PR try to resolve?

This is an alternative to #12158's CARGO_WORKSPACE_DIR that was
implementing the solution to #3946 that previously discussed in the
cargo team meeting.

CARGO_WORKSPACE_DIR is a bit awkward to document / describe because
its the effective workspace directory of the thing being built.
If the thing being built doesn't have a workspace, it falls back to
CARGO_MANIFEST_DIR.

It would also be hard to take into account what the
CARGO_WORKSPACE_DIR would be for path dependencies into foreign
workspaces and it wouldn't solve the problem the user is having.

What the user really wants is the CWD of rustc when it is invoked.
This is much simpler to describe and is accurate when using a path
dependency to a foreign package.

Because the CWD is a much simpler mechanism to talk about, I figured we
could diverge from our prior consensus and make it always present,
rather than limiting it to tests.

How should we test and review this PR?

The preparatory refactor commits have explanation for why they were to help

Additional information

Remaining work for #3946: get this stabilized

epage and others added 3 commits November 17, 2023 09:09
@epage
refactor(compile): Always add base args first
b3d0eeb 
This will make it less likely that anything that depends on them will
get moved before them and makes it easier to consistently add logic that
depends on them.
@epage
test(compiler): Make inner test names more specific
c75c456 
This will make it easier to verify other env variables later
@yerke @epage
feat: Add CARGO_RUSTC_CURRENT_DIR
e81d84c 
This is an alternative to rust-lang#12158's `CARGO_WORKSPACE_DIR` that was
implementing the solution to rust-lang#3946 that previously discussed in the
cargo team meeting.

`CARGO_WORKSPACE_DIR` is a bit awkward to document / describe because
its the effective workspace directory of the thing being built.
If the thing being built doesn't have a workspace, it falls back to
`CARGO_MANIFEST_DIR`.

It would also be hard to take into account what the
`CARGO_WORKSPACE_DIR` would be for path dependencies into foreign
workspaces *and* it wouldn't solve the problem the user is having.

What the user really wants is the CWD of rustc when it is invoked.
This is much simpler to describe and is accurate when using a path
dependency to a foreign package.

Because the CWD is a much simpler mechanism to talk about, I figured we
could diverge from our prior consensus and make it always present,
rather than limiting it to tests.

Remaining work for rust-lang#3946: get this stabilized
@rustbot
Copy link
Collaborator

rustbot commented Nov 17, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2023
@epage epage changed the title Current dir feat: Add CARGO_RUSTC_CURRENT_DIR Nov 17, 2023
@epage epage changed the title feat: Add CARGO_RUSTC_CURRENT_DIR feat: Add CARGO_RUSTC_CURRENT_DIR (nightly-only) Nov 17, 2023
@epage epage changed the title feat: Add CARGO_RUSTC_CURRENT_DIR (nightly-only) feat: Add CARGO_RUSTC_CURRENT_DIR (unstable) Nov 17, 2023
@epage epage force-pushed the current_dir branch 2 times, most recently from f2669b5 to 5d0e5e4 Compare November 17, 2023 21:45
weihanglo
weihanglo reviewed Nov 20, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

What the user really wants is the CWD of rustc when it is invoked.
This is much simpler to describe and is accurate when using a path
dependency to a foreign package.

I don't quite agree. People might just want to get the workspace root to do some tricks like generating code for other members. Granted this might seem like a "misuse" of build script but that is already a thing.

The proposed CARGO_RUSTC_CURRENT_DIR sounds harder to find and educate as well.

@epage
Copy link
Contributor Author

epage commented Nov 21, 2023

I don't quite agree. People might just want to get the workspace root to do some tricks like generating code for other members. Granted this might seem like a "misuse" of build script but that is already a thing.

A previous iteration of the cargo team rejected that use case. When we met to discuss the problem, we intentionally designed the solution to exclude that use case, only allowing it for benches and tests. We also made it relative to communicate its wibbly wobbly nature.

The proposed CARGO_RUSTC_CURRENT_DIR sounds harder to find and educate as well.

Note that I'm not talking about documenting a fully generic CARGO_WORKSPACE_DIR but specifically what we discussed to stabilize: #3946 (comment)

  • Only for test and bin (does that include unit tests? doc tests)
  • If a member of the primary workspace, we use the workspace path. For registry dependencies, it is the manifesr dir.
    • But when it comes to path dependencies on foreign workspaces...
  • Its a relative path

But really what we are trying to assist the user with is resolving file!. They are assuming the workspace dir is what they need but what they actually care about is the current_dir for rustc.

@epage epage force-pushed the current_dir branch 3 times, most recently from 77c1b46 to e81d84c Compare November 24, 2023 02:39
weihanglo
weihanglo reviewed Nov 24, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for the reply. Now I recalled the discussion. This is a good step forward.

I don't think an FCP is needed, but the env name might worth bikdshedding? don't have a better idea though. CARGO_RUSTC_INVOKED_DIR sounds even more confusing.

src/doc/src/reference/environment-variables.md Show resolved Hide resolved
@epage
Copy link
Contributor Author

epage commented Nov 24, 2023

I don't think an FCP is needed, but the env name might worth bikdshedding? don't have a better idea though. CARGO_RUSTC_INVOKED_DIR sounds even more confusing.

I figure we'd do an FCP on stabilization and can worry about the name then. I'd be curious to get this in a nightly for feedback from requesters.

@weihanglo
Copy link
Member

Thank you. All make sense as this is an nightly-only feature.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 24, 2023

📌 Commit e81d84c has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2023
@bors
Copy link
Contributor

bors commented Nov 24, 2023

⌛ Testing commit e81d84c with merge 9b13310...

@bors
Copy link
Contributor

bors commented Nov 24, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 9b13310 to master...

@bors bors merged commit 9b13310 into rust-lang:master Nov 24, 2023
20 checks passed
@bors bors mentioned this pull request Nov 24, 2023
Closed
@weihanglo weihanglo mentioned this pull request Nov 25, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 25, 2023
@bors
Auto merge of rust-lang#118275 - weihanglo:update-cargo, r=ehuss
f5dc265 
Update cargo

7 commits in 71cd3a926f0cf41eeaf9f2a7f2194b2aff85b0f6..9b13310ca596020a737aaa47daa4ed9ff8898a2f
2023-11-20 15:30:57 +0000 to 2023-11-24 16:20:51 +0000
- feat: Add `CARGO_RUSTC_CURRENT_DIR` (unstable) (rust-lang/cargo#12996)
- Exited with hard error when custom build file no existence or not in package (rust-lang/cargo#12995)
- try running on windows (rust-lang/cargo#13042)
- refactor(toml): Better abstract inheritance details (rust-lang/cargo#13021)
- cargo-test-support: Add features to the default Cargo.toml file (rust-lang/cargo#12997)
- Migrate rustfix to the cargo repo (rust-lang/cargo#13005)
- typo: rusc -> rustc (rust-lang/cargo#13019)

---

This also removes the check to ensure that `rustfix` between

* src/tools/cargo
* src/tools/compiletest

has the same version,
since `rust-lang/rustfix` has migrated to under `rust-lang/cargo`.

r? ghost
@rustbot rustbot added this to the 1.76.0 milestone Nov 25, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Nov 27, 2023
@bors
Auto merge of #118275 - weihanglo:update-cargo, r=ehuss
77a9dfe 
Update cargo

7 commits in 71cd3a926f0cf41eeaf9f2a7f2194b2aff85b0f6..9b13310ca596020a737aaa47daa4ed9ff8898a2f
2023-11-20 15:30:57 +0000 to 2023-11-24 16:20:51 +0000
- feat: Add `CARGO_RUSTC_CURRENT_DIR` (unstable) (rust-lang/cargo#12996)
- Exited with hard error when custom build file no existence or not in package (rust-lang/cargo#12995)
- try running on windows (rust-lang/cargo#13042)
- refactor(toml): Better abstract inheritance details (rust-lang/cargo#13021)
- cargo-test-support: Add features to the default Cargo.toml file (rust-lang/cargo#12997)
- Migrate rustfix to the cargo repo (rust-lang/cargo#13005)
- typo: rusc -> rustc (rust-lang/cargo#13019)

---

This also removes the check to ensure that `rustfix` between

* src/tools/cargo
* src/tools/compiletest

has the same version,
since `rust-lang/rustfix` has migrated to under `rust-lang/cargo`.

r? ghost
@epage epage deleted the current_dir branch November 28, 2023 17:51
@epage epage mentioned this pull request Nov 29, 2023
Open
epage added a commit to epage/cargo that referenced this pull request Mar 26, 2024
@epage
feat: Stablize CARGO_RUSTC_CURRENT_DIR
5a1fa7f 
This provides what cargo sets as the `current_dir` for the `rustc`
process.
While `std::file!` is unspecified in what it is relative to,
it is relatively safe, it is generally relative to `rustc`s
`current_dir`.

This can be useful for snapshot testing.
For example, `snapbox` has been using this macro on nightly since
assert-rs/snapbox#247, falling back to finding a parent of
`CARGO_MANIFEST_DIR`, if present.
This has been in use in Cargo since rust-lang#13441.

This was added in rust-lang#12996.
Relevant points discussed in that issue:
- This diverged from the original proposal from the Cargo team of having
  a `CARGO_WORKSPACE_DIR` that is the "workspace" of the package being
  built (ie registry packages would map to `CARGO_MANIFEST_DIR`).
  In looking at the `std::file!` use case, `CARGO_MANIFEST_DIR`, no
  matter how we defined it, would only sort of work because no sane
  definition of that maps to `rustc`'s `current_dir`.a
  This instead focuses on the mechanism currently being used.
- Using "current dir" in the name is meant to be consistent with
  `std::env::current_dir`.
- I can go either way on `CARGO_RUSTC` vs `RUSTC`.  Existing related
  variables:
  - `RUSTC`
  - `RUSTC_WRAPPER`
  - `RUSTC_WORKSPACE_WRAPPER`
  - `RUSTFLAGS` (no `C`)
  - `CARGO_CACHE_RUSTC_INFO`

Note that rust-lang#3946 was overly broad and covered many use cases.
One of those was for packages to look up information on their
dependents.
Issue rust-lang#13484 is being left open to track that.

Fixes rust-lang#3946
@epage epage mentioned this pull request Mar 26, 2024
Closed
epage added a commit to epage/cargo that referenced this pull request May 21, 2024
@epage
feat: Stablize CARGO_RUSTC_CURRENT_DIR
d60e025 
This provides what cargo sets as the `current_dir` for the `rustc`
process.
While `std::file!` is unspecified in what it is relative to,
it is relatively safe, it is generally relative to `rustc`s
`current_dir`.

This can be useful for snapshot testing.
For example, `snapbox` has been using this macro on nightly since
assert-rs/snapbox#247, falling back to finding a parent of
`CARGO_MANIFEST_DIR`, if present.
This has been in use in Cargo since rust-lang#13441.

This was added in rust-lang#12996.
Relevant points discussed in that issue:
- This diverged from the original proposal from the Cargo team of having
  a `CARGO_WORKSPACE_DIR` that is the "workspace" of the package being
  built (ie registry packages would map to `CARGO_MANIFEST_DIR`).
  In looking at the `std::file!` use case, `CARGO_MANIFEST_DIR`, no
  matter how we defined it, would only sort of work because no sane
  definition of that maps to `rustc`'s `current_dir`.a
  This instead focuses on the mechanism currently being used.
- Using "current dir" in the name is meant to be consistent with
  `std::env::current_dir`.
- I can go either way on `CARGO_RUSTC` vs `RUSTC`.  Existing related
  variables:
  - `RUSTC`
  - `RUSTC_WRAPPER`
  - `RUSTC_WORKSPACE_WRAPPER`
  - `RUSTFLAGS` (no `C`)
  - `CARGO_CACHE_RUSTC_INFO`

Note that rust-lang#3946 was overly broad and covered many use cases.
One of those was for packages to look up information on their
dependents.
Issue rust-lang#13484 is being left open to track that.

Fixes rust-lang#3946
@epage epage mentioned this pull request Nov 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo left review comments

Assignees

@ehuss ehuss

Labels
A-build-execution Area: anything dealing with executing the compiler A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
6 participants
@epage @rustbot @weihanglo @bors @ehuss @yerke