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

Resync config.toml and suggest better settings #795

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

spastorino
Copy link
Member

No description provided.

src/building/how-to-build-and-run.md Show resolved Hide resolved
Comment on lines 81 to 82
# Emits extraneous output from tests to ensure that failures of the test
# harness are debuggable just from logfiles.
verbose-tests = true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Emits extraneous output from tests to ensure that failures of the test
# harness are debuggable just from logfiles.
verbose-tests = true
# Emits extra output from tests so test failures are debuggable just from logfiles.
verbose-tests = true

Extraneous means it's not useful or irrelevant.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jyn514 👍 but consider changing this on rust-lang/rust, I've just copied what it's there already.

Copy link
Member

@eddyb eddyb Aug 30, 2020

Choose a reason for hiding this comment

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

This change doesn't seem right, specifically "test harness" refers to the code running the tests, rather than the tests themselves. And it is extraneous for the common case of e.g. libtest not crashing.

EDIT: found the Rust PR, leaving comment there too.

Copy link
Member

@mark-i-m mark-i-m left a comment

Choose a reason for hiding this comment

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

Thanks!

@mark-i-m mark-i-m merged commit 3078d38 into rust-lang:master Jul 14, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 14, 2020
Comment on lines -39 to -40
# This will make your build more parallel; it costs a bit of runtime
# performance perhaps (less inlining) but it's worth it.
Copy link
Member

Choose a reason for hiding this comment

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

Was this from config.toml.example? It looks like these two lines explained why the = 0 value was set, and this was lost.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is from config.toml.example. I'll add it back in rust-lang/rust#76141.

Comment on lines +81 to +82
# Emits extra output from tests so test failures are debuggable just from logfiles.
verbose-tests = true
Copy link
Member

@eddyb eddyb Aug 30, 2020

Choose a reason for hiding this comment

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

Why was this added? The default is that failures are listed at the end, without noisy output while running the tests.

EDIT: looks like the purpose of this was misunderstood? (see my other comments about verbose-tests)

Comment on lines +78 to +79
# Whether to always use incremental compilation when building rustc
incremental = true
Copy link
Member

Choose a reason for hiding this comment

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

According to Cargo documentation, the incremental default for codegen-units is 256, which makes codegen-units = 0 unnecessary/undesirable (unless you're compiling Rust on a machine with more than 256 hardware cores/threads, heh).

So my understanding is that when adding incremental = true, we also need to remove codegen-units = 0. cc @Mark-Simulacrum

jyn514 added a commit to jyn514/rustc-dev-guide that referenced this pull request Aug 31, 2020
- `verbose-tests` is for debugging the test harness, not the tests
themselves. See also rust-lang/rust#76141
- `codegen-units` defaults to `256` whenever `incremental = true`. So
there's no need to explicitly set it to `0` if we already recommend
incremental. See also rust-lang#795 (comment).
jyn514 added a commit to jyn514/rustc-dev-guide that referenced this pull request Aug 31, 2020
- `verbose-tests` is for debugging the test harness, not the tests
themselves. See also rust-lang/rust#76141
- `codegen-units` defaults to `256` whenever `incremental = true`. So
there's no need to explicitly set it to `0` if we already recommend
incremental. See also rust-lang#795 (comment).
tshepang pushed a commit that referenced this pull request Aug 31, 2020
- `verbose-tests` is for debugging the test harness, not the tests
themselves. See also rust-lang/rust#76141
- `codegen-units` defaults to `256` whenever `incremental = true`. So
there's no need to explicitly set it to `0` if we already recommend
incremental. See also #795 (comment).
tmandry added a commit to tmandry/rust that referenced this pull request Sep 9, 2020
Address review comments about config.toml from rustc-dev-guide PR

This info was lost in rust-lang#74334. See also rust-lang/rustc-dev-guide#795 (review).
r? @Mark-Simulacrum or @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants