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

add NO_OP mode to rustup-init.sh #3830

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented May 15, 2024

As part of one of my tasks I need to resolve the target triple at the shell script level during the initial bootstrap phase (before rustc is available). The functionality is already implemented in the rustup script and I don't want to duplicate it in the upstream rust repository. My plan is to fetch the rustup script from https://sh.rustup.rs using curl and then source it from the caller script, so I can access/execute the get_architecture function. However, this is not possible because it immediately initiates the download process. As a solution to that, I propose this PR (which is a dead-simple change) that allows us to source this file easily (like NO_OP=1 . ./rustup-init.sh) without runnig the actual workflow.

@onur-ozkan onur-ozkan marked this pull request as draft May 15, 2024 17:44
@onur-ozkan onur-ozkan changed the title add --no-op/-n arg add NO_OP mode to rustup-init.sh May 15, 2024
@onur-ozkan onur-ozkan marked this pull request as ready for review May 15, 2024 18:17
When calling this script, it immediately starts the download process.
This change is mostly useful for when we want to source this file from
another script file.

Signed-off-by: onur-ozkan <[email protected]>
@djc
Copy link
Contributor

djc commented May 15, 2024

As part of one of my tasks I need to resolve the target triple at the shell script level during the initial bootstrap phase (before rustc is available).

What are you actually trying to achieve?

I'm not inclined to add complexity to this script in order to support niche use cases like this. You can just copy the source code of the rustup-init.sh.

@onur-ozkan
Copy link
Member Author

fwiw initially I tried adding "--no-op/-n" argument, but passing arguments wasn't working well when sourcing the script file.

@onur-ozkan
Copy link
Member Author

onur-ozkan commented May 15, 2024

What are you actually trying to achieve?

We are replacing the python sources from bootstrap with shell scripts in order to remove python dependency on bootstrap. Which means we need to figure out the target triple and download its stage0 artifacts.

I'm not inclined to add complexity to this script in order to support niche use cases like this. You can just copy the source code of the rustup-init.sh.

The change is quite simple actually. Do you think copying part of this source will be better in terms of maintainability? It seems much complex solution IMO.

@rami3l
Copy link
Member

rami3l commented May 15, 2024

As part of one of my tasks I need to resolve the target triple at the shell script level during the initial bootstrap phase (before rustc is available).

What are you actually trying to achieve?

I'm not inclined to add complexity to this script in order to support niche use cases like this. You can just copy the source code of the rustup-init.sh.

@djc I think the goal is to reuse Rustup's host triple resolution as a shell script module.

@onur-ozkan I guess rustup-init.sh it's not intended to be imported, so it is not guaranteed to have a stable API surface... OTOH I can see how it might be helpful to reuse a part of its logic. How about duplicating the functionalities you need in another repo first? I guess we can figure out how to reuse it later (e.g. submodules is an option).

@onur-ozkan
Copy link
Member Author

onur-ozkan commented May 16, 2024

@djc I think the goal is to reuse Rustup's host triple resolution as a shell script module.

Correct.

@onur-ozkan I guess rustup-init.sh it's not intended to be imported, so it is not guaranteed to have a stable API surface... OTOH I can see how it might be helpful to reuse a part of its logic. How about duplicating the functionalities you need in another repo first? I guess we can figure out how to reuse it later (e.g. submodules is an option).

I can duplicate the script, I thought it would be much better to import the script right away (as the change we are adding here simply quits the process, it should be stable enough) so we don't have to maintain/sync the another copy. But I see your point, I can copy it as a start.

@rami3l
Copy link
Member

rami3l commented May 16, 2024

I can duplicate the script, I thought it would be much better to import the script right away (as the change we are adding here simply quits the process, it should be stable enough) so we don't have to maintain/sync the another copy. But I see your point, I can copy it as a start.

@onur-ozkan I mean I don't see why we cannot both depend on some same thing in the end. If you have found out the right boundaries to make the split, we can depend on you instead :)

PS: This functionality is especially interesting for our downstreams as well.
PPS: Please note that the logic of get_architecture() is going through a change in #3800.

@rami3l
Copy link
Member

rami3l commented Oct 26, 2024

@djc @ChrisDenton I've been having second thoughts on this one, since adding an optional "dry run" mode would also be useful for testing target detection on our side (even in cases like #3800).

But maybe it should not be implemented like this (I mean the if __name__ == "__main__":-like trick used in this PR to use rustup-init.sh "as a library"); I think better UX will be achieved by stdout/stderr outputs.

It's true that we don't have guarantees about CLI output stability, but at least we have this --version thing, so the user can still get the slightest clues about what they'd expect. And if they'd like to have a "stable" script, they could download from, say, GitHub raw?

@djc
Copy link
Contributor

djc commented Oct 27, 2024

Can you describe what you would propose exactly (maybe as a PR if that's a straightforward way to implement it)?

@rami3l
Copy link
Member

rami3l commented Oct 28, 2024

Can you describe what you would propose exactly (maybe as a PR if that's a straightforward way to implement it)?

@djc I'd like to propose an alternative execution mode for the rustup-init.sh script that will print out what triple it'll download the binary for without doing anything. I'm not sure about the output format yet, but ideally it'd be machine-readable.

How does that sound?

@onur-ozkan
Copy link
Member Author

Alternatively, we could move the useful functions into a separate script that only provides the functions without executing anything. This way we can import and use it from other places without needing to add an extra execution mode to rustup-init.sh.

@ChrisDenton
Copy link
Member

This is sounding like the --dry-run option some tools have.

I'd be a bit uncomfortable with a separate script due to the need to keep the two scripts in sync.

@onur-ozkan
Copy link
Member Author

I'd be a bit uncomfortable with a separate script due to the need to keep the two scripts in sync.

I didn’t mean that. rustup-init.sh can use the separate script to call functions.

@djc
Copy link
Contributor

djc commented Oct 28, 2024

Can you describe what you would propose exactly (maybe as a PR if that's a straightforward way to implement it)?

@djc I'd like to propose an alternative execution mode for the rustup-init.sh script that will print out what triple it'll download the binary for without doing anything. I'm not sure about the output format yet, but ideally it'd be machine-readable.

How does that sound?

Sounds reasonable to me.

@rami3l
Copy link
Member

rami3l commented Oct 29, 2024

I'd be a bit uncomfortable with a separate script due to the need to keep the two scripts in sync.

I didn’t mean that. rustup-init.sh can use the separate script to call functions.

@onur-ozkan Yeah, that's basically what I proposed initially, but after some investigation on my side... It now seems to me that the user's side would very much like to have a self-contained script at least for rustup-init.sh, and when it comes to independent audits, it'd be weird to do dependency management in Shell (about the only thing I can think of is to use GitHub raw pointing to exact commits).


On the other hand, some rambling: we have one strange requirement that calling the script with --version gives a commit hash as rustup-init the executable currently does:

rustup/rustup-init.sh

Lines 33 to 40 in 708ffd6

# NOTICE: If you change anything here, please make the same changes in setup_mode.rs
usage() {
cat <<EOF
rustup-init 1.27.1 (a8e4f5c64 2024-04-24)
The installer for rustup
Usage: rustup-init[EXE] [OPTIONS]

My original plan was that we keep the rustup-init.sh here in the repo as some sort of "template script" that leaves this field blank (so that it happens to run in the Shell) to be filled by t-release. However, maybe these two things combined means the Shell script can really benefit from a "compilation from source" step? I don't really know 🤷‍♀️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants