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

RFE: expose target_dir as an environment variable to 3rd party subcommands #9804

Closed
Firstyear opened this issue Aug 18, 2021 · 7 comments · Fixed by #9836
Closed

RFE: expose target_dir as an environment variable to 3rd party subcommands #9804

Firstyear opened this issue Aug 18, 2021 · 7 comments · Fixed by #9836
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@Firstyear
Copy link
Contributor

Describe the problem you are trying to solve
A number of 3rd party subcommands exist that provide extended behaviours such as rinstall or cargo-native-install which can install content built from the target-dir into the host system in various places or extended ways. Today, these have to infer target-dir either from a CLI switch, or attempt to recreate cargo's interpretation of CARGO_TARGET_DIR and the various .cargo/config files that may exist.

Describe the solution you'd like
When running 3rd party subcommands, cargo should expose an environment variable to these which contains the processed target-dir allowing inheritance of cargos processing of this variable.

@Firstyear Firstyear added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Aug 18, 2021
@alexcrichton
Copy link
Member

External subcommands are intended to be somewhat black-boxes to Cargo. Cargo doesn't impose much of a particular shape over the subcommand, and the subcommand is expected to use its own arguments to figure out what to do. For example there isn't really a target directory associated with any external subcommand in the same way that cargo yank doesn't really have a target directory.

I don't think that this is something that can be implemented as-is and otherwise subcommands need to invoke things like cargo metadata to learn about a specific workspace they might be interested in.

@Firstyear
Copy link
Contributor Author

Hey there, thanks for the response.

I understand that while cargo doesn't impose a shape to the subcommand and will treat them as opaque entries, those subcommands still are being added as subcommands to cargo due to being rust-project adjacent. Else they would just be seperate commands and wouldn't need cargo to invoke them! They must have a relationship to cargo and rust projects in some way.

For these 3rd party commands I think this change still leaves them as "opaque" - cargo still isn't caring what they do or how they operate. But what is important here is that by cargo exposing some of it's knowledge of the project we may be in or the environment in which cargo operates, it allows the subcommands to perform better and potentially closer to what cargo intends in the existence of subcommands (extensions). In a way this becomes a change to improve human interaction.

For example lets take my friends command he is writing, rinstall. It's a tool that can install not just binaries but other components. But to install those binaries it needs to know where cargo built them out to. And that's something that cargo configures in it's own way through environment and parsing of a nested series of files.

So there become really three scenarioes:

  • The developer of rinstall duplicates cargos behaviour exactly (how it reads .cargo/config, and CARGO_TARGET_DIR) to find this. This is an annoying experience for the developer, and there is no real "promise" cargo may not change this behaviour in the future. If the developer gets this behaviour duplication wrong (IE forgets the env var) then this confuses users to why this isn't working consistently.
  • The developer prompts the user for the target dir IE cargo rinstall -t /my/custom/target. This is annoying for the user because they already configured this directory in the env or .cargo/config, why do they need to repeat themself?
  • Cargo exposes some of its metadata allowing the developer to have a better developer experience (read env vars), and passing that on to the user who has a predictable user experience.

There is already a precedent to this, with build.rs being "opaque" build scripts, and cargo passes a lot of metadata through to these scripts allowing them to perform extended functionality, without cargo knowing "whats in the script".

So I'd really like to ask you and others to reconsider this and allow cargo to expose some of it's knowledge through to subcommands, so that those subcommands can make better decisions and operations as extensions of cargos functionality. It's worth noting that subcommands are still free to ignore any of the environment that cargo exposes through and they are free to make that choice.

@alexcrichton
Copy link
Member

Sorry I may not be clear, but I'm not saying Cargo shouldn't expose this information, I'm saying that Cargo can't expose the information at the subcommand-invocation-time level. Rather subcommands need to tell Cargo "interpret this cwd as a Rust project and tell me metadata about it". Cargo doesn't know what a subcommand is going to do so it has to wait for the subcommand to ask it information.

It's definitely not expected that you reimplement Cargo. This is why cargo metadata exists, for external tooling to consume.

@Firstyear
Copy link
Contributor Author

Hmmm I don't still see why you say "can't" - If I call cargo subcommand and then my subcommand intern invokes cargo metadata, then why do we need the extra step? If I run cargo build it assumes the cwd is the project/workspace. If I run cargo subcommand I'd expect the same behaviour, and similar, if I run cargo metadata I assume the cwd is the workspace.

So why do we enforce the extra step to prove that this really is the cwd, when we could make it more accessible for people when we already are in that cwd, while retaining the ability that if someone wants to utilise an alternate cwd they can then subsequently invoke cargo metadata?

If this still isn't enough to convince you, then perhaps that on https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-3rd-party-subcommands can we document that cargo metadata is the prefered way to read metadata - literally until you mentioned it here on this thread I had no idea this existed, so making it more prominient in the environment docs could assist people to discover this.

@alexcrichton
Copy link
Member

If Cargo blindly assumed that all subcommands used the cwd as a workspace, that would be incorrect for some subcommands. Cargo, in my opinion, shouldn't be incorrect for those subcommands, and as a result the subcommand should determine for itself if it thinks the current directory is a workspace or not.

@Firstyear
Copy link
Contributor Author

Okay, what do think about the documentation changes in this case?

@alexcrichton
Copy link
Member

Seems reasonable to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
2 participants