Cleanup option parsing and config.toml.example #82451

jyn514 · 2021-02-23T18:13:25Z

Add an assertion that link-shared = true when thin-lto = true.
Previously, link-shared would be silently overwritten.
Get rid of Option<bool> in bootstrap/config.rs. Set defaults
immediately instead of delaying until later in bootstrap. This makes
it easier to find what the default value is.
Remove redundant config.x = false when the default was already false
Set defaults for bindir in default_opts() instead of parse()
Update download-ci-llvm = if-supported option to match bootstrap.py
Remove redundant check for link_shared. Previously, it was checked twice.
Update various options in config.toml.example to their defaults.
Previously, some options showed an example value instead of the
default value.
Fix incorrect defaults in config.toml.example
- use-libcxx defaults to false
- Add missing check-stage = 0
- Update several defaults to be conditional (e.g. if incremental { 10 } else { 100 })
Remove redundant defaults in prose
Use the same comment for the default and target-dependent musl-root
Fix typos
Link to cc_detect for cc and cxx, since the logic is ... complicated.
Update more defaults to better reflect how they actually get set
Remove ignored gpg-password-file option

This stopped being used in
7704d35,
but was never removed from config.toml.

Remove unused flags from config.toml

Disallow infodir and localstatedir in config.toml
Allow the flags in ./configure, but give a warning that they will be
ignored.
Fix incorrect comment that datadir will be ignored.

Example output:

$ ./configure --set install.infodir=xxx
configure: processing command line
configure:
configure: install.infodir      := xxx
configure: build.configure-args := ['--set', 'install.infodir=xxx']
warning: infodir will be ignored
configure:
configure: writing `config.toml` in current directory
configure:
configure: run `python /home/joshua/rustc3/x.py --help`
configure:

Update CHANGELOG

cc https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/bootstrap.20defaults

rust-highfive · 2021-02-23T18:13:28Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

config.toml.example

src/bootstrap/config.rs

Mark-Simulacrum

This seems good overall, but left some nits.

Mark-Simulacrum · 2021-03-01T02:07:23Z

config.toml.example

@@ -171,37 +176,37 @@ changelog-seen = 2
 # first compiler.
 #
 # Defaults to host platform
-#build = "x86_64-unknown-linux-gnu"
+#build = <host platform>


I think saying host platform is a plausibly a bit confusing here given host key below, though it's always been a bit hard to pin down what build really is.

Hmm - would it be helpful to say "native platform" instead? Or maybe just "current platform"?

Mark-Simulacrum · 2021-03-01T02:09:22Z

config.toml.example


 # Which triples to build libraries (core/alloc/std/test/proc_macro) for. Each of
 # these triples will be bootstrapped from the build triple themselves.
 #
 # Defaults to `host`. If you set this explicitly, you likely want to add all
 # host triples to this list as well in order for those host toolchains to be
 # able to compile programs for their native target.
-#target = ["x86_64-unknown-linux-gnu"]
+#target = host (array)


"configured host" or something like this, otherwise it's unclear whether this is defaulting to the host platform (i.e., build = ) or the host array above.

I'm not actually convinced that the diffs to these over stating the linux build triple is actually helpful, it feels like it might just confuse people more. We could instead add a comment that this is just an example configuration, but will differ based on the platform being compiled on.

We could instead add a comment that this is just an example configuration, but will differ based on the platform being compiled on.

I like this idea - I'm fine with examples as long as it's clear they're examples and not defaults :) adding that now.

This doesn't seem done, unless I'm missing something.

I seem to have lost these commits somewhere; I'm just going to rewrite the changes.

Mark-Simulacrum · 2021-03-01T02:11:30Z

config.toml.example


 # Instead of download the src/stage0.txt version of rustfmt specified,
 # use this rustfmt binary instead as the stage0 snapshot rustfmt.
-#rustfmt = "/path/to/bin/rustfmt"
+#rustfmt = "<build-dir>/<host platform>/stage0/bin/rustfmt"


I think this change is intending to specify the default, but I think it's not a good idea for us to say what the default is in this case. I really would prefer that people keep the exact location of these files in the "default" out of there configuration, and really even out of their knowledge -- if folks are configuring something else, then the old suggestions here were just as good, but if you're fine with the default then you shouldn't set this at all. We already note the default is specified via src/stage0.txt just above each of these, which seems good enough to me.

Mark-Simulacrum · 2021-03-01T02:12:39Z

config.toml.example

@@ -373,7 +371,9 @@ changelog-seen = 2

 # Sets the number of codegen units to build the standard library with,
 # regardless of what the codegen-unit setting for the rest of the compiler is.
-#codegen-units-std = 1
+# NOTE: building with anything other than 1 is known to occasionally have bugs.
+# See https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/bootstrapping.20on.20s390x.2Fmips.20for.20musl/near/227436952 for details.


I would prefer we not link to ephemeral chats like this. If there's some useful conversation there, we can either inline the salient points into this file or otherwise document it (e.g., on rustc-dev-guide) and link there. One other approach would be to file a GitHub issue with some details on the bugs encountered and how to reproduce them and say something like "those bugs are tracked here: #xxx". If it's true that just setting codegen-units-std to non-1 value causes bugs for most folks we should probably prioritize an investigation at least.

Ok, I opened #83600 and I can link there in config.toml.

config.toml.example

Mark-Simulacrum · 2021-03-01T02:25:25Z

config.toml.example


 # Force static or dynamic linkage of the standard library for this target. If
 # this target is a host for rustc, this will also affect the linkage of the
 # compiler itself. This is useful for building rustc on targets that normally
 # only use static libraries. If unset, the target's default linkage is used.
-#crt-static = false
+#crt-static = <see above> (bool)


Do you mean read the comment on this option? It seems like we should say something like "platform specific" rather than see above, which is pretty confusing to me at least.

Sure thing, done.

This doesn't seem done, unless I'm missing something.

Fixed, thanks. Not sure if it just slipped my mind or if I lost it in git somewhere 🤷

klensy · 2021-03-28T08:21:47Z

It's hard to understand if some values in comments shows their default values or examples.
For example: codegen-units-std broken by default by setting it to codegen-units (that i assume > 1, by default) or not?

src/bootstrap/config.rs

Mark-Simulacrum · 2021-04-07T18:33:28Z

r=me with commits squashed

I think it may be useful to have a broader conversation - somewhere - about our goals for the example file and whether we're meeting them with the current format. I'm worried it's a pretty long file and may not be readily searchable. It may also be fine.

- Add an assertion that `link-shared = true` when `thin-lto = true`. Previously, link-shared would be silently overwritten. - Get rid of `Option<bool>` in bootstrap/config.rs. Set defaults immediately instead of delaying until later in bootstrap. This makes it easier to find what the default value is. - Remove redundant `config.x = false` when the default was already false - Set defaults for `bindir` in `default_opts()` instead of `parse()` - Update `download-ci-llvm = if-supported` option to match bootstrap.py - Remove redundant check for link_shared. Previously, it was checked twice. - Update various options in config.toml.example to their defaults. Previously, some options showed an example value instead of the default value. - Fix incorrect defaults in config.toml.example + `use-libcxx` defaults to false + Add missing `check-stage = 0` + Update several defaults to be conditional (e.g. `if incremental { 10 } else { 100 }`) - Remove redundant defaults in prose - Use the same comment for the default and target-dependent `musl-root` - Fix typos - Link to `cc_detect` for `cc` and `cxx`, since the logic is ... complicated. - Update more defaults to better reflect how they actually get set - Remove ignored `gpg-password-file` option This stopped being used in rust-lang@7704d35, but was never removed from config.toml. - Remove unused flags from `config.toml` + Disallow `infodir` and `localstatedir` in `config.toml` + Allow the flags in `./configure`, but give a warning that they will be ignored. + Fix incorrect comment that `datadir` will be ignored. Example output: ``` $ ./configure --set install.infodir=xxx configure: processing command line configure: configure: install.infodir := xxx configure: build.configure-args := ['--set', 'install.infodir=xxx'] warning: infodir will be ignored configure: configure: writing `config.toml` in current directory configure: configure: run `python /home/joshua/rustc3/x.py --help` configure: ``` - Update CHANGELOG - Add "as an example" where appropriate - Link to an issue instead of to ephemeral chats

jyn514 · 2021-04-07T18:41:18Z

@bors r=Mark-Simulacrum

I think it may be useful to have a broader conversation - somewhere - about our goals for the example file and whether we're meeting them with the current format. I'm worried it's a pretty long file and may not be readily searchable. It may also be fine.

Yeah, I also have some concerns about that ... I've started recommending that people not copy the whole example file because it makes it easier to find which settings you've changed. Maybe we could sort this file somehow to put the most common options first? Or have an easy way to only show the options without the comments - that would trim it down a lot.

bors · 2021-04-07T18:41:19Z

📌 Commit 28e83a4 has been approved by Mark-Simulacrum

jyn514 · 2021-04-07T18:43:44Z

Or have an easy way to only show the options without the comments - that would trim it down a lot.

grep -v '^# ' config.toml.example | sed '/^$/d' | grep -v '^#$' works.

klensy · 2021-04-07T18:47:00Z

config.toml.example

-#codegen-units-std = 1
+# NOTE: building with anything other than 1 is known to occasionally have bugs.
+# See https://github.com/rust-lang/rust/issues/83600.
+#codegen-units-std = codegen-units


Here codegen-units-std = codegen-units use codegen-units as default value, that defined as #codegen-units = if incremental { 256 } else { 16 }, so it will be not 1 and have some bugs by default(by comment)?

Yes, that's correct. It's unclear to me how common the bug is or why it's related to codegen units.

bors · 2021-04-07T20:44:44Z

⌛ Testing commit 28e83a4 with merge 361bfce...

bors · 2021-04-07T23:14:10Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 361bfce to master...

rust-highfive assigned Mark-Simulacrum Feb 23, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2021

jyn514 force-pushed the defaults branch from 2bef491 to adddbee Compare February 23, 2021 18:14

jyn514 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2021

bjorn3 reviewed Feb 23, 2021

View reviewed changes