Adding Workspace DIR functionality #4073
Conversation
- Adds ability to get directory containing Workspace manifest - Adds test for the functionality
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| /// That is, this returns the path of the directory containing the | ||
| /// `Cargo.toml` which is the root of this workspace. | ||
| pub fn workspace_dir(&self) -> Option<&Path> { | ||
| match self.root_manifest { |
There was a problem hiding this comment.
@alexcrichton I know you said use root, but I wanted it to be option so I can check if the crate has a workspace.
There was a problem hiding this comment.
I think this should always be available, right? In that this function should return &Path instead of Option<&Path>? (sort of how root above cannot fail)
There was a problem hiding this comment.
I thought @shepmaster said in #3946, that it's best it's not present if it's not a cargo workspace project?
There was a problem hiding this comment.
Hm so in some sense all crates are a member of a workspace regardless of configuration, it's just that some workspaces only contain one member. My feeling is that code wishing to use this would basically othrewise be getting this env var unwarp_or CARGO_MANIFEST_DIR.
@shepmaster mind weighing in though? Do you have thoughts on what you'd do if a build script isn't in a workspace?
There was a problem hiding this comment.
Hm, there doesn't seem to be enough interest for this feature. I managed to bypass it for my needs.
I am willing to work on it this weekend.
There was a problem hiding this comment.
Ok, let's switch this to returning &Path and always exporting it to the build script?
| use std::env; | ||
|
|
||
| fn main() { | ||
| assert!(env::var("CARGO_WORKSPACE_DIR").unwrap().ends_with("foo")); |
There was a problem hiding this comment.
Is there a better way to test it is looking at correct folder?
There was a problem hiding this comment.
Yeah you could set a different env var with the expected value as part of p.cargo below perhaps?
There was a problem hiding this comment.
Are there any examples of that in tests? Ideally, I want to say, set this to be wherever the parent dir of this file is.
EDIT: Clarification, CARGO_WORKSPACE_DIR should point to folder where Cargo.toml that defines members is located (where applicable).
There was a problem hiding this comment.
Oh you'd just do something like p.cargo("foo").env("EXPECTED_DIR", "...")
There was a problem hiding this comment.
I was more asking, how to get "..." to point to correct folder. As far as I understand, cargotest creates temp files ?
EDIT: Would this https://github.com/rust-lang/cargo/blob/master/tests/cargotest/support/paths.rs#L49
There was a problem hiding this comment.
Oh p.root() points to the root path of the project, and you can use that as a PathBuf to calculate the correct value to pass in
|
Closing due to inactivity, but feel free to resubmit with a rebase! |
This is the basic code I did to get #3946 fixed.
The functionality is about allowing
build.rscustom build script to find Workspace directory, so it could extract some data, etc. It's possible, it makes sense for this environment variable to be present even during regularcargo build.Workspace directory variable is called
CARGO_WORKSPACE_DIRand points to workspace directory (one withCargo.tomlthat declares members), regardless of which crate from workspace called it. If the crate built doesn't have a workspace there will be no value inCARGO_WORKSPACE_DIR, i.e.env!("CARGO_WORKSPACE_DIR")will beErr(NotPresent).