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

Slightly refactor TargetSelection in bootstrap #128983

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Aug 11, 2024

Mostly a drive-by refactoring of TargetSelection to reduce some manual "windows-gnu" detection and also accesses to the triple field.

r? @onur-ozkan

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 11, 2024

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 11, 2024
@onur-ozkan
Copy link
Member

Feel free to r=me once rebased.

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 13, 2024

@bors r=onur-ozkan

@bors
Copy link
Contributor

bors commented Aug 13, 2024

📌 Commit 1c0c2c3 has been approved by onur-ozkan

It is now in the queue for this repository.

@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 Aug 13, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#128643 (Refactor `powerpc64` call ABI handling)
 - rust-lang#128655 (std: refactor UNIX random data generation)
 - rust-lang#128745 (Remove unused lifetime parameter from spawn_unchecked)
 - rust-lang#128841 (bootstrap: don't use rustflags for `--rustc-args`)
 - rust-lang#128983 (Slightly refactor `TargetSelection` in bootstrap)
 - rust-lang#129026 (CFI: Move CFI ui tests to cfi directory)
 - rust-lang#129040 (Fix blessing of rmake tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9d9c9ac into rust-lang:master Aug 13, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2024
Rollup merge of rust-lang#128983 - Kobzol:bootstrap-target, r=onur-ozkan

Slightly refactor `TargetSelection` in bootstrap

Mostly a drive-by refactoring of `TargetSelection` to reduce some manual "windows-gnu" detection and also accesses to the `triple` field.

r? `@onur-ozkan`
@Kobzol Kobzol deleted the bootstrap-target branch August 13, 2024 14:42
@ColinFinck
Copy link
Contributor

@Kobzol I stumbled upon this PR as it conflicted with my #128876

Looking more into it, we now join target to the path in some places and target.triple in other places.
target implements Display and outputs target.triple there, but in the case that target.file is set, it also outputs the file name in parentheses. Is this really what we want? Don't we want to use target.triple everywhere?

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 13, 2024

I tried to only replace situations where the target was joined as a path, so that the AsRef<Path> implementation is used. If I also replaced target.triple with target somewhere where Display is used, that's a bug! Did you find any such case?

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 13, 2024

Oh, this one: 1c0c2c3#diff-11a177c4925352bc8852241c59fd49c369df531495f11f81914eb6972783bb8aR522 That's not great, I'll send a fix. Thank you!

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 13, 2024

Fixed in #129056. Thanks!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 13, 2024
…zkan

Fix one usage of target triple in bootstrap

This bug was introduced in rust-lang#128983. In this one case, the `TargetSelection` was also used as `Display` (not just as `Path`), which I did not notice in the original PR. If the target contained a custom file, it would be included in its `Display` formatting, even though only the triple should be used.

Found [here](rust-lang#128983 (comment)).

r? `@onur-ozkan`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2024
Rollup merge of rust-lang#129056 - Kobzol:fix-target-triple, r=onur-ozkan

Fix one usage of target triple in bootstrap

This bug was introduced in rust-lang#128983. In this one case, the `TargetSelection` was also used as `Display` (not just as `Path`), which I did not notice in the original PR. If the target contained a custom file, it would be included in its `Display` formatting, even though only the triple should be used.

Found [here](rust-lang#128983 (comment)).

r? `@onur-ozkan`
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. 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.

5 participants