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

feat: Stablize CARGO_RUSTC_CURRENT_DIR #13644

Closed
wants to merge 1 commit into from

Conversation

epage
Copy link
Contributor

@epage epage commented Mar 26, 2024

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 rustcs 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 #13441.

This was added in #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 #3946 was overly broad and covered many use cases. One of those was for packages to look up information on their dependents.
Issue #13484 is being left open to track that.

Fixes #3946

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 added the T-cargo Team: Cargo label Mar 26, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 26, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
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 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 Mar 26, 2024
@epage epage marked this pull request as draft March 26, 2024 01:35
@epage epage marked this pull request as ready for review March 26, 2024 20:42
@epage
Copy link
Contributor Author

epage commented Mar 26, 2024

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 26, 2024

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Mar 26, 2024
Comment on lines +703 to +705
base.get_cwd()
.map(|c| c.display().to_string())
.unwrap_or(String::new()),
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if it could just be.

Suggested change
base.get_cwd()
.map(|c| c.display().to_string())
.unwrap_or(String::new()),
base.get_cwd().unwrap_or_default(),

(and ditto for CARGO_TARGET_TMPDIR? though that is not relevant to this PR)

@@ -265,7 +265,7 @@ corresponding environment variable is set to the empty string, `""`.
where integration tests or benchmarks are free to put any data needed by
the tests/benches. Cargo initially creates this directory but doesn't
manage its content in any way, this is the responsibility of the test code.
* `CARGO_RUSTC_CURRENT_DIR` --- This is a path that `rustc` is invoked from **(nightly only)**.
* `CARGO_RUSTC_CURRENT_DIR` --- This is a path that `rustc` is invoked from
Copy link
Member

Choose a reason for hiding this comment

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

Still feel like people might overlook this, as it isn't an obvious thing easy to relate to their problems. That said, I am fine with the previous explanation. Just forward it here if people have the same question: #12996 (comment).

When we talked about this, Eric was hesitant about locking down the behavior of file! so I left out that use case. It should work and I can't imagine rustc doing anything but absolute or relative to std::env::current_dir but I'm leaving that to people to make the choice that they want to make those assumptions, rather than us advertising it.

@ehuss
Copy link
Contributor

ehuss commented Apr 1, 2024

Is there some information about the use cases for this? How is it expected to be used?

@epage
Copy link
Contributor Author

epage commented Apr 1, 2024

Is there some information about the use cases for this? How is it expected to be used?

One of the requests in #3946 is a way to be able to resolve std::file!() so that the paths can be looked up at runtime. A common use for this is in snapshot testing.

For example, snapbox::path::current_dir!) is useful for

  • Finding snapshot files relative to the test mod
  • Finding the test mod for updating of inline snapshots

Possible workarounds

  • CARGO_MANIFEST_DIR: Cargo initially used this for UI tests but ran into problems when being inside of the rust workspace (when we used to do that)
    • Like in snapbox, this can be augmented by a manual path walk that mostly works
  • Defining a blank path for the CARGO_WORKSPACE_DIR in [env] config. This does find the workspace dir but that isn't always the current-dir used for rustc which is what std::file! is determined by
  • Combination of the above

As you mentioned the last time the Cargo team discussed this, std::file!s behavior is not specified.

For a summary of the team's last discussion on this, see #3946 (comment)

@ehuss
Copy link
Contributor

ehuss commented Apr 3, 2024

I am concerned that if the primary (or only?) way to use this environment variable is with file!, and file! isn't defined and primarily intended for debugging purposes that it is changing the meaning and relationship of file!. There might also be concerns that if something like trim-paths changes the behavior of file!. It also seems to provide a way to circumvent trim-paths, which seems a bit concerning.

Were there other considerations made, such as adding something like abs_file!()?

Have there been any discussions with the libs team, and what their thoughts are on making file! be used for something other than debugging?

This is another benefit to starting life as test/bench only as it lowers the barrier for abusing file!.

Is this limited to only test/bench?

@bukowa
Copy link

bukowa commented Jun 19, 2024

I'm not sure if this is the appropriate place and topic—if not, feel free to delete this message.

I'm wondering how to obtain the full path to the current file, regardless of whether I'm on Linux, Windows, or Mac. I would like it to be absolute so that I can start testing my library—specifically, file operations that are located in the tests/fixtures folder within my test library path. Therefore, obtaining this path is very crucial for the proper functioning of the tests in my program.

Now imagine that I am working with a workspace, or I am working with a workspace where one package is dependent on another package, and they have different tests... so how can I get the absolute path to the current file? So that I can always move the test_fixture.rs file along with the fixtures folder - not worrying about implementation details?

@bors
Copy link
Contributor

bors commented Jun 21, 2024

☔ The latest upstream changes (presumably #14068) made this pull request unmergeable. Please resolve the merge conflicts.

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Aug 20, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 20, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@ehuss
Copy link
Contributor

ehuss commented Aug 20, 2024

@rfcbot concern file-is-meant-for-debug
@rfcbot concern interaction-with-trim-paths

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. and removed final-comment-period FCP — a period for last comments before action is taken labels Aug 20, 2024
derezzedex added a commit to derezzedex/iced that referenced this pull request Aug 23, 2024
this is needed to easily create a path from the runtime use of `panic::Location`, see rust-lang/cargo#13644
@RalfJung
Copy link
Member

How does this interact with caching, specifically #4788? A bunch of work was done (causing quite painful side-effects) to ensure that a project can be moved to a different location in the file system without invalidating the cache. But if the current absolute dir is an input to the build then that can't work, can it?

@epage
Copy link
Contributor Author

epage commented Aug 26, 2024

This would only break caching if you use it and only for those build targets, just like with CARGO_MANIFEST_DIR.

For trim-paths, I see this having a similar interaction as CARGO_MANIFEST_DIR.

For file!s intended purpose, I talked with libs-api about a counter-proposal for this to be handled in the standard library and they pushed back as well. They seemed fine with us solving it but its been a few months and I do not remember if we explicitly called out this re-purposing of file!. I will note that the documentation for this doesn't call out file! and it would be left for the users to decide to do like they are doing today. If we change file!, we'll have to decide if we are ok with breaking people who assumed more than they should. The situation here doesn't change too much. We do streamline it but most users likely already have file! abstracted away from them in a snapshotting API and so doing so is unlikely to affect adoption.

@RalfJung
Copy link
Member

Never mind, I misunderstood what this feature does.

@epage
Copy link
Contributor Author

epage commented Sep 12, 2024

Discussed this with @Amanieu at RustConf about this.

In stepping through the use cases, he is now leaning towards file_absolute!(). This potentially would warn or error if trim paths is enabled.

file! documentation should be updated to either

  • unspecified and should only be used for diagnostic output and not for programmatic usage
  • specified precisely.

This might need to go through compiler (MCP) and libs-api (ACP)...

@epage epage closed this Sep 19, 2024
@epage epage deleted the rustc_current_dir branch September 19, 2024 16:54
@rfcbot rfcbot removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Sep 19, 2024
derezzedex added a commit to derezzedex/iced that referenced this pull request Sep 24, 2024
this is needed to easily create a path from the runtime use of `panic::Location`, see rust-lang/cargo#13644
@epage
Copy link
Contributor Author

epage commented Nov 8, 2024

Created the ACP for file_absolute! at rust-lang/libs-team#478

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-cargo Team: Cargo
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Environment variable for Cargo Workspace
8 participants