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

no python in shell scripts #107812

Closed
wants to merge 5 commits into from
Closed

no python in shell scripts #107812

wants to merge 5 commits into from

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Feb 8, 2023 •
edited by jyn514
Loading

Helps with #94829.

For now this needs you to manually opt in with cargo run --manifest-path src/bootstrap/Cargo.toml -p bootstrap-shim.

@rustbot
Copy link
Collaborator

rustbot commented Feb 8, 2023

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Feb 8, 2023
@onur-ozkan onur-ozkan 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 Feb 8, 2023
@onur-ozkan onur-ozkan force-pushed the no-python-in-shell-scripts branch 3 times, most recently from 886c2ef to b33b610 Compare February 8, 2023 22:36
@onur-ozkan onur-ozkan changed the title [WIP] no python in shell scripts no python in shell scripts Feb 9, 2023
@onur-ozkan
Copy link
Member Author

Regarding to #102282 (comment)

Right now it doesn't actually save any time, because the shim depends on all of bootstrap's dependencies, but I think we can cut down the shim's dependencies to essentially 0 in a follow-up.

Since this is one of the long-term defined task, I believe this should be done in a seperated PR as a seperated task. bootstrap-shim is currently built on top of MinConfig and Flags which are defined in bootstrap tree.

Right now bootstrap-shim doesn't accept any arguments (oops):

This should work just fine with this PR.

This is non-trivial to fix with the current arg-parsing setup: either we'd have to share flags.rs between bootstrap and bootstrap-shim (which I'd rather not do to cut down on compile times), or we'd have to switch to a different arg-parsing library.

I used Flags in bootstrap shim, not just because of fixing the arg passing bug, but also for easier way to set MinConfig specific flags/options without parsing flags from arguments twice. We already parse flags in Config::parse and we need them in MinConfig::parse as well.

We should add a download-bootstrap # bool option to config.toml to tell bootstrap-shim to fall back to using python instead.

I am not sure if this should be covered in this PR.

cc @jyn514

@onur-ozkan onur-ozkan 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 Feb 9, 2023
@onur-ozkan
Copy link
Member Author

r? @jyn514

@rustbot rustbot assigned jyn514 and unassigned albertlarsan68 Feb 9, 2023
@albertlarsan68
Copy link
Member

The correct syntax is @rustbot ready, not the other way around.

albertlarsan68
albertlarsan68 reviewed Feb 10, 2023
View reviewed changes
Copy link
Member

@albertlarsan68 albertlarsan68 left a comment

Choose a reason for hiding this comment

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

Not a full review

src/bootstrap/bin/bootstrap-shim.rs Outdated Show resolved Hide resolved
@onur-ozkan onur-ozkan added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2023
@bors
Copy link
Contributor

bors commented Feb 16, 2023

☔ The latest upstream changes (presumably #108096) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Collaborator

rustbot commented Feb 17, 2023

src/tools/x was changed. Bump version of Cargo.toml in src/tools/x so tidy will suggest installing the new version.

@onur-ozkan onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Feb 17, 2023
jyn514
jyn514 reviewed Feb 17, 2023
View reviewed changes
src/tools/x/Cargo.toml Outdated Show resolved Hide resolved
@onur-ozkan
Copy link
Member Author

@rustbot ready

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 7, 2023
@bors
Auto merge of rust-lang#109448 - ozkanonur:download-beta-compiler-too…
ba86600 
…lchain, r=Mark-Simulacrum

Download beta compiler toolchain in bootstrap if it doesn't yet exist

Blocker for rust-lang#107812 and rust-lang#99989

See: rust-lang#107812 (comment)

r? `@jyn514`
@bors

This comment was marked as resolved.

Sign in to view
@onur-ozkan onur-ozkan removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Apr 8, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
saethlin pushed a commit to saethlin/miri that referenced this pull request Apr 10, 2023
@bors
Auto merge of #109448 - ozkanonur:download-beta-compiler-toolchain, r…
8939456 
…=Mark-Simulacrum

Download beta compiler toolchain in bootstrap if it doesn't yet exist

Blocker for #107812 and #99989

See: rust-lang/rust#107812 (comment)

r? `@jyn514`
@bors
Copy link
Contributor

bors commented Apr 14, 2023

☔ The latest upstream changes (presumably #110324) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Apr 30, 2023

☔ The latest upstream changes (presumably #111017) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented May 5, 2023

☔ The latest upstream changes (presumably #111231) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum
Copy link
Member

#107812 (comment) seems still unresolved here, but it may merit more synchronous discussion - I feel like we're not yet aligned in fact (even if there's no objections) on the exact trajectory here, and that makes me reluctant to merge PRs that create future implications (in particular I still don't think the comments align with a global install).

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2023
@onur-ozkan
Copy link
Member Author

but it may merit more synchronous discussion

I agree, let's talk about this on the next team meeting.

jyn514 and others added 4 commits May 16, 2023 14:00
@jyn514 @onur-ozkan
[wip] Add bootstrap-shim and package it on nightly
ea813a0 
- Pass `dist bootstrap-shim` explicitly in CI

 This makes it possible to run `dist bootstrap` without also building
 bootstrap-shim, and more importantly works around a bug where currently
 two steps aren't allowed to have the same path.

- Add `check::BootstrapShim`
- [wip] start unifying parsing for Config and MinimalConfig
@onur-ozkan
Unify MinimalConfig::parse and Config::parse
8ae63b1 
Signed-off-by: ozkanonur <work@onurozkan.dev>
@jyn514 @onur-ozkan
Also re-download bootstrap if build_helper changes
8a6c18f
@onur-ozkan
move min_config related options from config module
a8f85ae 
Signed-off-by: ozkanonur <work@onurozkan.dev>
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@onur-ozkan
refactor src/tools/x
8ad8906 
Signed-off-by: ozkanonur <work@onurozkan.dev>
@bors
Copy link
Contributor

bors commented May 24, 2023

☔ The latest upstream changes (presumably #111566) made this pull request unmergeable. Please resolve the merge conflicts.

@onur-ozkan onur-ozkan closed this Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@jyn514 jyn514 jyn514 approved these changes

@albertlarsan68 albertlarsan68 albertlarsan68 left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@onur-ozkan @rustbot @albertlarsan68 @bors @jyn514 @Mark-Simulacrum @rust-log-analyzer