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

Add a default filter for library/std to build if there are no filters #77489

Closed
wants to merge 1 commit into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Oct 3, 2020

  • Add an opt-in way to build all tools with x.py build src/tools
  • Add an entry in the changelog
  • Bump the major version

This implements rust-lang/compiler-team#351.

r? @Mark-Simulacrum on the implementation

@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 3, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2020
- Add an opt-in way to build all tools with `x.py build src/tools`
- Add an entry in the changelog
- Bump the major version
@tmiasko
Copy link
Contributor

tmiasko commented Oct 3, 2020

Could we bump changelog-seen in config.toml.example along with bumps to the major version?

@petrochenkov
Copy link
Contributor

I disagree with this, it's one more change that requires passing extra options in the common case.

  • rustdoc often breaks when something in rustc is changed, and needs to be tested (it would be nice to build clippy by default as well, since it's now in the repo), other tools are tiny and build quickly.
  • this introduces one more difference between build and test, making the rules more complex and harder to remember.
  • if you are working on something specific requiring limited testing, then it's better to build and test specific components instead of building "everything" with x.py build.

@jyn514
Copy link
Member Author

jyn514 commented Oct 3, 2020

I really wish you would bring this stuff up during the MCP process. All of these are reasonable objections to a decision that happened a month ago.

@petrochenkov
Copy link
Contributor

I haven't seen this specific MCP.

@jyn514
Copy link
Member Author

jyn514 commented Oct 3, 2020

This was rust-lang/compiler-team#351.

@jyn514
Copy link
Member Author

jyn514 commented Oct 3, 2020

rustdoc often breaks when something in rustc is changed

Do you often use build to look for breakage? I normally use check if I'm refactoring.

@@ -8,6 +8,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

None.

## [Version 3] - 2020-10-03
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is worth a major change -- it will build strictly less than before, right? People don't need to be notified of changes that likely don't affect their workflow. The host/build change was much more likely to break your build in a confusing way, here you're just going to get less built and can easily opt back in.

"build" | "b" => Subcommand::Build { paths },
"build" | "b" => {
if paths.is_empty() {
paths.push(PathBuf::from("library/std"));
Copy link
Member

Choose a reason for hiding this comment

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

This feels very odd. This seems no different than just removing DEFAULT = true from all of the build-called steps except std; why is that not the approach taken here?

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2020
@bors
Copy link
Contributor

bors commented Oct 4, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@petrochenkov
Copy link
Contributor

@jyn514

Do you often use build to look for breakage? I normally use check if I'm refactoring.

  • I usually use run specific tests after doing changes to code, e.g ./x.py test --bless --stage 1 src/test/ui --test-args my_test. This use case improved recently because --stage 1 is no longer necessary, but at cost of inconsistencies and more complex rules.
  • I run tidy and ./x.py test --bless --stage 1 --pass check src/test/{pretty,compile-fail,ui} or ./x.py test --bless --stage 1 --pass check src/test/{rustdoc*,pretty,compile-fail,ui} before submitting a PR with small-scale compiler changes. This use case improved recently because --stage 1 is no longer necessary, but at cost of inconsistencies and more complex rules.
  • I run ./x.py test --bless --stage 2 before submitting a PR with large-scale or risky changes with may have unknown effects (e.g. Expand NtExpr tokens only in key-value attributes #77271 or rustbuild: Build tests with LLD if use-lld = true was passed #76378). This use case regressed recently because I have to explicitly add --stage 2 instead of just running x.py test to "test everything".

I honestly don't know any use cases for running x.py build, but deviating its behavior from x.py test (or x.py check) by introducing a different set of built targets only introduces inconsistencies and more complex rules.

@jyn514
Copy link
Member Author

jyn514 commented Oct 4, 2020

I don't feel very strongly about this. @jonas-schievink or @tmandry feel free to take this over if you're interested.

@jyn514 jyn514 closed this Oct 4, 2020
@camelid camelid removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 4, 2020
@jyn514 jyn514 deleted the no-tools branch October 22, 2020 16:51
@jyn514 jyn514 changed the title Add a default filter for library/std if there are no filters Add a default filter to build for library/std if there are no filters Oct 27, 2020
@jyn514 jyn514 changed the title Add a default filter to build for library/std if there are no filters Add a default filter for library/std to build if there are no filters Oct 27, 2020
@jyn514 jyn514 mentioned this pull request Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants