Environment variable for Cargo Workspace

[Ygg01]

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]

when there is no workspace defined

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

[Ygg01]

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]

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]

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

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

[Ygg01]

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]

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

[Ygg01]

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

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]

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]

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

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

[alexcrichton]

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

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]

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]

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]

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]

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

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]

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]

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]

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]

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

[teohhanhui]

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]

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

• One use case is for toml-cfg (Environment variable for Cargo Workspace #3946 (comment)) which is side band communication and its unlikely we'd intentionally do feature work to support this. See also the next item.
• Accessing workspace-wide data (Environment variable for Cargo Workspace #3946 (comment)). This still hasn't been fully explored (why does it need to be workspace-wide? what are the alternatives? etc) and this breaks the assumption that packages are atomic on publish (Environment variable for Cargo Workspace #3946 (comment)). Granted, not everything gets published but we'd need to approach intentionally supporting features for unpublishable packages carefully
• Find the binary path (Environment variable for Cargo Workspace #3946 (comment)), this is also making assumptions about the target directory location (needing instead Need a reliable way to get the target dir from the build script #9661) and we do now offer some env variables for looking up binary paths, depending on the use case

[ratmice]

In my case I needed to get the path to the workspace for a .cargo/config.toml runner.
So that we could set up wasmtime so that the to allow access to all of the paths in the workspace.

It is possible to get at the workspace_root key cargo metadata, but the environment variable would be much simpler.

[epage]

@ratmice would you be able to go into more detail on this? I'm not quite sure I see how all the pieces fit together.

[ratmice]

Yes, I would be happy to,

Say we've got a .cargo/config.toml file like

[[target.wasm32-wasip2]
runner = "wasmtime run --dir ."

wasmtime sandboxes filesystem access such that the current working directory --dir . is visible from within wasmtime.

In our case we've got some tests that want to read data files within various crates the repository, these tests are in crates which exist in the workspace, but they aren't crates that get published to crates.io.

When run on cargo test --target wasm32-wasip2 because we only specified --dir . in the runner.
These tests can't actually read outside of their working directory.

So I had to write something https://github.com/ratmice/workspace_time_test_runner
which gets the workspace_root, and figures out the relative path between the CARGO_MANIFEST_DIR
and the workspace root. Then sets up all of the --dir flags between the two (it also keeps wasmtime from sandboxing cargo environment variables).

FWIW, when I wrote my comment some of the most important comments about why the patches for this weren't accepted were hidden. Particularly boats comment here #3946 (comment)

[epage]

So let's see if I can re-phrase this.

You have target-runner that you want to be able to pass it the path to the entire workspace but using --dir . is being evaluated within CARGO_MANIFEST_DIR and not a workspace-root, so not enough content is made accessible. This is so you can access data spread throughout your workspace.

To make sure we have the complete picture, could you provide the motivation for why the files are not consolidated with the tests? Also, could you use symlinks or are you supporting the Windows without symlinks or wasmtime won't allow resolving of the symlinks?

boats' comment deals with published packages and you had said that yours aren't published. If it wasn't clear from the other comments you saw, in designing a solution, we have to consider that it "blesses" a workflow and people will use it if they discover it. We might document there are problems with using it with published packages but documentation tends to be a last resort way of solving a problem as people are unlikely to notice it or even heed it until its too late. I will say that some day we will need to grapple with the needs of unpublished packages that would be frowned upon within the context of publishing. I suspect #6153 will be a blocker for that.

[ratmice]

Your first paragraph sounds spot on.

The additional motivation is that we have a small parser and file format that is shared between multiple crates.
Say we have crate A and crate B, and B depends on A, Crates 'A', and 'B' are both published.
Then we parse some data with A, and that same data gets parsed again with B, It isn't actually using serde but we can invent the same situation using that.

// Crate A only knows about `some_key`
#[serde(Deserialize)]
struct Foo {
   some_key,
   // When we deserialize we ignore unknown keys by default.
}

// Crate B
#[serde(Deserialize)]
struct Foo {
   some_key,
   // Crate A ignores some_other_key, but Crate B deserializes it...
   some_other_key: SomeTypeFromCrateB
}

What we're using this for is testing that all the data in our workspace can be deserialized by all crates involved,
even when portions of the data are unknown by the crate and must be ignored. So testing cross-crate consistency.
This is a separate crate which is not published. It doesn't know about any specific keys, values, or types it just provides the most specification of the serialization format, and the format keys/values must have, and checks all keys/values against that specification for all crates.

This specific unpublished crate now depends upon crate B, not just because it reads crate B data, but because crate B provides the format that the specification is written in. So crate A cannot use the specification without introducing a circular dependency.

And indeed we would like to avoid symlinks due to windows, I don't know if wasmtime would resolve them but they would be an issue on other targets.

Regarding boats comment, I was just saying I now see why allowing cargo metadata to export workspace_root isn't a problem where an environment variable is much more of a problem for build.rs (I mean I guess you could go out of your way to invoke cargo metadata from build.rs, and shoot ones self in the foot?).

It is a somewhat complex situation so I hope I have conveyed it well.

Edit:

To make sure we have the complete picture, could you provide the motivation for why the files are not consolidated with the tests?

To hopefully respond to this a little more succinctly, the files are not consolidated with the tests for two reasons:
The tests build-depend on the library which gets called from build.rs, and the files are largely spread out across examples/*/src/*.xyz So it performs additional checks on all the code generation happening in all the examples.

The only other thing I should say is there is a disconnect between what I actually need (the package_root) and what I can get (the workspace_root). I only actually need the package_root but the closest anchor points to the package root that I can get is the workspace root.