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

Don't compile on install without binaries #10842

Conversation

yerke
Copy link
Contributor

@yerke yerke commented Jul 10, 2022

Fixes #8970

This PR is trying to revive the effort started in #9576.
We don't want to compile code if there is no viable binary when running cargo install my-name-here.

The PR in the current form is not great. Due to the new compile filter, we show warning: Target filter `bins` specified, but no targets matched. This is a no-op, even when the customer did not specify --bins.

I think one the way to address this issue would to update CompileMode::Build to CompileMode::Build { install: bool }, but I am not sure if that change would be welcome.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2022
@yerke yerke changed the title Yerke/dont compile on install without binaries Dont compile on install without binaries Jul 10, 2022
@yerke yerke changed the title Dont compile on install without binaries Don't compile on install without binaries Jul 10, 2022
@yerke
Copy link
Contributor Author

yerke commented Aug 19, 2022

@ehuss Do you mind taking a look when you have time? Specifically, I want some guidance on this proposal:
I think one the way to address this issue would to update CompileMode::Build to CompileMode::Build { install: bool }, but I am not sure if that change would be welcome.

Of course, I would love to hear from @epage and @weihanglo as well :)

Thanks!

@ehuss
Copy link
Contributor

ehuss commented Aug 19, 2022

I would prefer to not modify CompileMode as that breaks the abstraction with the lower-level code knowing about higher-level concepts like "install".

As mentioned in #9576 (comment), is it possible to adjust the checks to retain the error message?

@ehuss ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2022
@bors
Copy link
Collaborator

bors commented Feb 23, 2023

☔ The latest upstream changes (presumably #11410) made this pull request unmergeable. Please resolve the merge conflicts.

@yerke
Copy link
Contributor Author

yerke commented Feb 24, 2023

For now I don't have any good ideas on how to achieve this without modifying CompileMode::Build, so I will close this PR.

@yerke yerke closed this Feb 24, 2023
@yerke yerke deleted the yerke/dont-compile-on-install-without-binaries branch February 24, 2023 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo install should not need to build the crate to determine there is no binary available
4 participants