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

Unify no-library-target error into no-target warn #14163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryoqun
Copy link

@ryoqun ryoqun commented Jun 28, 2024

What does this PR try to resolve?

Fixes #10958

as far as i checked gitub issues, source code and git log, it seems that there's no strong reason for cargo to emit hard-errors, not warns, only if --lib is given. This is an inconsistent behavior compared to other filters like --bins.

And the no-library-target error predates the generic no-target warn.

All being said, i think it's acceptable to demote the lib error into a generic warn.

How should we test and review this PR?

Updated existing unit tests demonstrate the new expected behavioral change (error => warn demotion)

@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (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-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2024
@@ -372,21 +372,6 @@ impl<'a> UnitGenerator<'a, '_> {
libs.push(proposal)
}
}
if !all_targets && libs.is_empty() && *lib == LibRule::True {
Copy link
Author

@ryoqun ryoqun Jun 28, 2024

Choose a reason for hiding this comment

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

Seems the earliest ancestor of this error is this: https://github.com/rust-lang/cargo/pull/839/files#r1658124748 (around Nov 15, 2014)

miss_count += 1;
filters.push_str(s);
}
};
append(bins, " `bins`,");
append(tests, " `tests`,");
append(examples, " `examples`,");
Copy link
Author

@ryoqun ryoqun Jun 28, 2024

Choose a reason for hiding this comment

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

seems the earliest ancestor of the following out-of-diff warn is this: https://github.com/rust-lang/cargo/pull/9549/files#diff-98da1b66b532e50e9bca971b453ee7de96b17436de1d7ef3824c9888d55be9bbR1189-R1193 (for some reason, i can't comment on the referenced pr....) (around Jun 10, 2021)

                return shell.warn(format!(
                    "target {}{} specified, but no targets matched; this is a no-op",
                    if miss_count > 1 { "filters" } else { "filter" },
                    filters,
                ));

@ryoqun ryoqun marked this pull request as ready for review June 28, 2024 04:40
@ryoqun ryoqun changed the title WIP Unify no-library-target error into no-target warn Unify no-library-target error into no-target warn Jun 28, 2024
Comment on lines -524 to -530
let mut append = |t: &FilterRule, s| {
if let FilterRule::All = *t {
miss_count += 1;
filters.push_str(s);
}
};

Copy link
Author

Choose a reason for hiding this comment

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

needed to move closer to the actually used place to avoid borrow check error.

@epage
Copy link
Contributor

epage commented Jun 28, 2024

Note that #10958 has not been marked Accepted and its generally best to build consensus on a solution in an issue and leave a PR for discussing the implementation.

@ryoqun
Copy link
Author

ryoqun commented Jul 2, 2024

Note that #10958 has not been marked Accepted and its generally best to build consensus on a solution in an issue and leave a PR for discussing the implementation.

thanks for the heads-up. I just replied there: #10958 (comment)

@bors
Copy link
Collaborator

bors commented Jul 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) 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.

Provide an option to cargo check everything except benchmarks
4 participants