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
| @@ -564,6 +571,8 @@ impl BuildConfig { | |||
| .collect() | |||
| }); | |||
| 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
from
e425056 to
8629e9c
Compare
ranger-ross
force-pushed
the
build-dir-support
branch
from
8629e9c to
e2befe7
Compare
| 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.
This PR adds
build_dirto theBuildConfigSee: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#build-dir
closes #28