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

Adding Workspace DIR functionality #4073

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,17 @@ impl<'cfg> Workspace<'cfg> {
}.parent().unwrap()
}

/// Returns the root path of this workspace.
///
/// 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 {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcrichton I know you said use root, but I wanted it to be option so I can check if the crate has a workspace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought @shepmaster said in #3946, that it's best it's not present if it's not a cargo workspace project?

Copy link
Member

@alexcrichton alexcrichton May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's switch this to returning &Path and always exporting it to the build script?

Some(ref p) => p.parent(),
None => None
}
}

pub fn target_dir(&self) -> Filesystem {
self.target_dir.clone().unwrap_or_else(|| {
Filesystem::new(self.root().join("target"))
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/ops/cargo_rustc/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
cmd.env("CARGO_MANIFEST_LINKS", links);
}

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

// Be sure to pass along all enabled features for this package, this is the
// last piece of statically known information that we have.
for feat in cx.resolve.features(unit.pkg.package_id()).iter() {
Expand Down
2 changes: 2 additions & 0 deletions src/doc/environment-variables.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ let out_dir = env::var("OUT_DIR").unwrap();
script). Also note that this is the value of the
current working directory of the build script when it
starts.
* `CARGO_WORKSPACE_DIR` - The directory containing the manifest of the workspace,
for the package being built.
* `CARGO_MANIFEST_LINKS` - the manifest `links` value.
* `CARGO_FEATURE_<name>` - For each activated feature of the package being
built, this environment variable will be present
Expand Down
69 changes: 69 additions & 0 deletions tests/workspaces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1469,3 +1469,72 @@ Caused by:
"));
}

fn missing_workspace_build_dir(){
let p = project("foo")
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.5.0"
authors = []
"#)
.file("src/lib.rs", "")
.file("build.rs", r#"
fn main() {
use std::env;
use std::env::VarError::NotPresent;

assert_eq!(env::var("CARGO_WORSPACE_DIR"), Err(NotPresent));
}
"#);
p.build();

assert_that(p.cargo("build"),
execs().with_status(0));
}

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


[workspace]
members = ["bar"]
"#)
.file("src/lib.rs", "")
.file("build.rs", r#"
use std::env;

fn main() {
assert!(env::var("CARGO_WORKSPACE_DIR").unwrap().ends_with("foo"));
}
"#)
.file("bar/Cargo.toml", r#"
[project]
name = "bar"
version = "0.5.0"
authors = []
links = "bar"
build = "build.rs"
"#)
.file("bar/src/lib.rs", "")
.file("bar/build.rs", r#"
use std::env;

fn main() {
assert!(env::var("CARGO_WORKSPACE_DIR").unwrap().ends_with("foo"));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to test it is looking at correct folder?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you could set a different env var with the expected value as part of p.cargo below perhaps?

Copy link
Author

@Ygg01 Ygg01 May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you'd just do something like p.cargo("foo").env("EXPECTED_DIR", "...")

Copy link
Author

@Ygg01 Ygg01 May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
"#);
p.build();

assert_that(p.cargo("build"),
execs().with_status(0));
assert_that(&p.root().join("target"), existing_dir());
assert_that(p.cargo("build").cwd(p.root().join("bar")),
execs().with_status(0));
}