cargo icon indicating copy to clipboard operation
cargo copied to clipboard

feat: Stablize `CARGO_RUSTC_CURRENT_DIR`

Open epage opened this issue 1 year ago • 10 comments
trafficstars

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/trycmd#247, falling back to finding a parent of CARGO_MANIFEST_DIR, if present. This has been in use in Cargo since rust-lang/cargo#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

epage avatar Mar 26 '24 01:03 epage

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 avatar Mar 26 '24 01:03 rustbot

@rfcbot fcp merge

epage avatar Mar 26 '24 20:03 epage

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

  • [x] @Eh2406
  • [ ] @Muscraft
  • [x] @arlosi
  • [ ] @ehuss
  • [x] @epage
  • [x] @joshtriplett
  • [x] @weihanglo

Concerns:

  • file-is-meant-for-debug (https://github.com/rust-lang/cargo/pull/13644#issuecomment--1995866749)
  • interaction-with-trim-paths (https://github.com/rust-lang/cargo/pull/13644#issuecomment--1995866749)

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 avatar Mar 26 '24 20:03 rfcbot

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

ehuss avatar Apr 01 '24 19:04 ehuss

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 https://github.com/rust-lang/cargo/issues/3946#issuecomment-1402464796

epage avatar Apr 01 '24 20:04 epage

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?

ehuss avatar Apr 03 '24 02:04 ehuss

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?

bukowa avatar Jun 19 '24 20:06 bukowa

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

bors avatar Jun 21 '24 18:06 bors

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

rfcbot avatar Aug 20 '24 14:08 rfcbot

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

ehuss avatar Aug 20 '24 15:08 ehuss

How does this interact with caching, specifically https://github.com/rust-lang/cargo/pull/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?

RalfJung avatar Aug 25 '24 10:08 RalfJung

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.

epage avatar Aug 26 '24 21:08 epage

Never mind, I misunderstood what this feature does.

RalfJung avatar Aug 27 '24 05:08 RalfJung

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 avatar Sep 12 '24 13:09 epage

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

epage avatar Nov 08 '24 22:11 epage