rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.66k stars 2.41k forks source link

Environment variable for Cargo Workspace #3946

Open Ygg01 opened 7 years ago

Ygg01 commented 7 years ago

T-cargo notes:

A CARGO_RUSTC_CURRENT_DIR is added as a nightly only environment variable. See https://github.com/rust-lang/cargo/issues/3946#issuecomment-1832514876. 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 commented 7 years ago

when there is no workspace defined

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

Ygg01 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

Ygg01 commented 7 years ago

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 commented 7 years ago

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

Ygg01 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

alexcrichton commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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.

withoutboats commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

peterhuene commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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

folex commented 3 years ago

Seems like calling cargo metadata is the only way currently. Basically what @mitsuhiko done above is

  1. Call cargo metadata --format-version=1
  2. Parsing it as JSON
  3. Retrieving field workspace_root

So, in a ~nut~shell that would look like

cargo metadata --format-version=1 | jq .workspace_root

I'm a bit concerned about output size of cargo metadata though. It's ~1.6MB on my project:

$ cargo metadata --format-version=1 | wc -c
 1717575
mitsuhiko commented 3 years ago

This lack of envvar also came back to my mine when investigating this regression: https://github.com/rust-lang/cargo/issues/8992

Ygg01 commented 3 years ago

So should we just make CARGO_WORKSPACE_DIR be convenience for workspace_root from cargo metadata --format-version=1?

ghost commented 3 years ago

Seems like calling cargo metadata is the only way currently.

I think this also works:

dirname "$(cargo locate-project --workspace --message-format plain)"

The --workspace flag is from #8712.

I'm a bit concerned about output size of cargo metadata though. It's ~1.6MB on my project:

$ cargo metadata --format-version=1 | wc -c
 1717575

cargo locate-project --workspace --message-format plain has the benefit that it prints only the path in plain text format (JSON format can be printed by removing --message-format plain), without additional redundant informations.

pksunkara commented 3 years ago

I think people understand that they can't rely on CARGO_WORKSPACE_DIR in build.rs because of the reason specified above. But there are lot of other scenarios other than build.rs where people want this env var. Would be nice if one of the PRs get accepted and in documentation, we note that using it in build.rs would not work.

wycats commented 3 years ago

Is there any reason that an environment variable for workspace dir never happened? It seems pretty aligned with the rest of the environment variable and is already exposed via the CLI.

RandomInsano commented 2 years ago

Here's a bit of a silly workaround. Create a '.cargo/config.toml' file at the root of your workspace and set this as an environment variable:

[env]
CARGO_WORKSPACE_DIR = { value = "", relative = true }

I don't think this is a good excuse to not have this built in, but at least it's a stop-gap for those who need it. This only works because of the "relative = true" which prepends the path that the ".config" folder is in to the beginning of the variable.

Veykril commented 2 years ago

Another use case for this is the expect-test crate which has problems in some set ups without this env var: https://github.com/rust-analyzer/expect-test/issues/33

jjant commented 1 year ago

Any updates on this?

epage commented 1 year ago

We discussed this today in the cargo team meeting.

tl;dr our starting proposal for this is: A new variable, possibly named CARGO_WORKSPACE_DIR, would be set for test/bench targets which would help people open paths based on file!, likely starting off as relative but the exact format is undefiend atm. This can start off as a nightly-only PR for us to get a better idea of what we think and then we can evaluate and plan next steps accordingly.

There was the question of whether this is for the "workspace" of what is being built now or the final artifact. We felt that it would best be associated with what is being built right now, either exposing macros for dependents to use or have them pass paths down into what is being compiled. That has clearer semantics and doesn't run into inverted dependencies where the crate knows what it is being compiled fir. This means that for the registry crates, the path would end up being the same as CARGO_MANIFEST_DIR.

Someone proposed a theory that making this variable relative would help convey its changeable nature on publish, so we might explore this being a relative path instead of an absolute path.

We were also concerned about making failure cases easier by making this variable available to production code for people accessing files outside of CARGO_MANIFEST_DIR which will inaccessible on publish. We felt that we should start this off as a test/bench only variable, like CARGO_TARGET_TMPDIR, but that we can consider expanding the scope in the future based on feedback and use cases.

One concern raised is that this would primarily be used to make std:file! accessible from current_dir for opening but file! is not fully defined as its intended for debug purposes. This is another benefit to starting life as test/bench only as it lowers the barrier for abusing file!.

The remaining question is on what to name this variable. Does CARGO_WORKSPACE_DIR convey

david-mcgillicuddy-moixa commented 1 year ago

It's funny but we worked around this by using concat!(env!(CARGO_MANIFEST_DIR), "/..") in our use case where we invoked cargo run -p foo on our own binary (from a different package in the workspace) from a test, which caused cargo to look up in the parents for a toml, find the workspace toml, and then -p works as expected.

Just using env!(CARGO_MANIFEST_DIR) as our env failed because it found the package toml and we didn't get proper workspace package resolution for cargo run -p foo to work. Having just seen RandomInsano's workaround that's way better.

mimoo commented 1 year ago

needed this today again : D

yerke commented 1 year ago

@rustbot claim

epage commented 1 year ago

@yerke in case its useful I hold office hours: https://github.com/rust-lang/cargo/wiki/Office-Hours

epage commented 1 year ago

While there might be multiple ways to test this, since a major use case for this is to work with file!, we should probably make sure that use case works.

We can test file! by having the test define a workspace with a package inside of that workspace. In that package, we have a test that uses the file! macro to assert that a file exists. Before this feature, that would work by manually specifying how to make the path valid. After this feature, its just a Path::join

Nick-Mazuk commented 1 year ago

FYI for those that need this behavior now, I found this crate that gets the current workspace path.

https://crates.io/crates/project-root

Granted an environment variable would be more performance since this crate traverses your file system looking for the nearest Cargo.lock file.

weihanglo commented 1 year ago

Granted an environment variable would be more performance since this crate traverses your file system looking for the nearest Cargo.lock file.

This approach not always correct, as a Cargo.toml can be included/excluded either by itself or from the [workspace] section.

There is a pull request getting stale https://github.com/rust-lang/cargo/pull/12158. Not sure about if they've got time working on it again, but I guess it pretty acceptable for someone informing them and take over.

epage commented 10 months ago

Looking back over use cases. Unfortunately, there aren't actually that much in the way of details

When the cargo team discussed this (https://github.com/rust-lang/cargo/issues/3946#issuecomment-1402464796) it was mostly focused on file!().

epage commented 10 months ago

12996 provided an alternative to CARGO_WORKSPACE_DIR called CARGO_RUSTC_CURRENT_DIR which, in theory, should be exactly what file! is directly relative to in all cases, rather than indirectly.

rust-lang/rust was updated with this change in rust-lang/rust#118275 (24 Nov 2023) so within a day or two after that, it should be in a nightly.

Would some of the snapshot testing people be interested in trying this out on nightly and providing feedback to help towards stabilization?

For other uses, we are continuing to punt, see https://github.com/rust-lang/cargo/issues/3946#issuecomment-359619839

lu-zero commented 8 months ago

Another use-case for it is with tools like https://github.com/jamesmunns/toml-cfg since you would like to find the cfg file at the root of the workspace and there is the additional annoyance of having to find the information from a proc_macro.

epage commented 8 months ago

@lu-zero so it sounds like that is a case of wanting access to the end-users workspace and not to your own package's workspace.

I think we had expressed concern in the past about being able to change your build with side band information like that. With https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618, I had considered the idea of allowing something like that though.

lu-zero commented 8 months ago

For my specific needs, putting a cfg.toml in the root of the workspace is already good. It makes easier to share the same configuration among multiple example-crates that might be radically different (no_std vs std-ish esp targets)

epage commented 6 months ago

For the snapshot testing side of this (ie using std::file!), I've posted #13644 for stabilizing CARGO_RUSTC_CURRENT_DIR.

Nemo157 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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"