-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh you'd just do something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was more asking, how to get EDIT: Would this https://github.com/rust-lang/cargo/blob/master/tests/cargotest/support/paths.rs#L49 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh |
||
} | ||
"#); | ||
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)); | ||
} | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 ofOption<&Path>
? (sort of howroot
above cannot fail)There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?