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

introduce no-path-check for install command #14274

Closed
wants to merge 1 commit into from

Conversation

jdknight
Copy link

What does this PR try to resolve?

Provides the option to allow a user to trigger an install request that will not generate a warning if an installed program is not available in the PATH. This can be helpful when the install root is configured to an interim location (e.g. cross-compiling) and there is a desire to suppress any path checks warnings not relevant for the install case.

For example, using a Buildroot configuration that uses a cargo-based package can have logs that

>>> 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

The warning message ("warning: be sure to add...") is not desired in a cross-compiling scenario.

How should we test and review this PR?

At this time, only manually testing using the following scenarios have been performed:

cargo install --no-path-check
cargo install --config install.no-path-check=true
CARGO_INSTALL_NO_PATH_CHECK=true cargo install

Sanity checking via unit tests should be possible (which can be added later if this change set is something desired).

Additional information

  • Has yet to be discussed/accepted for consideration.
  • First time touching Rust. 😅

Provides the option to allow a user to trigger an install request that
will not generate a warning if an installed program is not available in
the `PATH`. This can be helpful when the install root is configured
to an interim location (e.g. cross-compiling) and there is a desire to
suppress any path checks warnings not relevant for the install case.

Signed-off-by: James Knight <james.d.knight@live.com>
@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-completions Area: shell completions A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation Command-install S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2024
@weihanglo
Copy link
Member

Thanks for the pull request! This seems pretty complete. However, we tend not to add a one-off flag for suppressing warnings.

We have an unstable [lints.cargo] table under development. There is also a discussion for CLI flag of lint level control in general. We may probably want to discuss there instead of adding a specific flag/config only for cargo-install.

See

@epage
Copy link
Contributor

epage commented Jul 20, 2024

As this is more about the build env, [lints] likely wouldn't be appropriate.--warnings=allow would be a big hammer.

Something else to note is that we generally recommend starting with issues

  • Keep problem/solution discussion in one place (sometimes multiple PRs are created per problem)
  • Avoid you doing throwaway work
  • Removes the social pressure to immediately resolve PRs from the conversation

@jdknight
Copy link
Author

Apologies on adding this pull request before submitting a formal issue. I have created a new issue (#14276) that any design/etc. can be discussed on for this change. No pressure if some or all of the current content will not be desired -- working on this change was more so a learning experience to see the effort of making the proposed change.

@jdknight jdknight closed this Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. A-completions Area: shell completions A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation Command-install 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