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

Fix missing rustfmt in msi installer #101993 #131365

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

heiseish
Copy link
Contributor

@heiseish heiseish commented Oct 7, 2024

Context

r​? @jyn514

  • Please let me know if I should request from someone else instead. I divided the changes into 3 separate commits for the ease of review. The refactoring commit fbdfd5c03c3c979bcf105ccdd05ff4ab9f37a763 is a bit more involved, but I think it helps in the long term for readability and to avoid bugs.
  • I changed build-manifest to build_manifest in order to invoke it as a library. Not sure if this is gonna break any upstream processes. I checked generate-manifest-list and generate-release but didn't find any obvious reference
  • Will push fixes for linting later

@rustbot
Copy link
Collaborator

rustbot commented Oct 7, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Oct 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 7, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@heiseish heiseish force-pushed the fix-issue-101993 branch 3 times, most recently from 9758acb to a119803 Compare October 9, 2024 11:24
@Mark-Simulacrum
Copy link
Member

r=me on the first two commits with direct fixes. The refactoring commit is rather large and I'd prefer to see it split into a separate PR and/or have some discussion on Zulip first -- I'm not really following how reusing PkgType significantly helps avoid problems... it seems like a small part of the puzzle at best.

@heiseish
Copy link
Contributor Author

@Mark-Simulacrum thanks! one of the benefits for the refactor commit is to keep the tool attributes (eg is_preview) consistent without having them duplicated in multiple places.

That said, I agree the refactor commit is a bit more involved and happy to open another PR for it. I could also revert the change to build-manifest and declare another enum for internal usage within build step.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 15, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSI installer is missing clippy and rustfmt
5 participants