no python in shell scripts by onur-ozkan · Pull Request #107812 · rust-lang/rust

onur-ozkan · 2023-02-08T17:55:23Z

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 · 2023-02-08T17:55:32Z

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

onur-ozkan · 2023-02-09T10:51:47Z

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 · 2023-02-09T10:53:11Z

r? @jyn514

albertlarsan68 · 2023-02-09T10:53:25Z

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

albertlarsan68

Not a full review

src/bootstrap/bin/bootstrap-shim.rs

bors · 2023-02-16T04:29:46Z

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

rustbot · 2023-02-17T10:56:04Z

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

src/tools/x/Cargo.toml

onur-ozkan · 2023-02-17T19:13:39Z

@rustbot ready

Mark-Simulacrum

I get the feeling that this PR is trying to do too much which both makes it harder to review and as such harder to land. But given the very sparse PR description, it's not clear to me what the minimal step this PR is taking is; it looks like it (roughly speaking):

Introduces a notion of a minimal configuration
Adds a new component to be distributed (bootstrap-shim)
Refactors a bunch of code (mostly I think a result of minimal config, but seemingly not only - parts feel independent).

I'm having a hard time with this coupling, is there a chance we can avoid it? I've left a few comments inline that I think will help factor things out a little.

Mark-Simulacrum · 2023-03-18T17:10:54Z

src/bootstrap/min_config.rs

+};
+
+/// The bare minimum config, suitable for `bootstrap-shim`, but sharing code with the main `bootstrap` binary.
+/// We only want to include indispensable parts of configuration to have highest possible BC for bootstrap-shim.


Can we avoid "BC" here? I assume that means "build caching", but it does not feel like a standard term to me.

It means "backward compatibility". Pretty common and popular term. I will explicitly update it as "backward compatibility".

Never seen it before, but I think in that case we need more here - what are the implications if we break compatibility? Shouldn't it be the same across both configs?

I see, will provide more detailed informations

I was quite wrong about backwards compatibility. Just realised that.

rust/src/bootstrap/min_config.rs

Lines 20 to 24 in d3039c7

/// The bare minimum config, suitable for `bootstrap-shim`, but sharing code with the main `bootstrap` binary.

/// This only contains fields that are used for `bootstrap-shim` initialization. If the field is not needed for

/// `bootstrap-shim` initialization process, consider adding it in `bootstrap::config::Config`.

#[derive(Default, Clone)]

pub struct MinimalConfig {

Is this clear enough?

Unlike bootstrap itself, bootstrap-shim needs to be compatible across multiple versions of the Rust repo. I thought separating the fields it reads into another struct and adding a big "DO NOT CHANGE THIS LOGIC WITHOUT CHANGING THE SHELL SCRIPTS" comment would make it easier to maintain without accidentally regressing the shim.

I think this is a good start to such a comment -- but I think we also should dig a little deeper in the comment. In particular, I think this "must be compatible" piece is a distinct requirement from the one we typically have -- usually we always are checking the current commit and searching back to find the latest change to some tool and then downloading that.

But, the plan for bootstrap-shim is, I guess, different : we will be using a global install of it? I forget the details now, but this seems like exactly the content I would want in a comment on the minimal config so that we can read that and understand what the compatibility story should be. I'm also not sure how the shell scripts play into this yet, but in any case good to reflect into the comment I'm looking for here.

I don't have a huge stake in what we ship or don't ship, but it seems to me that at least in theory if the shim is a separate binary we should see LLVM/linkers cut out any unused dependencies. On the other hand, maybe that doesn't give us that much - seems like the shim ends up at least pulling in decompression (xz + gzip?), tar, etc, so it probably won't be tiny.

Anyway to summarize it seems to me that probably the next step is for @jyn514 to maybe either point to a hackmd (I seem to recall one) or otherwise reflect some of these thoughts into the comment, so we can move forward based on that. I think there's also some other comments on this PR I've left that seem to still be warranted to resolve from my perspective, but it's possible that with a more detailed description of the purpose/rationale/strategy for the shim and minimal config I'll change my mind :)

Sounds like we're making definite progress though.

Here's the hackmd you're remembering: https://hackmd.io/j-XXBeYERuajJknd6zErPA?both

once the blocker PR is merged, I will re-update the doc-comments by the given updates from @jyn514

Here's the hackmd you're remembering: https://hackmd.io/@jynelson/S1Aa4cozo/edit

FYI the link is broken for me. Returns 404 Not Found.

fixed, thank you

rust/src/bootstrap/min_config.rs

Lines 20 to 26 in 8ad8906

/// The bare minimum config, suitable for `bootstrap-shim`, but sharing code with the main `bootstrap` binary.

/// Unlike bootstrap itself, bootstrap-shim needs to be compatible across multiple versions of the Rust repo.

///

/// FIXME: Update the following information once bootstrap-shim is default in shell scripts

///

/// Once bootstrap-shim is used as default(today it's opt in) in shell scripts, DO NOT CHANGE this configuration

/// without the version bump for the pinned nightly version of bootstrap-shim.

I didn't want to leave it as to be filled, because devs(who needs to make a change on config) might still be confused until we make the shim default. So I give the idea why it shouldn't be changed directly, and left a FIXME to update it once shim is default.

What do you think about the current change? @Mark-Simulacrum

Mark-Simulacrum · 2023-03-18T17:12:02Z

src/bootstrap/min_config.rs

+
+/// The bare minimum config, suitable for `bootstrap-shim`, but sharing code with the main `bootstrap` binary.
+/// We only want to include indispensable parts of configuration to have highest possible BC for bootstrap-shim.
+/// Prefer adding new fields to `bootstrap::config::Config` if possible.


"if possible" feels like it's not very clear to me. For example, I see comments below about verbose and dry_run not being used in the shim, yet they are here. So that seems odd - why are they here?

I believe it's clear enough. We simply say "the reason we have this minimal version is to avoid breaking backward compatibility". (for now it's written as BC, I guess that's why you thought it's not clear).

Mark-Simulacrum · 2023-03-18T17:18:08Z

src/bootstrap/config.rs

+            )
+        };
+
+        config.minimal_config = MinimalConfig::parse(&flags, toml.build.clone());


This feels like the wrong approach to me. We should always start with MinimalConfig::parse, and then take that as an argument for the big Config struct. Any processing that is necessary for the minimal config is likely to also be necessary here, and can be read from fields in that struct as needed.

I think that will help remove the helper functions shared between the two (e.g., set_cfg_path_and_return_toml_cfg) and make the relationship between the two clearer in general.

We should always start with MinimalConfig::parse, and then take that as an argument for the big Config struct.

Right now min_config module is short and simple. If I apply the refactoring based on this suggestion, this will cause having a lot of Config field calculations in min_config module which will kill the seperation between them. Which will make the maintenance harder for both Config and MinConfig. Are you sure you want me to do this?

I don't understand. Why would Config field calculations need to move into MinConfig for this refactoring? It seems like only the calculations for the fields already in MinConfig would need to move; nothing else should. And those calculations already need to be in MinConfig anyway, as otherwise we can't just parse it?

So you want MinConfig field mapping to be moved in config module then. I am still unsure if this will be better. We intentionally did this seperation with 54b7ff2, but I can do it if you want.

src/bootstrap/config.rs

bors · 2023-03-19T23:18:45Z

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

onur-ozkan · 2023-03-21T08:20:51Z

@rustbot ready

src/bootstrap/check.rs

src/bootstrap/builder.rs

bors · 2023-04-14T18:05:16Z

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

bors · 2023-04-30T20:29:58Z

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

bors · 2023-05-05T08:07:28Z

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

Mark-Simulacrum · 2023-05-06T16:51:16Z

#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).

onur-ozkan · 2023-05-06T21:38:47Z

but it may merit more synchronous discussion

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

- 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

Signed-off-by: ozkanonur <work@onurozkan.dev>

bors · 2023-05-24T14:05:43Z

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

rustbot assigned albertlarsan68 Feb 8, 2023

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 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 changed the title ~~[WIP] no python in shell scripts~~ no python in shell scripts Feb 9, 2023

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

rustbot assigned jyn514 and unassigned albertlarsan68 Feb 9, 2023

onur-ozkan force-pushed the no-python-in-shell-scripts branch from 8c36d7a to 8411dd1 Compare February 9, 2023 20:51

albertlarsan68 reviewed Feb 10, 2023

View reviewed changes

src/bootstrap/bin/bootstrap-shim.rs Outdated Show resolved Hide resolved

onur-ozkan added S-blocked Status: 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

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: Blocked on something else such as an RFC or other implementation work. labels Feb 17, 2023

jyn514 reviewed Feb 17, 2023

View reviewed changes

src/tools/x/Cargo.toml Outdated Show resolved Hide resolved

onur-ozkan force-pushed the no-python-in-shell-scripts branch from 15fc13e to a445f80 Compare February 17, 2023 18:46

jyn514 mentioned this pull request Mar 14, 2023

add bootstrap tests for detecting src and out #109120

Open

onur-ozkan mentioned this pull request Mar 15, 2023

extend detect_src_and_out test #109162

Merged

Mark-Simulacrum reviewed Mar 18, 2023

View reviewed changes

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 Mar 18, 2023

onur-ozkan force-pushed the no-python-in-shell-scripts branch 4 times, most recently from 90f7294 to d3039c7 Compare March 21, 2023 08:18

onur-ozkan mentioned this pull request Mar 21, 2023

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

Merged

This comment was marked as resolved.

Sign in to view

Mark-Simulacrum reviewed Apr 2, 2023

View reviewed changes

src/bootstrap/check.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

jyn514 reviewed Apr 8, 2023

View reviewed changes

src/bootstrap/builder.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jyn514 and others added 4 commits May 16, 2023 14:00

Unify MinimalConfig::parse and Config::parse

8ae63b1

Signed-off-by: ozkanonur <work@onurozkan.dev>

Also re-download bootstrap if build_helper changes

8a6c18f

move min_config related options from config module

a8f85ae

Signed-off-by: ozkanonur <work@onurozkan.dev>

This comment has been minimized.

Sign in to view

refactor src/tools/x

8ad8906

Signed-off-by: ozkanonur <work@onurozkan.dev>

	/// The bare minimum config, suitable for `bootstrap-shim`, but sharing code with the main `bootstrap` binary.
	/// This only contains fields that are used for `bootstrap-shim` initialization. If the field is not needed for
	/// `bootstrap-shim` initialization process, consider adding it in `bootstrap::config::Config`.
	#[derive(Default, Clone)]
	pub struct MinimalConfig {

	/// The bare minimum config, suitable for `bootstrap-shim`, but sharing code with the main `bootstrap` binary.
	/// Unlike bootstrap itself, bootstrap-shim needs to be compatible across multiple versions of the Rust repo.
	///
	/// FIXME: Update the following information once bootstrap-shim is default in shell scripts
	///
	/// Once bootstrap-shim is used as default(today it's opt in) in shell scripts, DO NOT CHANGE this configuration
	/// without the version bump for the pinned nightly version of bootstrap-shim.

Uh oh!

Conversation

onur-ozkan commented Feb 8, 2023 • edited by jyn514 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 8, 2023

Uh oh!

onur-ozkan commented Feb 9, 2023

Uh oh!

onur-ozkan commented Feb 9, 2023

Uh oh!

albertlarsan68 commented Feb 9, 2023

Uh oh!

albertlarsan68 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bors commented Feb 16, 2023

Uh oh!

rustbot commented Feb 17, 2023

Uh oh!

Uh oh!

onur-ozkan commented Feb 17, 2023

Uh oh!

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyn514 Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Mar 19, 2023

Uh oh!

onur-ozkan commented Mar 21, 2023

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

This comment has been minimized.

Uh oh!

bors commented Apr 14, 2023

Uh oh!

bors commented Apr 30, 2023

Uh oh!

This comment has been minimized.

bors commented May 5, 2023

Uh oh!

Mark-Simulacrum commented May 6, 2023

onur-ozkan commented Feb 8, 2023 •

edited by jyn514

Loading

jyn514 Mar 26, 2023 •

edited

Loading