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

config.toml parsing error improvements #82360

Merged
merged 1 commit into from
Mar 1, 2021
Merged

Conversation

osa1
Copy link
Contributor

@osa1 osa1 commented Feb 21, 2021

Improve error messages for musl-libdir and wasi-root keys. Previously
the parser would panic with unwrap(). Now it prints

  Target "wasm32-wasi" does not have a "wasi-root" key

(and similar for the musl-libdir field, which is used in target that
use musl)

Also update comments around wasi-root field to make it clear that the
field is only valid in wasm32-wasi target and needs to be moved to a
[target.wasm32-wasi] section to be valid.

Fixes #82317


r? @Mark-Simulacrum

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

This comment has been minimized.

@osa1
Copy link
Contributor Author

osa1 commented Feb 21, 2021

configure.py looks for an llvm-config key in all target sections, and it doesn't exist in target.wasm32-wasi section.

I think specifying a different llvm-config for different targets doesn't make sense, right? So perhaps this key should be in [target] rather than in target subsections like target.x86_64-....

@Mark-Simulacrum
Copy link
Member

llvm-config can be different for different targets, if e.g. you are targeting 64-bit and 32-bit systems. I think adding a commented one to the wasi section is probably easiest - even if compiling LLVM for wasm32-wasi seems unlikely :)

@osa1
Copy link
Contributor Author

osa1 commented Feb 25, 2021

llvm-config can be different for different targets, if e.g. you are targeting 64-bit and 32-bit systems

I'm surprised that this works. Does LLVM allow linking multiple version in the single binary? If not, I don't see how this can work.

I think adding a commented one to the wasi section is probably easiest - even if compiling LLVM for wasm32-wasi seems unlikely :)

config.toml.example is already quite large. I'm thinking of reverting the changes to change.toml.example and just committing the improved errors messages. Would that work?

@Mark-Simulacrum
Copy link
Member

I'm surprised that this works. Does LLVM allow linking multiple version in the single binary? If not, I don't see how this can work.

Note that we produce a single rustc per target, so there's not actually multiple LLVMs getting linked in. But if I'm producing compilers for, say, both 32-bit and 64-bit windows, those'll need two different LLVMs (just like the Rust code needs to be built twice).

I would be happy to just add the improved error messages, but we can also add a note on the wasi-root in config.toml.example indicating that it likely wants to get set for the wasi target.

@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 Mar 1, 2021
Improve error messages for musl-libdir and wasi-root keys. Previously
the parser would panic with `unwrap()`. Now it prints

      Target "wasm32-wasi" does not have a "wasi-root" key

(and similar for the `musl-libdir` field, which is used in target that
use musl)

Also update comments around wasi-root field to make it clear that the
field is only valid in wasm32-wasi target and needs to be moved to a
`[target.wasm32-wasi]` section to be valid.

Fixes rust-lang#82317
@osa1
Copy link
Contributor Author

osa1 commented Mar 1, 2021

@Mark-Simulacrum OK, that makes sense. I updated the PR.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2021
@Mark-Simulacrum Mark-Simulacrum changed the title config.toml parsing: config.toml parsing error improvements Mar 1, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 1, 2021

📌 Commit 81cfa98 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#81210 (BTreeMap: correct node size test case for choices of B)
 - rust-lang#82360 (config.toml parsing error improvements)
 - rust-lang#82428 (Update mdbook)
 - rust-lang#82480 (Remove `ENABLE_DOWNLOAD_RUSTC` constant)
 - rust-lang#82578 (Add some diagnostic items for Clippy)
 - rust-lang#82620 (Apply lint restrictions from renamed lints)
 - rust-lang#82635 (Fix typos in rustc_infer::infer::nll_relate)
 - rust-lang#82645 (Clarify that SyncOnceCell::set blocks.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c8825d0 into rust-lang:master Mar 1, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 1, 2021
@osa1 osa1 deleted the issue82317 branch March 2, 2021 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config.toml.example: wasi-root field is in a wrong section
6 participants