Skip to content

Added build.build_dir templating support #15236

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

Merged
merged 4 commits into from
Mar 17, 2025

Conversation

ranger-ross
Copy link
Contributor

@ranger-ross ranger-ross commented Feb 26, 2025

What does this PR try to resolve?

This PR is a follow up on #15104 and and adds support for the path templating in build.build-dir as defined in #14125.

Supported templates:

  • {workspace-root}
  • {cargo-cache} (pointing to CARGO_HOME for now)
  • {workspace-manifest-path-hash}

Unresolved questions

What should we name {workspace-manifest-path-hash} and what should it include? Should we shorten to {workspace-hash} or even just {hash}? Should we include the Cargo version so we get unique whole-target directories for easier cleanup (#13136)

How should this handle unknown variables (error) or unclosed { / } (ignored), see #15236 (comment)

When using {workspace-manifest-path-hash} this hash will change based on the project path. In the event of a cargo being executed in a symlinked project, the hash will change.

For example, given the following directory

/Users/
└─ user1/
    └─ projects/
        ├─ actual-crate/
        │  └─ Cargo.toml
        └─ symlink-to-crate/ -> actual-crate/

the hash will be unique when running cargo from each of the following directories.

  • /Users/user1/actual-crate
  • /Users/user1/symlink-to-crate

Figuring out whether to handle this is deferred out, see

How should we test and review this PR?

This PR is fairly small. I included tests for each template variable.

You can also clone my branch and test it locally with

CARGO_BUILD_BUILD_DIR='{workspace-root}/foo' cargo -Z build-dir build

Additional information

While searching Cargo for any prior art for path templating, I found sources/registry/download.rs doing a simple string replace. Thus I followed the same behavior.

r? @epage

@rustbot rustbot added A-configuration Area: cargo config files and env vars A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2025
@epage epage mentioned this pull request Feb 26, 2025
@ranger-ross
Copy link
Contributor Author

Another question: Should Cargo error if the user adds something like

build-dir =  "/foo/{workspace-root}/bar"

This is invalid as {workspace-root} points to a specific directory so prefixes don't make logical sense.
The same is true for cargo-cache but not {workspace-manifest-path-hash} since the hash is not pointing to a specific directory. It's merely part of some path.

In my opinion it would be best to error on an invalid path.
Although another approach would be to simply ignore the prefix.

@epage
Copy link
Contributor

epage commented Feb 27, 2025

Another question: Should Cargo error if the user adds something like

On Linux systems, that is valid and I could see someone doing that, rather than using a hash.

On Windows systems, that will be a mess.

I would lean towards not doing anything special wrt and instead we can note this in the tracking issue for people to be aware of this when we stabilize.

Comment on lines 45 to 51
let mut value = self.0.val.clone();

for (from, to) in replacements {
value = value.replace(from.as_ref(), to.as_ref());
}

self.0.definition.root(gctx).join(&value)
Copy link
Contributor

Choose a reason for hiding this comment

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

What should this do on undefined variables?

Copy link
Contributor Author

@ranger-ross ranger-ross Mar 8, 2025

Choose a reason for hiding this comment

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

I gave this some thought and I am of the opinion that we should ignore them. (at least for now)

On unix system's its possible to create paths with curly braces. (not sure about Windows)

> mkdir 'foo/{bar}/baz'
> ls foo
{bar}

Soo the following would be legal

[build]
build-dir = "{workspace-root}/foo/{bar}/baz"

Although this is probably a very niche usecase I think it would be better to allow.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is if we add a new variable, what will be the experience on older Cargo? Is it better to lossily succeed or fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, good point. Perhaps it's better to start out strict to avoid potential for breaking changes.

If there is a demand for curly braces in paths, we could probably add the ability escape them in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking: this hasn't been put in yet, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I should have time to add it this weekend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unexpected variable validation in 707d637

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are already making changes, when I added this as a stabilization question in the PR description, I also mentioned unmatched { / } (and forgot to mention it here).

I'll leave the decision up to you of whether you want to check for that and we can always re-evaluate on the way to stabilization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about that while implementing the last commit.
I am inclined to ignore unterminated brackets for now since they can technically be valid file paths on unix.

But I agree we should re-evaluate before stabilizing.

I think that it will come down to whether we consider { and } in paths a valid usecase.
I'd be perfectly happy to say "No, you cannot use brackets in file paths in templated path configs." (It would simplify validation to a simple path.contains("{") || path.contains("}") after resolving all known template variables)
But the Cargo team members might have different opinions this

@ranger-ross ranger-ross force-pushed the build-dir-templating branch from 80b8345 to b7469f8 Compare March 9, 2025 14:17
@rustbot rustbot added the A-testing-cargo-itself Area: cargo's tests label Mar 9, 2025
This is in preparation for adding templating suppport to
the `build.build-dir` configuration option.
@ranger-ross ranger-ross force-pushed the build-dir-templating branch from b7469f8 to ba4df9f Compare March 14, 2025 11:13
This commit adds logic to check for unexpected variables in templated
paths like `build.build-dir`. Cargo will error if it finds a variable
that it does not know how to expand.
@ranger-ross ranger-ross force-pushed the build-dir-templating branch from 707d637 to 229489e Compare March 15, 2025 08:25
@epage epage added this pull request to the merge queue Mar 17, 2025
@epage
Copy link
Contributor

epage commented Mar 17, 2025

Thanks!

Merged via the queue into rust-lang:master with commit 68c26f6 Mar 17, 2025
21 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2025
Update cargo

14 commits in 6cf8267012570f63d6b86e85a2ae5627de52df9e..307cbfda3119f06600e43cd38283f4a746fe1f8b
2025-03-14 15:25:36 +0000 to 2025-03-20 20:00:39 +0000
- feat: Add custom completer for cargo <TAB> to complete aliases defined in config.toml (rust-lang/cargo#15319)
- fix(build-dir): Renamed workspace-manifest-path-hash to workspace-path-hash (rust-lang/cargo#15334)
- feat: vcs, color, and message format native completion (rust-lang/cargo#15322)
- Fix `[env]` `relative` description in reference (rust-lang/cargo#15332)
- chore: fix some typos (rust-lang/cargo#15329)
- Cleanup for rustc-link-arg-cdylib (rust-lang/cargo#15326)
- fix(toml): Report '<target>.edition' deprecation to users (rust-lang/cargo#15321)
- test(build-std): address overly-matched snapshot (rust-lang/cargo#15325)
- Added `build.build_dir` templating support (rust-lang/cargo#15236)
- docs: make it clearer that `rust_version` is enforced during compile (rust-lang/cargo#15303)
- feat: Add custom completer for cargo +<TAB> to complete toolchain name (rust-lang/cargo#15301)
- chore: fix some typos (rust-lang/cargo#15316)
- fix: deduplicate crate types in cargo rustc command (rust-lang/cargo#15314)
- docs: mention wrong URLs as a cause of git authentication errors (rust-lang/cargo#15304)

r? ghost
@rustbot rustbot added this to the 1.87.0 milestone Mar 22, 2025
@ranger-ross ranger-ross deleted the build-dir-templating branch April 29, 2025 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation A-testing-cargo-itself Area: cargo's tests A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants