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

Improve defaults in x.py #326

Closed
3 tasks done
jyn514 opened this issue Jul 2, 2020 · 9 comments
Closed
3 tasks done

Improve defaults in x.py #326

jyn514 opened this issue Jul 2, 2020 · 9 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@jyn514
Copy link
Member

jyn514 commented Jul 2, 2020

Proposal

  • Make the default stage dependent on the subcommand
  • Don't build stage1 rustc artifacts with x.py build --stage 1. If this is what you want, use x.py build --stage 2 instead, which gives you a working libstd.
  • Change default debuginfo when debug = true from 2 to 1
  • Enable debug = true by default This change is no longer planned.

See also rust-lang/rust#73964 which is a WIP which makes the changes in bootstrap, but not in CI or in the documentation.

Make the default stage dependent on the subcommand

x.py build/test: stage 1

I've seen very few people who actually use full stage 2 builds on purpose. These compile rustc and libstd twice three times and don't give you much more information than a stage 1 build (except in rare cases like rust-lang/rust#68692 (comment)). For new contributors, this makes the build process even more daunting than it already is. As long as CI is changed to use --stage 2 I see no downside here.

x.py bench/dist/install: stage 2

These commands have to do with a finished, optimized version of rustc. It seems very rare to want to use these with a stage 1 build.

x.py doc: stage 0

Normally when you document things you're just fixing a typo. In this case there is no need to build the whole rust compiler, since the documentation will usually be the same when generated with the beta compiler or with stage 1.

Note that for this release cycle only there will be a significant different between stage0 and stage1 docs: rust-lang/rust#73101. However most of the time this will not be the case.

Don't build stage1 rustc artifacts with x.py build --stage 1

Currently the behavior is as follows (where stageN <-> stage(N-1) artifacts, except for stage0 libstd):

  • x.py build --stage 0:
    • stage0 libstd
    • stage0 rustc (but without putting rustc in stage1/)

This leaves you without any rustc at all except for the beta compiler
(rust-lang/rust#73519). This is never what you want (currently you should use either x.py check or x.py build --stage 0 src/libstd instead).

  • x.py build --stage 1:
    • stage0 libstd
    • stage0 rustc
    • stage1 libstd
    • stage1 rustc
    • stage1 rustdoc

This leaves you with a stage1 rustc which doesn't even have
libcore and is effectively useless. Additionally, it compiles rustc
twice, which is not normally what you want.

  • x.py build --stage 2:
    • stage0 libstd
    • stage0 rustc
    • stage1 libstd
    • stage1 rustc
    • stage2 libstd
    • stage2 rustc
    • stage2 rustdoc

This builds the whole source tree in release mode. This is the correct usage for CI,
but takes far too long for development.

The proposed new behavior is as follows:

  • x.py build --stage 0:
    • stage0 libstd

This is suitable for contributors only working on the standard library,
as it means rustc never has to be compiled.

  • x.py build --stage 1:
    • stage0 libstd
    • stage0 rustc
    • stage1 libstd
    • stage1 rustdoc

This is suitable for contributors working on the compiler. It ensures
that you have a working rustc and libstd without having to pass
src/libstd in addition.

  • x.py build --stage 2:
    • stage0 libstd
    • stage0 rustc
    • stage1 libstd
    • stage1 rustc
    • stage2 libstd
    • stage2 rustdoc

This is suitable for debugging failures which only occur the second time rustc is built.

CI wants even more than this: x.py build --stage 2 + stage2 rustc. There will be a new option added to meet this use case: x.py build src/rustc.

To get the previous behavior of x.py build --stage 1, use x.py build --stage 2 src/libstd, which also builds a working libstd, but does not build the release tools. --stage 2 works the same, and --stage 0 was broken anyway.

Change default debuginfo when debug = true from 2 to 1

From a conversation in discord:

Linking seems to consume all available RAM, leading to the OS to swap memory to disk and slowing down everything in the process
Compiling itself doesn't seem to take up as much RAM, and I'm only looking to check whether a minimal testcase can be compiled by rustc, where the runtime performance isn't much of an issue

do you have debug = true or debuginfo-level = 2 in config.toml?
if so I think that results in over 2GB of debuginfo nowadays and is likely the culprit
which might mean we're giving out bad advice :(

There is another conversation about how debuginfo-level is painful on Zulip.

Anecdotally, this sped up my stage 1 build from 15 to 10 minutes. It still adds line numbers, it only removes variable and type information.

Enable debug = true by default

This allows contributors to get RUST_LOG=debug messages without having
to modify config.toml. It also enables debug assertions by default.

This change is no longer planned.

Mentors or Reviewers

I do not have a mentor in mind. I do know several people who have expressed interested in having different defaults: @eddyb, @joshtriplett, @pnkfelix, various others.

Process

The main points of the Major Change Process is as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@jyn514 jyn514 added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Jul 2, 2020
@rustbot
Copy link
Collaborator

rustbot commented Jul 2, 2020

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jul 2, 2020
@Mark-Simulacrum
Copy link
Member

I can volunteer as reviewer/mentor on the changes, but do not have bandwidth to work on building consensus around these (or determining what the right defaults are etc).

@jyn514
Copy link
Member Author

jyn514 commented Jul 3, 2020

After discussion about compile times, memory usage, and runtime performance, it seems that debug = true is not the best change for everyone (or at least, not a clear win the way the other changes are). I will remove it from the MCP and the draft PR.

@jyn514 jyn514 changed the title Use sane defaults in x.py Improve defaults in x.py Jul 7, 2020
@nikomatsakis
Copy link
Contributor

@rustbot second

I'm not 100% sure whether these are the right defaults, in particular the staging question, but I think it's worth a try!

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jul 7, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Jul 8, 2020
@jyn514
Copy link
Member Author

jyn514 commented Jul 14, 2020

As originally written, this MCP used imprecise numbering for the stages that didn't match the numbering used by x.py. With @eddyb's help I have updated it to be consistent with x.py. However there is no difference in the changes proposed.

See the Zulip thread for more discussion.

@jyn514
Copy link
Member Author

jyn514 commented Jul 18, 2020

When writing this I forgot that there are multiple x.py subcommands, and --stage 1 isn't a good default for all of them. Accordingly I'll change the MCP to have different default stages for different commands, as discussed on Zulip.

@spastorino spastorino added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Jul 22, 2020
@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jul 22, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Jul 22, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 28, 2020
Improve defaults in x.py

- Make the default stage dependent on the subcommand
- Don't build stage1 rustc artifacts with x.py build --stage 1. If this is what you want, use x.py build --stage 2 instead, which gives you a working libstd.
- Change default debuginfo when debug = true from 2 to 1

I tried to fix CI to use `--stage 2` everywhere it currently has no stage, but I might have missed a spot.
This does not update much of the documentation - most of it is in https://github.com/rust-lang/rustc-dev-guide/ or https://github.com/rust-lang/rust-forge and will need a separate PR.

See individual commits for a detailed rationale of each change.
See also the MCP: rust-lang/compiler-team#326

r? @Mark-Simulacrum , but anyone is free to give an opinion.
@vext01
Copy link

vext01 commented Sep 3, 2020

Hi! Sorry, I'm late to the party.

I think making stage 1 default is a good idea, but there are a couple of other things that confuse me about this change.

Old behaviour

x.py build --stage 1
This leaves you with a stage1 rustc which doesn't even have libcore and is effectively useless.

When I work on rust (mostly in our fork, which I'm regularly syncing with you folks) ./x.py --stage 1 is what I've always used to try out my changes. And I've always been able to compile programs using libcore with the resulting compiler. But it says above that libcore isn't built. Is that right?

I wonder if I'm inadvertently doing something odd without realising.

New behaviour

x.py build --stage 1 [does not build rustc]

I don't understand the motivation for this. When I do a stage 1 build, a working rustc is the thing I care about most. That's what I need to run to see if my changes did what I expected.

It's not hard to add src/rustc to the invocation, but seems like a strange default, doesn't it?

Thanks!

@jyn514
Copy link
Member Author

jyn514 commented Sep 3, 2020

@vext01

When I work on rust (mostly in our fork, which I'm regularly syncing with you folks) ./x.py --stage 1 is what I've always used to try out my changes. And I've always been able to compile programs using libcore with the resulting compiler. But it says above that libcore isn't built

You've built two versions of the compiler. One is the 'stage 0 artifacts', which end up in stage1/bin. One is the 'stage 1 artifacts', which end up in stage2/bin. The first one is the one that works successfully to build programs with libcore, what this change does is no longer build the second one.

I don't understand the motivation for this. When I do a stage 1 build, a working rustc is the thing I care about most. That's what I need to run to see if my changes did what I expected.

You still get a working rustc in the same place as before. It's just not doing extra work to build two versions of the compiler.

@vext01
Copy link

vext01 commented Sep 3, 2020

OK, so looks like I misunderstood on both counts, so sorry for my noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

6 participants