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

RFC: Templating CARGO_TARGET_DIR to make it the parent of all target directories #3371

Open
wants to merge 67 commits into
base: master
Choose a base branch
from

Conversation

poliorcetics
Copy link

@poliorcetics poliorcetics commented Jan 12, 2023

Introduce a new templating option for CARGO_TARGET_DIR to tell it to move the crate/workspace's target
directory into a crate/workspace-specific subdirectory of the configured path, allowing for easy exclusion from save tools for example.

Rendered
FCP (previous FCP)

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jan 13, 2023
@ogoffart
Copy link

What I would like to see is a "global" target directory for all projects, that way, common dependency artifacts would be shared. (Not all projects would need to compile syn and other common dependencies over and over again)

I think that would solve the problems that this RFC tries to solve. But I realise this has other challenges (different features set of dependencies, when/how to clean that cache, ...)

@poliorcetics
Copy link
Author

What I would like to see is a "global" target directory for all projects, that way, common dependency artifacts would be shared. (Not all projects would need to compile syn and other common dependencies over and over again)

I think that would solve the problems that this RFC tries to solve. But I realise this has other challenges (different features set of dependencies, when/how to clean that cache, ...)

Already possible by setting CARGO_TARGET_DIR globally, and it encounters the expected problems with features and cleaning

@poliorcetics
Copy link
Author

If this is ever slated to merge I'll squash then, while the RFC is ongoing I'll make new commits to allow people to follow the evolution more easily :)

@poliorcetics
Copy link
Author

Sorry it took me a long time to answer to comments

Comment on lines +317 to +318
While unlikely, the transition period may break workflows by introducing errors for previously valid target directories.
Several alternatives exist for this:
Copy link

@sunshowers sunshowers Apr 21, 2024

Choose a reason for hiding this comment

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

It would be worth being explicit about what happens if the config option is turned on and off, or if older and newer Cargo versions are invoked with e.g. cargo +version build. And to consider the behavior both with and without forward links. For example, let's say Cargo version 1.500 supports templating:

  • Fresh build with Cargo 1.500 with option turned on
  • Fresh build with 1.500 with option turned on and then off
  • Fresh build with 1.500 with option turned off and then on
  • Fresh build with 1.500 with option turned on and then with 1.499
  • Fresh build with 1.499 and then with Rust 1.500 with option turned on
  • cargo +1.499 clean
  • cargo +1.500 clean with option turned off

Old Rust versions will be around for a while, and typical user behavior would be to set it in the global env or .cargo/config, and then forget about it.

For comparison, targo needs to be invoked once to create the symlink, and then most cargo commands work fine as usual regardless of version.

Copy link
Author

Choose a reason for hiding this comment

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

It would be worth being explicit

On the contrary, I don't think it's worth it: that would be better determined by experimentation I think, and once we have something of a consensus by testers we can finalize and write a definite table for it.

There are effectively two options:

  • Templating
  • Forwarding

The second does not matter if the first is inactive.

If templating is active it means CARGO_TARGET_DIR is set in some way, in which case the behaviour is known: cargo creates a directory as directed, so CARGO_TARGET_DIR=/tmp/{target} would create exactly /tmp/{target}, as it does today already.

I have a few rules I would like to see because they're also how cargo operates today (for the non-templating rules):

  • No CARGO_TARGET_DIR setting: work with ./target, regardless of if it's a symlink or directory, as is done today
  • Using CARGO_TARGET_DIR=/no-template, ignore ./target completely, as is done today
  • Using CARGO_TARGET_DIR=/tmp/{manifest-path-hash}
    • 1.499: ignore ./target completely, as is done today, works in exactly /tmp/{manifest-path-hash}
    • 1.500: works in /tmp/<hash path>
      • Forwarding:
        • ./target is symlink: replace it, work with it
        • ./target is not symlink: ignore it completely
      • No forwarding: ignore ./target completely

Subsequent commands where the target dir (regardless of position) already exist would just work with it, as is done today: I can build with 1.76 and 1.77 in the same repo, it will just work in the same target dir and do whatever it wants with the artifacts and such.

But all of those rules (barring those that are already in action in released versions of cargo) shouldn't be set in stone IMO, I would prefer having users coming to us to tell us what they (didn't) like about them during the unstable period.

If enough people want to see them in the RFC then I can them though, since it will likely mean there is some consensus on them already

@epage
Copy link
Contributor

epage commented May 6, 2024

@rfcbot resolve cargo-target-dir


## Definition of `{manifest-path-hash}`

In the example in the previous section, `{manifest-path-hash}` was replaced with a relative path. This relative path is computed from the full and canonicalized path to the manifest for the workspace `Cargo.toml` (or the `script.rs` file directly for cargo-scripts).
Copy link
Member

Choose a reason for hiding this comment

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

This will be the very first templating supported in .cargo/config.toml. Is the syntax flexible enough to extend to other configuration like [build.rustflags], [env]. I can see it useful in other issues like

Copy link
Author

Choose a reason for hiding this comment

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

I see no reason for it to be limited.

{name} is not used anywhere currently IIRC and the most likely places it can crop up unexpectedly are paths, where this RFC already discuss how unlikely that is.

I suppose anything where cargo shells out to another program can also have it come up, but I'm not sure we can even design for every possible target.TTT.runner in eternity

Copy link
Member

Choose a reason for hiding this comment

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

Nice. Thanks for the reply. Maybe it is worth putting this in future posibilities in some way?

Copy link
Author

Choose a reason for hiding this comment

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

Done in d549cb1 :) I tried noting possible future problems I could find in 10 minutes of thinking and why they're not limiting

@kornelski
Copy link
Contributor

Cargo has these dilemmas about the location of the target dir, because Cargo has conflated two concepts:

  1. intermediate build products (3rd party dependencies, object files, compiler-internal caches)
  2. final build artifacts (linked build targets of the current workspace only)

Users want 1 in a centralized location to deduplicate/reuse intermediate products (files they never touch). And users want 2 in ./target to run or copy the files easily, from a convenient location that has no global naming clashes.

As long as Cargo will continue to dump both kinds of products into the same directory, it will get complaints about making build results difficult to reach, or even overwriting each other, while at the same time getting requests to unify the directory.

Adding a hash to the path is not solving either of these problems. The hash keeps common dependencies built redundantly and puts the final artifacts in an inconvenient location.

@sunshowers
Copy link

Cargo has these dilemmas about the location of the target dir, because Cargo has conflated two concepts:

  1. intermediate build products (3rd party dependencies, object files, compiler-internal caches)
  2. final build artifacts (linked build targets of the current workspace only)

Users want 1 in a centralized location to deduplicate/reuse intermediate products (files they never touch). And users want 2 in ./target to run or copy the files easily, from a convenient location that has no global naming clashes.

As long as Cargo will continue to dump both kinds of products into the same directory, it will get complaints about making build results difficult to reach, or even overwriting each other, while at the same time getting requests to unify the directory.

Adding a hash to the path is not solving either of these problems. The hash keeps common dependencies built redundantly and puts the final artifacts in an inconvenient location.

This isn't a full solution to this problem, but (having used targo full time for over a year) it solves several other problems that are roughly as important.

@poliorcetics
Copy link
Author

Cargo has these dilemmas about the location of the target dir, because Cargo has conflated two concepts:

1. intermediate build products (3rd party dependencies, object files, compiler-internal caches)

2. final build artifacts (linked build targets of the current workspace only)

Users want 1 in a centralized location to deduplicate/reuse intermediate products (files they never touch). And users want 2 in ./target to run or copy the files easily, from a convenient location that has no global naming clashes.

As long as Cargo will continue to dump both kinds of products into the same directory, it will get complaints about making build results difficult to reach, or even overwriting each other, while at the same time getting requests to unify the directory.

I think you misunderstood the RFC.

Point 2. final build artifacts (linked build targets of the current workspace only) is already being worked by the out-dir/artifact-dir discussion, which is mentioned in the RFC, and out of scope for this one.

Users want 1 in a centralized location to deduplicate/reuse intermediate products (files they never touch).

That is already possible, always has been I think, just by setting a cargo-target-dir globally. The RFC discusses this and also why this can be an issue for some (e.g all the people that commented on the original issue, which is linked at the start of the RFC).

Adding a hash to the path is not solving either of these problems. The hash keeps common dependencies built redundantly and puts the final artifacts in an inconvenient location.

Because the problems you're describing are not what this RFC is proposing to solve.

What this RFC is proposing is to gather all target directories under a common "base" path while keeping them separate for each project, for the purposes of caching, cleaning local repositories, sharing build artifacts via NFS or any other usage that people will think of or have mentioned in the original issue.

This RFC proposes an opt-in option: it won't make life harder for people that don't use it, just like lots of people don't override their cargo-target-dir path and so don't have to deal with it.

Separating intermediate and final build artifacts or making them (either or) easy to access are not goals: those are either cargo issues or RFCs already.

@epage
Copy link
Contributor

epage commented Jun 21, 2024

This RFC proposes an opt-in option: it won't make life harder for people that don't use it, just like lots of people don't override their cargo-target-dir path and so don't have to deal with it.

To add to this, I want to eventually change this but I feel its dependent on finalizing --out-dir / --artifact-dir which I suspect will eventually lead to separate locations for intermediate artifacts and final artifacts. The big question to deal with is "what is a final artifact?". For most people, its just bins and dylibs. But some also consider examples, tests, etc. However, thats a topic for --artifact-dir.

@kornelski
Copy link
Contributor

kornelski commented Jun 21, 2024

What I'm trying to say that if Cargo redefined CARGO_TARGET_DIR to be only for artifacts, and not temp files or registry deps, then it would achieve the same goals motivating this RFC, but without the drawbacks of moving ./target.

To avoid hijacking this thread, I've posted: rust-lang/cargo#14125

@Ddystopia
Copy link

Ddystopia commented Jul 18, 2024

Just to clarify, would this RFC allow a use case when you compile the same project with, for example, different RUSTFLAGS alternatingly and it will not invalidate cache every time, but keep 2 of them and use appropriate?

@poliorcetics
Copy link
Author

Just to clarify, would this RFC allow a use case when you compile the same project with, for example, different RUSTFLAGS alternatingly and it will not invalidate cache every time, but keep 2 of them and use appropriate?

Nop, only the location of the cache would be affected, not its workings

@epage
Copy link
Contributor

epage commented Jul 30, 2024

@rfcbot cancel

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 30, 2024

@epage proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jul 30, 2024
@epage
Copy link
Contributor

epage commented Jul 30, 2024

@rfcbot postpone

We discussed this, rust-lang/cargo#6790, and rust-lang/cargo#14125 in the Cargo team meeting today. In that discussion, we came up with an idea that merges these different ideas, see rust-lang/cargo#14125 (comment).

However, we recognize that we don't want an "ideal" to forever block people getting functionality now and we were thinking of time boxing that effort for 1 year after which we could re-evaluate whether we should go with a "short term solution" of this RFC.

@poliorcetics thank you for all of your work on this proposal. You've put in a lot of work on this and this RFC serves as the basis for this alternative idea we are exploring. If you'd like to continue with that as an extension of this work, we'd welcome your help! I hope you don't see this as "a rejection" or "this was thrownaway".

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 30, 2024

Team member @epage has proposed to postpone this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
Status: FCP postpone
Status: Final Comment
Development

Successfully merging this pull request may close these issues.