Add CARGO_WORKSPACE_DIR env variable.

[withoutboats]

This variable refers to the workspace this package is inside of.

[alexcrichton]

Weird I thought we already did this! I think I was thinking of #3946 which I think just never got landed?

I'll take a look!

[alexcrichton]

The idea seems fine by me, although perhaps we could store the workspace root directly in a Compilation when created to avoid passing it in again? Other than that with a test r=me

[withoutboats]

Added this to the existing env var checks.

Looked into adding the path to the compilation type but how the compilation type is built confused me.

[matklad]

@withoutboats what are the use-cases for WORKSPACE_DIR? I am afraid there are some edge-cases which might make this variable less useful than it seems.

Mainly, workspaces are not restricted to a single directory, you can refer to arbitrary paths from [workspace.members]. So, while naively one might think that everything will be under WORKSPACE_DIR, it's not actually the case.

I think using ROOT_MANIFEST[_DIR] as a name for this variable might be a better choice. Or perhaps we should just carefully document the caveats?

[withoutboats]

The directory contains the root manifest as well as the lockfile, both of which may be useful for codegen purposes.

[matklad]

Oh, and we do need docs for this (there's environmental variables section) =P

[withoutboats]

The docs are in the crates.io repo, right (not sure how to handle the stable/nightly aspect of the docs, but I assume there's a policy in place)?

[matklad]

The docs are in the crates.io repo, right?

Uhm, I am not sure. At least, there's still cargo/src/doc and cargo/src/book folders in this repository.

[matklad]

One more potential gotcha is the difference in building the crate locally vs building a crate as a dependency from crates.io.

I.e, if you build a crate foo in a local workspace, you'll have WORKSPACE_DIR, but, if you then upload foo and other crates from the workspace to crates.io, and then try to build unrelated crate bar, which depends on foo = 1.0.0 then you'll be building foo outside of the workspace, because workspaces do not exist for published crates.

[matklad]

Also cc #4764 which might provide an alternative solution to the problem at hand: at the codegen time, you can call cargo metadata, which gives you information about the workspace as well as a parsed lockfile. I am not sure about exposing lockfile directly to tools, because, at least in my mind, the lockfile format is an implementation detail, and cargo metadata is a public API.

Does this all make sense? I certainly understand the need to know more about the package being build, but I am not sure what is the most reliable and dependable way to give this information :)

[alexcrichton]

Oh excellent points @matklad!

The docs in src/doc in this repo should be updated in this PR. I sort of forget how they're deployed but we can figure that out later if it doesn't work.

I wonder if we should also only export this env var for members themselves of the workspace? That I think would solve the crates.io problem as well. I'm also slightly wary of giving crates.io crates access to the main directory of the build...

[matklad]

I wonder if we should also only export this env var for members themselves of the workspace? That I think would solve the crates.io problem as well.

That means that building a crate locally and building crate as a crates.io dependency does different things. I imagine the following situation:

• Alice dreams of a great new Rust project
• Alice creates the project, consisting of a family of libraries inside a worksapce
• Alice writes a custom build.rs which looks at WORKSPACE_DIR and does stuff
• Alice successfully publishes all the packages of the workspace
• Bob, impressed by Alice's work, adds her project as a dependency, but gets an obscure panic from ::std::os::env::var().unwrap() from the dependency.

Not sure how likely a similar scenario is likely to arise, but the result looks pretty bad to me. The key thing is that we can't say that something is going wrong until the very last moment.

I think it's fine to have WORKSPACE_DIR (with a less ambitious name) for applications, and not for libraries, but I am not sure how we can do this distinction? Perhaps, prepublish checks should execute a cargo build without WORKSPACE_DIR?

[alexcrichton]

Hm good point! We could definitely make cargo publish not actually set the env var, but otherwise our distinction of "app" and "lib" today is mostly related to "are you a path dependency or not" so something like cargo test on a lib makes it look like an app.

Hm... Unsure :(

[withoutboats]

My read of the code was that this was the same as CARGO_MANIFEST_DIR if you aren't in a workspace; is that not always the case?

More generally @matklad's point about edge cases is pretty good and #4764 might be a better approach.

[alexcrichton]

@withoutboats I think it's not the same as CARGO_MANIFEST_DIR if you're not a member of the workspace. For example crates.io crates have a CARGO_MANIFEST_DIR pointing in ~/.cargo/registry/... but I think CARGO_WORKSPACE_DIR would point to the cwd?

[withoutboats]

It seems like we should have a larger convo about motivations.

My personal goal was to be able to consistently read the lockfile to be able to gain info about dependencies. @matklad mentions that the lockfile format is an implementation detail, but we have certain compatibility guarantees around it - definitely backwards compatibility and probably a certain amount of forward compatibility if you don't use any new features. I would describe the lockfile format as stable.

However, likely it is better to just run cargo metadata and parse the output of that process. In that case, I'm not sure what the benefit of having this variable is, and possibly this PR should be closed because of the sharp edges @matklad has identified. But does that raise some of the same issues about being built as a library vs being built as the root? What is the correct way to invoke cargo metadata from a build script? Is this sufficient?:

let cargo = env::var_os("CARGO").unwrap();
let dir = env::var_os("CARGO_MANIFEST_DIR").unwrap();
let cmd = Command(cargo).arg("metadata").current_dir(dir);

[withoutboats]

Maybe this is straying off topic, but more generally, should the cargo metadata output to be expanded to include everything in both Cargo.toml and Cargo.lock, and then we expect tooling that wants to know about the state of the manifest information to parse cargo metadata and not open either of those files?

[matklad]

My personal goal was to be able to consistently read the lockfile to be able to gain info about dependencies.

Which kind of dependency information exactly? There's dependencies declared in your crate (SemverReqs parsed from Cargo.toml), dependencies declared in the downstream crate, and the actual, resolved dependencies of the downstream crate (from Cargo.lock/cargo metadata).

This sounds a bit like "an upstream dependency wants to know resolved dependencies of the downstream crate and make some decisions based on it", which is not too comfortable. Especially, upstream inquiring about downstream seems philosophically wrong?

What is the correct way to invoke cargo metadata from a build script? Is this sufficient?

It would be great to know which way is correct, nobody specifically designed for this use case :) I think the proposed snippet has a problem though: cargo metadata writes a Cargo.lock file, unless the --no-deps flag is passed as well. CARGO_MANIFEST_DIR may point to a directory inside ~/.cargo/, so this will mutate supposedly readonly crates store... An you probably won't get the result you want, because you'll get a fresh lockfile for this dependency, not for the workspace.

So, I think getting resolved dependencies is not possible in a nice way, because this info is available only downstream. Getting declared dependencies should be OK, but probably less useful.

Maybe this is straying off topic, but more generally, should the cargo metadata output to be expanded to include everything in both Cargo.toml and Cargo.lock, and then we expect tooling that wants to know about the state of the manifest information to parse cargo metadata and not open either of those files?

I think this is more or less the case now. cargo metadata --no-deps gives you Cargo.toml, cargo metadata gives you .toml + .lock. If something is missing, we can surely add it!

I would describe the lockfile format as stable.

Lockfiles generated by Cagro n are not guaranteed to be readable by Cargo n - k, where k is sufficiently large. For example, we've recently stopped generating the meaningless [root] section from Cargo.lock.

[withoutboats]

I'm just going to close this because of the edge cases and questions involved. Seems best to punt for now. Thanks for the review though @matklad and @alexcrichton :)