Remove duplicate aliases for `check codegen_{cranelift,gcc}` and fix `build codegen_gcc` by jyn514 · Pull Request #95901 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation7 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Remove duplicate aliases
Bootstrap already allows selecting these in PathSet::has, which allows
any string that matches the end of a full path.
I found these by adding assert!(path.exists()) in StepDescription::paths.
I think ideally we wouldn't have any aliases that aren't paths, but I've held
off on enforcing that here since it may be controversial, I'll open a separate PR.
Add build compiler/rustc_codegen_gcc as an alias for CodegenBackend
These paths (_cranelift and _gcc) are somewhat misleading, since they
actually tell bootstrap to build all codegen backends. But this seems like
a useful improvement in the meantime.

cc @bjorn3 @antoyo

r? @Mark-Simulacrum

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

Bootstrap already allows selecting these in PathSet::has, which allows any string that matches the end of a full path.

I found these by adding assert!(path.exists()) in StepDescription::paths. I think ideally we wouldn't have any aliases that aren't paths, but I've held off on enforcing that here since it may be controversial, I'll open a separate PR.

These paths (_cranelift and _gcc) are somewhat misleading, since they actually tell bootstrap to build all codegen backends. But this seems like a useful improvement in the meantime.

These paths (_cranelift and _gcc) are somewhat misleading, since they
actually tell bootstrap to build all codegen backends. But this seems like
a useful improvement in the meantime.

This seems like it matches behavior for other such things (e.g., we build all compiler crates regardless of which one in particular is specified), so it definitely seems OK to me at this time.

@bors r+

📌 Commit 4c14383 has been approved by Mark-Simulacrum

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

Apr 11, 2022

This seems like it matches behavior for other such things (e.g., we build all compiler crates regardless of which one in particular is specified), so it definitely seems OK to me at this time.

👍 Makes sense. Just FYI, I would like to make bootstrap more granular in the long term - #95503 is a first step there, and I hope to eventually remove all_crates together (which allows many different crates to build the same step without you thinking too hard about it).

Yeah, it makes sense to me as a goal (I often manually hack this by editing bootstrap with a -p flag when I want it today). I think we'll want to be careful around the UX of actually building a sysroot with std or without (since rustc is not "at" an obvious place in the crate tree) but we'll figure that out as we go, I'm sure.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request

Apr 11, 2022

…Mark-Simulacrum

Remove duplicate aliases for check codegen_{cranelift,gcc} and fix build codegen_gcc

Remove duplicate aliases Bootstrap already allows selecting these in PathSet::has, which allows any string that matches the end of a full path.

I found these by adding assert!(path.exists()) in StepDescription::paths. I think ideally we wouldn't have any aliases that aren't paths, but I've held off on enforcing that here since it may be controversial, I'll open a separate PR.
Add build compiler/rustc_codegen_gcc as an alias for CodegenBackend

These paths (_cranelift and _gcc) are somewhat misleading, since they actually tell bootstrap to build all codegen backends. But this seems like a useful improvement in the meantime.

cc @bjorn3 @antoyo

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request

Apr 11, 2022

…Mark-Simulacrum

Remove duplicate aliases for check codegen_{cranelift,gcc} and fix build codegen_gcc

Remove duplicate aliases Bootstrap already allows selecting these in PathSet::has, which allows any string that matches the end of a full path.

I found these by adding assert!(path.exists()) in StepDescription::paths. I think ideally we wouldn't have any aliases that aren't paths, but I've held off on enforcing that here since it may be controversial, I'll open a separate PR.
Add build compiler/rustc_codegen_gcc as an alias for CodegenBackend

These paths (_cranelift and _gcc) are somewhat misleading, since they actually tell bootstrap to build all codegen backends. But this seems like a useful improvement in the meantime.

cc @bjorn3 @antoyo

This was referenced

Apr 11, 2022

bors added a commit to rust-lang-ci/rust that referenced this pull request

Apr 11, 2022

Rollup of 7 pull requests

Successful merges:

rust-lang#95008 ([let_chains] Forbid let inside parentheses)
rust-lang#95801 (Replace RwLock by a futex based one on Linux)
rust-lang#95864 (Fix miscompilation of inline assembly with outputs in cases where we emit an invoke instead of call instruction.)
rust-lang#95894 (Fix formatting error in pin.rs docs)
rust-lang#95895 (Clarify str::from_utf8_unchecked's invariants)
rust-lang#95901 (Remove duplicate aliases for check codegen_{cranelift,gcc} and fix build codegen_gcc)
rust-lang#95927 (CI: do not compile libcore twice when performing LLVM PGO)

Failed merges:

r? @ghost @rustbot modify labels: rollup

jyn514 deleted the remove-duplicate-aliases branch

April 12, 2022 01:58

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request

Apr 17, 2022

…Simulacrum

Require all paths passed to ShouldRun::paths to exist on disk

This has two benefits:

There is a clearer mental model of how bootstrap works. Steps correspond to paths on disk unless it's strictly impossible for them to do so (e.g. dist components).
Bootstrap has better checks for internal consistency. This caught several issues:

src/sanitizers doesn't exist; I changed it to just be a sanitizers alias.
src/tools/lld doesn't exist; I removed it, since lld alone already works.
src/llvm doesn't exist; removed it since llvm and src/llvm-project both work.
src/lldb_batchmode.py doesn't exist, it was moved to src/etc.
install was still using src/librustc instead of compiler/rustc.
None of the tools in dist / install allowed using src/tools/X to build them. This might be intentional - I can change them to aliases if you like.

Builds on rust-lang#95901 and should not be merged before.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request

Apr 18, 2022

…Simulacrum

Require all paths passed to ShouldRun::paths to exist on disk

This has two benefits:

There is a clearer mental model of how bootstrap works. Steps correspond to paths on disk unless it's strictly impossible for them to do so (e.g. dist components).
Bootstrap has better checks for internal consistency. This caught several issues:

src/sanitizers doesn't exist; I changed it to just be a sanitizers alias.
src/tools/lld doesn't exist; I removed it, since lld alone already works.
src/llvm doesn't exist; removed it since llvm and src/llvm-project both work.
src/lldb_batchmode.py doesn't exist, it was moved to src/etc.
install was still using src/librustc instead of compiler/rustc.
None of the tools in dist / install allowed using src/tools/X to build them. This might be intentional - I can change them to aliases if you like.

Builds on rust-lang#95901 and should not be merged before.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Apr 18, 2022

…mulacrum

Require all paths passed to ShouldRun::paths to exist on disk

This has two benefits:

There is a clearer mental model of how bootstrap works. Steps correspond to paths on disk unless it's strictly impossible for them to do so (e.g. dist components).
Bootstrap has better checks for internal consistency. This caught several issues:

src/sanitizers doesn't exist; I changed it to just be a sanitizers alias.
src/tools/lld doesn't exist; I removed it, since lld alone already works.
src/llvm doesn't exist; removed it since llvm and src/llvm-project both work.
src/lldb_batchmode.py doesn't exist, it was moved to src/etc.
install was still using src/librustc instead of compiler/rustc.
None of the tools in dist / install allowed using src/tools/X to build them. This might be intentional - I can change them to aliases if you like.

Builds on rust-lang#95901 and should not be merged before.

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Remove duplicate aliases for check codegen_{cranelift,gcc} and fix build codegen_gcc by jyn514 · Pull Request #95901 · rust-lang/rust (original) (raw)

Conversation

Remove duplicate aliases for `check codegen_{cranelift,gcc}` and fix `build codegen_gcc` by jyn514 · Pull Request #95901 · rust-lang/rust (original) (raw)