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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Apr 12, 2022
Merged

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

merged 2 commits into from
Apr 12, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 10, 2022 •
edited
Loading

  • 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

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 10, 2022
jyn514 added 2 commits April 10, 2022 17:15
@jyn514
Remove duplicate aliases for codegen_{cranelift,gcc}
986c168 
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.
@jyn514
Add build compiler/rustc_codegen_gcc as an alias for CodegenBackend
4c14383 
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.
@jyn514 jyn514 force-pushed the remove-duplicate-aliases branch from 5d25a30 to 4c14383 Compare April 10, 2022 22:15
@jyn514 jyn514 mentioned this pull request Apr 10, 2022
Merged
@Mark-Simulacrum
Copy link
Member

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+

@bors
Copy link
Contributor

bors commented Apr 11, 2022

📌 Commit 4c14383 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 Apr 11, 2022
@jyn514
Copy link
Member Author

jyn514 commented 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).

@Mark-Simulacrum
Copy link
Member

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
@Dylan-DPC
Rollup merge of rust-lang#95901 - jyn514:remove-duplicate-aliases, r=…
7450505 
…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 Dylan-DPC mentioned this pull request Apr 11, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 11, 2022
@Dylan-DPC
Rollup merge of rust-lang#95901 - jyn514:remove-duplicate-aliases, r=…
c641fed 
…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
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2022
@bors
Auto merge of rust-lang#95944 - Dylan-DPC:rollup-idggkrh, r=Dylan-DPC
de392c7 
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
@bors bors merged commit ec95e7d into rust-lang:master Apr 12, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 12, 2022
@jyn514 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
@Dylan-DPC
Rollup merge of rust-lang#95906 - jyn514:enforce-valid-paths, r=Mark-…
1915df0 
…Simulacrum

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

This has two benefits:
1. 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).
2. 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
@Dylan-DPC
Rollup merge of rust-lang#95906 - jyn514:enforce-valid-paths, r=Mark-…
a4d2bc4 
…Simulacrum

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

This has two benefits:
1. 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).
2. 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
@bors
Auto merge of rust-lang#95906 - jyn514:enforce-valid-paths, r=Mark-Si…
0516711 
…mulacrum

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

This has two benefits:
1. 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).
2. 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jyn514 @rust-highfive @Mark-Simulacrum @bors @bjorn3 @rustbot