Added build.build-dir support
#29
Conversation
ranger-ross
force-pushed
the
build-dir-support
branch
from
8fb19c7 to
b31e2c7
Compare
|
Thanks! The implementation looks good. However, given that this is an unstable feature and could lead to errors if the accepted syntax changes in the future, it might be better to put it behind a feature flag. |
|
Sure, perhaps we add a generic |
|
That sounds like a good idea. I'm usually against using the Cargo feature for unstable stuff (rust-lang/api-guidelines#284 (comment)), but in this case I think the Cargo feature will be fine since the only users affected are those who explicitly use the unstable feature in their config files. |
ranger-ross
force-pushed
the
build-dir-support
branch
2 times, most recently
from
fd59ba4 to
4755153
Compare
|
I added an |
|
Thanks. That looks good to me. I looked at the documentation again and it looks like the environment variable is supported, so could you update src/env.rs to handle that? https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#build-dir
|
ranger-ross
force-pushed
the
build-dir-support
branch
from
4755153 to
e425056
Compare
|
Added Regarding the failing CI jobs, seems unrelated. I pulled the latest nightly and ran |
src/easy.rs
Outdated
| }); | ||
| let target_dir = de.target_dir.map(|v| v.resolve_as_path(current_dir).into_owned()); | ||
| #[cfg(feature = "unstable")] | ||
| let build_dir = de.build_dir.map(|v| v.resolve_as_path(current_dir).into_owned()); |
There was a problem hiding this comment.
Hmm... given the presence of templates, I wonder if there is a problem with unconditional invocation of resolve_as_path here.
Cargo Reference says:
This option supports path templating.
Available template variables:
{workspace-root}resolves to root of the current workspace.{cargo-cache-home}resolves toCARGO_HOME{workspace-path-hash}resolves to a hash of the manifest path
Perhaps resolve_as_path should be skipped if the path starts with {workspace-root} or {cargo-cache-home}.
Also, it would be nice to mention in the documentation that these templates are not resolved.
There was a problem hiding this comment.
yes, agreed.
Added handling for that in the latest commit.
ranger-ross
force-pushed
the
build-dir-support
branch
2 times, most recently
from
8629e9c to
e2befe7
Compare
src/easy.rs
Outdated
| let build_dir = de.build_dir.map(|v| { | ||
| if v.val.starts_with("{workspace-root}") | ||
| || v.val.starts_with("{cargo-cache-home}") | ||
| || v.val.contains("{workspace-path-hash}") |
There was a problem hiding this comment.
I don't think we need to care about {workspace-path-hash} since it doesn't resolve to an absolute path.
There was a problem hiding this comment.
thanks, removed!
|
Could you address the review (#29 (review)) and remove the unstable stuff (since build-dir is now stabilized)? |
ranger-ross
force-pushed
the
build-dir-support
branch
from
e2befe7 to
099d048
Compare
ranger-ross
force-pushed
the
build-dir-support
branch
from
099d048 to
920999f
Compare
|
Sorry for the late follow up on this! Addressed your comment and pushed another commit to stabilize |
|
Published in 0.1.37. |
2 participants
This PR adds
build_dirto theBuildConfigSee: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#build-dir
closes #28