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

Environment variable for Cargo Workspace #3946

Open
Ygg01 opened this issue Apr 24, 2017 · 55 comments
Open

Environment variable for Cargo Workspace #3946

Ygg01 opened this issue Apr 24, 2017 · 55 comments
Assignees
Labels
A-environment-variables Area: environment variables A-workspaces Area: workspaces C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@Ygg01
Copy link

Ygg01 commented Apr 24, 2017

T-cargo notes:

A CARGO_RUSTC_CURRENT_DIR is added as a nightly only environment variable. See #3946 (comment). Seek for feedback.


Hi, while working on using workspace in html5ever, I've ran into issue of needing the CARGO_WORKSPACE directory, and being unable, to find it. What I resorted to is essentially, &Path(cargo_manifest).join("..") which feels hacky.

Could CARGO_WORKSPACE be added as environment variable? I'm not sure what it should be when there is no workspace defined, I assume it should either return Err or default it to CARGO_MANIFEST_DIR.

Sidenote I'm willing to work on this issue, if I could get quick pointers, to what I need to do.

@shepmaster
Copy link
Member

when there is no workspace defined

I'd expect the environment variable to not be set at all in that case.

@Ygg01
Copy link
Author

Ygg01 commented Apr 24, 2017

What would adding the CARGO_WORKSPACE entail. From cursory look, I see custom_build.rs has reference to Context, that has reference to Workspace, but same doesn't exist for compilation.rs.

Would having CARGO_WORKSPACE only for custom builds be ok?

@alexcrichton
Copy link
Member

Thanks for the report! I think I may not quite be following what's going on here though? Do you mean accessing the workspace directory from a build script perhaps?

@Ygg01
Copy link
Author

Ygg01 commented Apr 24, 2017

@alexcrichton Yes. I was looking for workspace directory in my custom build script. It's related to servo/html5ever#261.

There is a simple workaround of taking CARGO_MANIFEST_DIR and using its parent, but I thought having CARGO_WORKSPACE would be cleaner solution.

@alexcrichton
Copy link
Member

Oh yeah definitely makes sense to me! Seems reasonable to basically enhance this section

@Ygg01
Copy link
Author

Ygg01 commented May 4, 2017

So if I understand correctly, if I expose workspace.root_manifest that would be workspace dir of all projects in workspace, correct? Then I can just:

if let Some(workspace_dir) = cx.ws.root_manifest() { 
    cmd.env("CARGO_WORKSPACE_DIR", workspace_dir);
}

@alexcrichton
Copy link
Member

sounds about right! I think you may not want precisely the root_manifest field but rather ws.root().join("Cargo.toml")

@Ygg01
Copy link
Author

Ygg01 commented May 8, 2017

@alexcrichton I am total newb, but won't adding Cargo.toml to workspace root, return the location of workspace manifest as opposed to directory of the directory in which workspace manifest is?

@alexcrichton
Copy link
Member

oh sure yeah, it just depends on the intent of what's being conveyed (the workspace manifest or the directory of the workspace), I'm fine with either.

@Ygg01
Copy link
Author

Ygg01 commented May 8, 2017

Hm, while writing tests, I've noticed a peculiarity. I assume I'm using this wrong. But I wanted to double check

let p = project("foo")
    .file("Cargo.toml", r#"
        [project]
        name = "foo"
        version = "0.5.0"
        authors = []


        [workspace]
        members = ["a"]
    "#)
    .file("src/lib.rs", "")
    .file("build.rs", r#"
        fn main() {
            //panic!("WILL FAIL");
        }
    "#)
    .file("a/Cargo.toml", r#"
        [project]
        name = "a"
        version = "0.5.0"
        authors = []
        links = "foo"
        build = "build.rs"
    "#)
    .file("a/src/lib.rs", "")
    .file("a/build.rs", r#"
        fn main() {
                panic!("PASSES?");
        }
    "#);
assert_that(p.cargo_process("build").arg("-v"),
                execs().with_status(0));

The panic in a/build.rs never happens, and panic in build.rs always happens, even if workspace doesn't have a build= "build.rs" line.

Idea behind tests was to verify that each member build.rs has appropriate value for environment variables.

@alexcrichton
Copy link
Member

@Ygg01 oh cargo build on a workspace doesn't execute all build scripts, and build.rs is inferred to be a build script if otherwise not specified.

@Ygg01
Copy link
Author

Ygg01 commented May 9, 2017

@alexcrichton Is there an alternative way to test env. variables are properly set in each member build script?

@alexcrichton
Copy link
Member

@Ygg01 I think you'd just cargo build in both directories, right? And then have an assert in the build script the env var is correct?

@Ygg01
Copy link
Author

Ygg01 commented May 10, 2017

Yes, I think that is correct (one call to cargo build in member directory and one call in workspace directory). Are there any examples of such code? I can only see adding files to ProjectBuilder file.

@alexcrichton
Copy link
Member

Oh you'll just want to call p.cargo multiple times basically, there's some other tests I believe which run cargo more than once.

@carols10cents carols10cents added A-environment-variables Area: environment variables A-workspaces Area: workspaces C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` labels Oct 2, 2017
@withoutboats
Copy link
Contributor

This was assigned to me to summarize why we didn't merge my PR which would have closed it #4787.

We decided to punt on this feature because of the question about what happens when building a crate downloaded from crates.io, which is no longer in a workspace in that form, but might have been originally produced in a workspace and have a build script that expects to have this env var set.

Its also unclear what the motivation for this variable is; my motivation was to find the lockfile, but I concluded that the best way to get the information I was getting from the lockfile was to run cargo metadata rather than actually read from Cargo.lock.

@ghost
Copy link

ghost commented Jan 23, 2018

Its also unclear what the motivation for this variable is;

One motivation would be to find the absolute path of the resulting binary executable. How I'm currently doing it.
EDIT: added the above.

run cargo metadata rather than

hey that's pretty cool:

$ pwd
/home/xftroxgpx/build/2nonpkgs/rust.stuff/rustlearnage/recompile_self
$ time cargo metadata --format-version 1 | json_reformat | grep workspace_root
    "workspace_root": "/home/xftroxgpx/build/2nonpkgs/rust.stuff/rustlearnage"

real	0m1.054s
user	0m0.847s
sys	0m0.214s

@Ygg01
Copy link
Author

Ygg01 commented Jan 23, 2018

@withoutboats my original motivation for this feature was when html5ever, was moving from one project per workspace to multiple. Namely some tests that were specific, became shared and not in the same directory they were left.

However fact that almost no one needed this feature, and it was easily implementable by other means, kinda made me think it's not needed.

I did forgot about it completely.

@Ygg01 Ygg01 closed this as completed Jan 23, 2018
@Ygg01 Ygg01 reopened this Jan 23, 2018
@peterhuene
Copy link

peterhuene commented Aug 12, 2018

I also have another use case for this feature: I'm trying to get the absolute path to the source file being compiled. I embed this as metadata from a procedural macro invocation so that source can be copied during a subsequent cargo run into a particular directory structure to integrate it into a third party, language-agnostic user interface.

When using a workspace, the file!() macro expands to be workspace directory relative (I was expecting it to be CARGO_MANIFEST_DIR relative, though, to be honest). Without a way to get the workspace directory, I have to rely on a hacky method of walking up the file!() path until I hit root, popping sub-directories off of CARGO_MANIFEST_DIR as I go.

Of course, there could easily be a better way to get the source file's absolute path that I'm completely ignorant of, so please let me know if that's the case. Thanks!

@mitsuhiko
Copy link
Contributor

I also ran into this problem where file! returns a workspace relative path but I cannot absolutize it which I need for some test related situations.

@mitsuhiko
Copy link
Contributor

This is the workaround i have in place now which is pretty ugly: https://github.com/mitsuhiko/insta/blob/b113499249584cb650150d2d01ed96ee66db6b30/src/runtime.rs#L67-L88

@epage epage added S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review E-medium Experience: Medium labels May 15, 2024
epage added a commit to epage/cargo that referenced this issue May 21, 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 `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
@Nemo157
Copy link
Member

Nemo157 commented Jun 4, 2024

Another usecase for this is the cargo-xtask pattern. If you want to have a config file for your xtask then you probably want this stored in the root of the workspace. An xtask crate is not designed to be used outside the workspace so avoids the issues around what happens post-publish.

EDIT: It looks like the CARGO_RUSTC_CURRENT_DIR would work for this currently, but it seems like it could change to not be the workspace root while keeping its semantics correct.

@Stargateur
Copy link

My use case is put data for testing in the root of our workspace so I need a simple way to do include_str!(concat!(env!(CARGO_ROOT_WORKSPACE), "data/foo.json")) in all my crates and yes I call it CARGO_ROOT_WORKSPACE :p

@epage
Copy link
Contributor

epage commented Jun 11, 2024

@Stargateur atm Cargo generally encourages a package to be self-contained, assuming it will be published to crates.io and can be tested from there (like for crater).

I think it would be interesting to evaluate what would be different in Cargo without the published assumption and how we reconcile Cargo functioning under the two workflows.

@Stargateur
Copy link

Stargateur commented Jun 11, 2024

I have no strong opinion about what cargo should encourage, I guess focus on "a package should be self contained for published crate" make sense, BUT workspace is not at all about that, for example tracing.workspace = true would never be valid for a published crate yet it's in cargo.toml. I think ask for a environnement variable that is the "root" of a project aka workspace is not a big demand. I mean it's asking since 2017... If we have workspace is also because sometime we want share thing between multiple crates.

Thus I happy enough with the trick of .cargo/config

@epage
Copy link
Contributor

epage commented Jun 11, 2024

for example tracing.workspace = true would never be valid for a published crate yet it's in cargo.toml.

That is different as Cargo has enough information to normalize this on publish.

@bukowa
Copy link

bukowa commented Jun 19, 2024

Use case:

use std::path::Path;

#[track_caller]
fn print_env(){
    let manifest_dir = env!("CARGO_MANIFEST_DIR");
    println!("MANIFEST_DIR: {:?}", manifest_dir);

    let caller_file = std::panic::Location::caller().file();
    println!("CALLER FILE: {:?}", caller_file);
    
    let root_dir = Path::new(manifest_dir);
    let this_file = root_dir.join(caller_file);
    println!("RESULT: {:?}", this_file);
}

fn main(){
    print_env()
}
MANIFEST_DIR: "/playground"
CALLER FILE: "src/main.rs"
RESULT: "/playground/src/main.rs"

@epage
Copy link
Contributor

epage commented Jun 19, 2024

@bukowa Making file! usable at runtime is one of the use cases we've been working towards with the unstable CARGO_RUSTC_CURRENT_DIR env variable. There has been debate between Cargo and Libs on which should solve the problem and we need to work to resolve it.

alilleybrinker added a commit to mitre/hipcheck that referenced this issue Sep 3, 2024
Turns out Cargo doesn't like when materials required to build a crate are
outside of the crate manifest directory. In this case, I'm talking about
the protobuf definition used for the Hipcheck plugin gRPC service.

There's a Cargo issue open about it, but the gist of it is that they
don't really want people doing what we were trying to do, having materials
in an outer directory.

There may be better solutions, but moving the files is the fastest one
for now.

Here's the issue: rust-lang/cargo#3946

Signed-off-by: Andrew Lilley Brinker <[email protected]>
alilleybrinker added a commit to mitre/hipcheck that referenced this issue Sep 3, 2024
Turns out Cargo doesn't like when materials required to build a crate are
outside of the crate manifest directory. In this case, I'm talking about
the protobuf definition used for the Hipcheck plugin gRPC service.

There's a Cargo issue open about it, but the gist of it is that they
don't really want people doing what we were trying to do, having materials
in an outer directory.

There may be better solutions, but moving the files is the fastest one
for now.

Here's the issue: rust-lang/cargo#3946

Signed-off-by: Andrew Lilley Brinker <[email protected]>
@epage
Copy link
Contributor

epage commented Sep 19, 2024

From #13644

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)...

As we work towards that, we'll need to remove CARGO_RUSTC_CURRENT_DIR.

Another thought I had was that we could change CARGO_RUSTC_CURRENT_DIR to always be CARGO_MANIFEST_DIR if we fixed the rendering if paths from compiler errors. However, this would likely make panic messages confusing.

@epage
Copy link
Contributor

epage commented Nov 8, 2024

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

@teohhanhui
Copy link

teohhanhui commented Nov 9, 2024

No idea how this issue deviated from getting the workspace directory, which for example is very useful for Bevy's asset directory (i.e. BEVY_ASSET_ROOT), to file_absolute! which is entirely useless for that use case...

@epage
Copy link
Contributor

epage commented Nov 11, 2024

@teohhanhui there are multiple use cases discussed in this thread. A clear cut use case is being able to figure out the runtime path to a source file for snapshot testing and other workflows (#3946 (comment), #3946 (comment), #3946 (comment)). That is what file_absolute! is trying to cover. I've been focusing on that and posting updates to this thread for a while.

That effort wouldn't close out this issue on its own as there are also other use cases covered here. For the ones that literally need the workspace root, the situation is less clear cut for moving forward atm:

JonasKruckenberg added a commit to JonasKruckenberg/k23 that referenced this issue Nov 20, 2024
JonasKruckenberg added a commit to JonasKruckenberg/k23 that referenced this issue Nov 20, 2024
* chore: update flake deps and bump rust nightly version

* refactor: add explicit lifetime names

* refactor: replace removed `CARGO_RUSTC_CURRENT_DIR`

ref: rust-lang/cargo#3946

* refactor: switch naked fns to `naked_asm!`

Switch to the newly required `naked_asm!` macro in all naked functions.
ref: rust-lang/rust#90957

* fix: update custom target manifest files to avoid ICE

apparently Rust doesn't like it when the custom target manifest file is missing the `llvm-abiname` field

* silence warning about mutable static

* refactor: elide lifetimes

* fmt

* refactor: apply `clippy::manual_div_ceil` suggestion

* fix: use explicit `ptr.wrapping_byte_add` for conversions from physmem to virtmem references

We previously relied on UB actually doing the wrapping behaviour, but well, that's not ideal is it
epage added a commit to epage/rust that referenced this issue Dec 17, 2024
This takes the current behavior of `file!` and documents it so it is
safe to make assumptions about.
For example, Cargo could provide a `CARGO_RUSTC_CURRENT_DIR` as a base
path for `file!`.

Example use cases
- Being able to look up test assets relative to the current file
  ([example](https://github.com/rust-lang/cargo/blob/b9026bf654d7fac283465e58b8b76742244ef07d/tests/testsuite/cargo_add/add_basic/mod.rs#L34))
- Inline snapshotting libraries being able to update Rust source code
  ([example](https://github.com/rust-lang/cargo/blob/b9026bf654d7fac283465e58b8b76742244ef07d/tests/testsuite/alt_registry.rs#L36-L45))

See rust-lang/cargo#3946 for more context.

T-libs-api discussed two solutions in rust-lang/libs-team#478
- `file_absolute!`:
  - Has less meaning in other build tools like buck2
  - Bakes in the assumption that a full path is available (e.g. with
    trim-paths)
- Specifying `file!`s behavior (this PR):
  - Leaves it to the user to deal with trim-paths
  - Even though `file!` is currently unspecified, changing it would
    likely have too large of an impact on the ecosystem at this time.

A future possibility is that rustc could have a flag that controls
modifies the base path used for `file!`.
That seems purely additive with specifying the behavior and we do not
want to block on it.
It would also likely be too disruptive for Cargo users (as mentioned).
However, we tried to keep this in mind when specifying the behavior.
epage added a commit to epage/rust that referenced this issue Dec 18, 2024
This takes the current behavior of `file!` and documents it so it is
safe to make assumptions about.
For example, Cargo could provide a `CARGO_RUSTC_CURRENT_DIR` as a base
path for `file!`.

Example use cases
- Being able to look up test assets relative to the current file
  ([example](https://github.com/rust-lang/cargo/blob/b9026bf654d7fac283465e58b8b76742244ef07d/tests/testsuite/cargo_add/add_basic/mod.rs#L34))
- Inline snapshotting libraries being able to update Rust source code
  ([example](https://github.com/rust-lang/cargo/blob/b9026bf654d7fac283465e58b8b76742244ef07d/tests/testsuite/alt_registry.rs#L36-L45))

See rust-lang/cargo#3946 for more context.

T-libs-api discussed two solutions in rust-lang/libs-team#478
- `file_absolute!`:
  - Has less meaning in other build tools like buck2
  - Bakes in the assumption that a full path is available (e.g. with
    trim-paths)
- Specifying `file!`s behavior (this PR):
  - Leaves it to the user to deal with trim-paths
  - Even though `file!` is currently unspecified, changing it would
    likely have too large of an impact on the ecosystem at this time.

A future possibility is that rustc could have a flag that controls
modifies the base path used for `file!`.
That seems purely additive with specifying the behavior and we do not
want to block on it.
It would also likely be too disruptive for Cargo users (as mentioned).
However, we tried to keep this in mind when specifying the behavior.
epage added a commit to epage/rust that referenced this issue Dec 19, 2024
This takes the current behavior of `file!` and documents it so it is
safe to make assumptions about.
For example, Cargo could provide a `CARGO_RUSTC_CURRENT_DIR` as a base
path for `file!`.

Example use cases
- Being able to look up test assets relative to the current file
  ([example](https://github.com/rust-lang/cargo/blob/b9026bf654d7fac283465e58b8b76742244ef07d/tests/testsuite/cargo_add/add_basic/mod.rs#L34))
- Inline snapshotting libraries being able to update Rust source code
  ([example](https://github.com/rust-lang/cargo/blob/b9026bf654d7fac283465e58b8b76742244ef07d/tests/testsuite/alt_registry.rs#L36-L45))

See rust-lang/cargo#3946 for more context.

T-libs-api discussed two solutions in rust-lang/libs-team#478
- `file_absolute!`:
  - Has less meaning in other build tools like buck2
  - Bakes in the assumption that a full path is available (e.g. with
    trim-paths)
- Specifying `file!`s behavior (this PR):
  - Leaves it to the user to deal with trim-paths
  - Even though `file!` is currently unspecified, changing it would
    likely have too large of an impact on the ecosystem at this time.

A future possibility is that rustc could have a flag that controls
modifies the base path used for `file!`.
That seems purely additive with specifying the behavior and we do not
want to block on it.
It would also likely be too disruptive for Cargo users (as mentioned).
However, we tried to keep this in mind when specifying the behavior.
epage added a commit to epage/rust that referenced this issue Dec 19, 2024
This takes the current behavior of `file!` and documents it so it is
safe to make assumptions about.
For example, Cargo could provide a `CARGO_RUSTC_CURRENT_DIR` as a base
path for `file!`.

Example use cases
- Being able to look up test assets relative to the current file
  ([example](https://github.com/rust-lang/cargo/blob/b9026bf654d7fac283465e58b8b76742244ef07d/tests/testsuite/cargo_add/add_basic/mod.rs#L34))
- Inline snapshotting libraries being able to update Rust source code
  ([example](https://github.com/rust-lang/cargo/blob/b9026bf654d7fac283465e58b8b76742244ef07d/tests/testsuite/alt_registry.rs#L36-L45))

See rust-lang/cargo#3946 for more context.

T-libs-api discussed two solutions in rust-lang/libs-team#478
- `file_absolute!`:
  - Has less meaning in other build tools like buck2
  - Bakes in the assumption that a full path is available (e.g. with
    trim-paths)
- Specifying `file!`s behavior (this PR):
  - Leaves it to the user to deal with trim-paths
  - Even though `file!` is currently unspecified, changing it would
    likely have too large of an impact on the ecosystem at this time.

A future possibility is that rustc could have a flag that controls
modifies the base path used for `file!`.
That seems purely additive with specifying the behavior and we do not
want to block on it.
It would also likely be too disruptive for Cargo users (as mentioned).
However, we tried to keep this in mind when specifying the behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-environment-variables Area: environment variables A-workspaces Area: workspaces C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.