-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix --package <non-existent> --workspace #14755
base: master
Are you sure you want to change the base?
Fix --package <non-existent> --workspace #14755
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We suggest a PR coming with some tests to show how the behavior changed. It is better if we can have a commit capturing the current behavior, followed by the fix and the test change with CI passing.
let package = self._values_of("package"); | ||
if w && !package.is_empty() { | ||
all = false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the behavior and is not even proposed in the original issue.
I would recommend starting with a warning first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here I am again, sorry for my lack of seriousness, i have a small question. issuing an error when the user uses --package <package> & --workspace / --all
would be good for this issue? I have the impression that the fact that the package doesn't exist doesn't change anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create a workspace and test it out. Before this patch, cargo b -p foo --workspace
works without error. After this, it reports foo
matching nothing. This should be a warning first, not a hard error.
@rustbot author Also, we recently updated our CI configuration, and this will need to be rebased on the latest master. |
What does this PR try to resolve?
Information is available in the following issue #12978
Additional information
Closes #12978