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.

Sign up for GitHub

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

Jump to bottom

fix: using --release/debug and --profile together becomes an error #13971

Merged
merged 2 commits into from
Jun 9, 2024
Merged

fix: using --release/debug and --profile together becomes an error #13971

merged 2 commits into from
Jun 9, 2024

Conversation

heisen-li
Copy link
Contributor

@heisen-li heisen-li commented May 28, 2024 •
edited
Loading

What does this PR try to resolve?

part of #13629

issue #13629 (comment)

@rustbot
Copy link
Collaborator

rustbot commented May 28, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-bench Command-clean Command-install Command-test S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2024
@epage
Copy link
Contributor

epage commented May 29, 2024

Is there something this PR is waiting on for why it is a Draft

@heisen-li heisen-li marked this pull request as ready for review May 31, 2024 07:04
@heisen-li
Copy link
Contributor Author

Is there something this PR is waiting on for why it is a Draft

Sorry, I thought there would be some documentation that would need to be changed, but it doesn't seem to be there when I looked today.

weihanglo
weihanglo reviewed May 31, 2024
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

src/cargo/util/command_prelude.rs Outdated Show resolved Hide resolved
src/cargo/util/command_prelude.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented May 31, 2024

Should we leverage clap's conflict support? We likely would need validation lower level like this. Unsure which style of error messages we'd prefer.

    fn arg_release(self, release: &'static str) -> Self {
        self._arg(
            flag("release", release)
                .short('r')
+              .conflicts_with("profile")
                .help_heading(heading::COMPILATION_OPTIONS),
        )
    }
...
        .arg(flag(
            "debug",
            "Build in debug mode (with the 'dev' profile) instead of release mode",
-        ))
+       ).conflicts_with("profile"))

You don't need to put it on --profile.

@weihanglo
Copy link
Member

Should we leverage clap's conflict support? We likely would need validation lower level like this. Unsure which style of error messages we'd prefer.

This sound better and we can remove most of those tests. (maybe leave one for preventing regression?)

weihanglo
weihanglo approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

Since this turns a warning into a hard error, I would like to schedule the merge after 1.81 nightly is out.

weihanglo
weihanglo reviewed Jun 4, 2024
View reviewed changes
src/cargo/util/command_prelude.rs
Copy link
Member

Choose a reason for hiding this comment

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

#13971 (comment)

@heisen-li have you considered this option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to you guys, I used this suggestion. Regarding tests, I keep a relevant test for each command (rustc, build, check, install).

@weihanglo
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Jun 9, 2024

📌 Commit 5d8022c has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 9, 2024
@bors
Copy link
Collaborator

bors commented Jun 9, 2024

⌛ Testing commit 5d8022c with merge 580dbc6...

@bors
Copy link
Collaborator

bors commented Jun 9, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 580dbc6 to master...

@bors bors merged commit 580dbc6 into rust-lang:master Jun 9, 2024
21 checks passed
This was referenced Jun 10, 2024
Open
Merged
@heisen-li heisen-li deleted the ignore_release branch June 12, 2024 06:45
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
@bors
Auto merge of rust-lang#126290 - weihanglo:update-cargo, r=weihanglo
bdb1b7f 
Update cargo

14 commits in b1feb75d062444e2cee8b3d2aaa95309d65e9ccd..4dcbca118ab7f9ffac4728004c983754bc6a04ff
2024-06-07 20:16:17 +0000 to 2024-06-11 16:27:02 +0000
- Add local registry overlays (rust-lang/cargo#13926)
- docs(change): Don't mention non-existent workspace.badges (rust-lang/cargo#14042)
- test: migrate binary_name to snapbox (rust-lang/cargo#14041)
- Bump to 0.82.0; update changelog (rust-lang/cargo#14040)
- tests: Migrate alt_registry to snapbox (rust-lang/cargo#14031)
- fix: proc-macro example from dep no longer affects feature resolution (rust-lang/cargo#13892)
- chore: Bump cargo-util-schemas to 0.5 (rust-lang/cargo#14038)
- chore(deps): update rust crate pulldown-cmark to 0.11.0 (rust-lang/cargo#14037)
- fix: remove `__CARGO_GITOXIDE_DISABLE_LIST_FILES` env var (rust-lang/cargo#14036)
- chore(deps): update rust crate itertools to 0.13.0 (rust-lang/cargo#13998)
- fix(toml): remove `lib.plugin` key support and make it warning (rust-lang/cargo#13902)
- chore(deps): update compatible (rust-lang/cargo#13995)
- fix: using `--release/debug` and `--profile` together becomes an error (rust-lang/cargo#13971)
- fix(toml): Convert warnings that `licence` and `readme` files do not exist into errors (rust-lang/cargo#13921)

r? ghost
@ehuss ehuss added this to the 1.81.0 milestone Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@ehuss ehuss

Labels
A-cli Area: Command-line interface, option parsing, etc. Command-bench Command-clean Command-install Command-test S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@heisen-li @rustbot @epage @weihanglo @bors @ehuss