rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.72k stars 2.42k forks source link

Request to support disabling path checks when using `cargo install` #14276

Open jdknight opened 3 months ago

jdknight commented 3 months ago

Problem

Been using a couple of tools which manage building multiple packages for a target. For a project, it may use a package which is Cargo-based and will eventually call cargo build/cargo install when processed. These tools will install Cargo packages into a target that is not in the path, but Cargo will generate a warning:

warning: be sure to add `<path>` to your PATH to be able to run the installed binaries

Such a warning is not applicable for these install cases, and it would be nice to have a way to ignore them. An example includes a Buildroot run that uses a cargo-based package can have logs that shows the warning:

>>> tealdeer 1.6.1 Installing to target
cd /home/autobuild/autobuild/instance-15/output-1/build/tealdeer-1.6.1/ && ...
  Installing tealdeer v1.6.1 (/home/autobuild/autobuild/instance-15/output-1/build/tealdeer-1.6.1)
    Finished release [optimized] target(s) in 2.35s
  Installing /home/autobuild/autobuild/instance-15/output-1/target/usr/bin/tldr
   Installed package `tealdeer v1.6.1 (/home/autobuild/autobuild/instance-15/output-1/build/tealdeer-1.6.1)` (executable `tldr`)
warning: be sure to add `/home/autobuild/autobuild/instance-15/output-1/target/usr/bin` to your PATH to be able to run the installed binaries

Proposed Solution

It would be nice to have an environment option or a command line flag to suppress such a warning.

A suggested reference implementation can be seen in https://github.com/rust-lang/cargo/pull/14274.

Notes

I made the unfortunate decision to create the pull request before submitting this issue (:bow:). To pull over an initial comment already brought up the in the pull request:

We have an unstable [lints.cargo] table under development.

I do not believe the prospect [lints.cargo] table is related to this proposed no-path-check configuration hint. The proposed option is more so tailored for users managing installs for a series Cargo packages and not specific to an individual package.

It appears to me that the focus of https://github.com/rust-lang/cargo/issues/12235 is for linting-specific handling. And since I believe a proposed no-path-check is not part of this scope, I have created this new issue. Feel free to correct me if I'm wrong here.

Note that the reference implementation does provide an example supporting a command line option, a configuration flag and an environment variable. Personally, I would be all for just an environment variable (e.g. CARGO_INSTALL_NO_PATH_CHECK), since these scenarios typically have means to manage the environment easily. I only provided the extended options since I did not observe any non-internal flag case where only an environment flag was used.

weihanglo commented 2 months ago

Have mixed feeling about suppressing this warning. It's simple enough that we can just do it. OTOH it also bloats the CLI and configuration interface for such a non-harmful warning (it is not even an error).

In terms of the implementation, I don't think we need a standalone flag in cargo-install. Maybe the configuration is sufficient, as it is able to specify configuration via --config install.no-patch-check=true

weihanglo commented 2 months ago

So maybe adding a install.path-check = <boolean> configuration is good enough? I don't think anyone needs it as a hard error, so no need to have enum options like diabled|warn|forbid.

epage commented 2 months ago

I agree that a CLI flag is overkill for this.

Note that we get env variables "for free" when adding a config field.

epage commented 2 months ago

Maybe there is something I'm missing but I don't understand why this is a problem that needs fixing.

Even if it is, I wonder if a one-off solution is the right approach. We have been working on a linting system and --warnings=deny support. All of that is geared towards workspace-related lints. We've not discussed how we want to handle non-workspace diagnostics, like config or CLI. I worry that we'll do a one off here that won't align well with what we actually want long term. If this was impactful enough, it might be worth a short term solution, but I don't feel this is.

jdknight commented 2 months ago

The goal is to remove a warning that is not applicable to a certain environments (cross compiling for a target). If the installed binaries were in the path for such an environment, it will most likely be an error. Having one less warning message to look at can be nice for builders.