Update getting-started.sh to install the specific rust version specified in .github/env file #5317

Wolfenheimm · 2024-08-12T09:30:26Z

Description

Add a means to fetch and install the expected rust version defined in .github/env for the getting-started script, as requested in Issue #5263. Discussions were made and the use of a rust-toolchain.toml file was decided.

Integration

The implementation relies on fetching the rust-toolchain.toml found within the polkadot-sdk repo.

Review Notes

Currently, there is no such rust-toolchain.toml file - but an issue has been made to create one #5335. The file has been added to this PR with basic requirements.
This PR is intended to resolve #5263 and #5335

Checklist

My PR includes a detailed description as outlined in the "Description" and its two subsections above.
My PR follows the labeling requirements of this project (at minimum one label for T
required)
- External contributors: ask maintainers to put the right label on your PR.
I have made corresponding changes to the documentation (if applicable)
I have added tests that prove my fix is effective or that my feature works (if applicable)

… specified in .github/env file

cla-bot-2021 · 2024-08-12T09:30:32Z

User @Wolfenheimm, please sign the CLA here.

…sion wasn't found.

…thub.com/Wolfenheimm/polkadot-sdk into wolfenheimm/getting-started-rust-version

Wolfenheimm · 2024-08-12T22:11:52Z

Updated the code to include installing the latest version of rust if the required version wasn't found.

I've chosen to do a check on the version that is retrieved for an empty string or if the string doesn't contain dots - if the variable passes those constraints, it just defaults to the latest version of rust. My original fix didn't take into account for a bad version fetch.

kianenigma · 2024-08-14T14:54:01Z

scripts/getting-started.sh

@@ -114,8 +114,16 @@ if command -v rustc >/dev/null 2>&1; then
    echo "\n✅︎🦀 Rust already installed."
 else
    if prompt_default_yes "\n🦀 Rust is not installed. Install it?"; then
-        echo "🦀 Installing via rustup."
-        curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
+        version=`cat ../.github/env | grep IMAGE | cut -d'-' -f3`


This seems flaky to me, any change to the other file will break this.

I think the nicer thing to do is to store this in a nicely discover-able variable, and have it be used in both places.

I would expect @alvicsam to know if my worry here is real or not, perhaps it is fine.

It's fine as a temporary solution for now, in the future it should be switched to using rust-toolchain.toml

in the future it should be switched to using rust-toolchain.toml

is this tracked in an issue?

yep, here: #5335

kianenigma

LGTM!

Thanks for @alvicsam for chiming in.

github-actions · 2024-08-16T12:07:01Z

Review required! Latest push from author must always be reviewed

Wolfenheimm · 2024-08-16T13:21:45Z

@alvicsam & @kianenigma, sorry for the inconvenience here, I just wanted to keep the branch updated and clear some fails - will refrain from touching it from now on.

kianenigma · 2024-08-19T16:07:28Z

scripts/getting-started.sh

@@ -114,8 +114,16 @@ if command -v rustc >/dev/null 2>&1; then
    echo "\n✅︎🦀 Rust already installed."
 else
    if prompt_default_yes "\n🦀 Rust is not installed. Install it?"; then
-        echo "🦀 Installing via rustup."
-        curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
+        version=`cat ../.github/env | grep IMAGE | cut -d'-' -f3`


But also, this won't if you are not in the polkadot sdk repo right? this .github/env is not available otherwise.

In other words, have you tried the outcome, and does it actually works?

I'm just now noticing that the curl, even if the version is defined, will always grab latest.

It requires something like:
rustup default 1.77.0

I pushed some changes.

Also you are right in that if you run this script from outside the repo, you will not get the required version, and it will default to using latest, tested.

I would argue that 99% of the users running this script are outside of the repo (recall the script is meant to onboard a new user), so I think the solution presented here is really marginally better than not having it.

I think that having a rust-toolchain file is probably the better approach. In that case all devs can inspect this file and adjust their rust versions accordingly. Hopefully IDEs can even automatically warn you about it.

True, and it could create complications if my proposed script (God forbid) finds "1.2.0" and installs + uses that version.

It might then be useful to add the following line to fetch the toml file directly from polkadot-sdk, once it's added in:

curl -s -H "Accept:application/vnd.github.v3.raw" https://api.github.com/repos/paritytech/polkadot-sdk/contents/rust-toolchain.toml | grep 'channel =' | awk -F'"' '{print $2}'

I pushed an example of this.

@kianenigma - After some guidance from Nazar, a decision was made to pull the rust-toolchain.toml from polkadot-sdk straight into the minimal-template fetched at the end of the script, which should satisfy the requirement. This PR now contains a base toml file with the required version to support this.

Wolfenheimm · 2024-10-10T06:50:19Z

@alvicsam
Fail on job check-getting-started (arch, archlinux, solochain, sh) I'd like some advice (if possible) on this failure, I'm not certain why it's failing here as local tests using getting-started.sh prove functional, I haven't changed much of anything except that and adding a rust-toolchain.toml.

Digging deeper for the time being, but my initial thoughs are that I suspect it's not selecting an option here?
I'm also hoping that adding the toolchain didn't also cause extra errors.

Wolfenheimm · 2024-10-10T08:59:23Z

@alvicsam
I'm noticing that the solochain template in the polkadot-sdk project already contains a rust-toolchain.toml inside an env-setup folder here perhaps I'm going about this in the wrong way.

It might be a better idea to have toolchains already set up inside the other templates like solochain does, like that we wouldn't need to add a redundant toolchain file in the root directory of this project.

The readme states:

Env setup

Special files for setting up an environment to work with the template:

rust-toolchain.toml when working with rustup.
flake.nix when working with nix.

These files will be copied by the installer script to the main directory. They are
put into this special directory to not interfere with the normal CI.

Will dig more, but this seems fruitful.

kianenigma · 2024-10-21T15:06:49Z

@alvicsam I am increasingly thinking that having a rust-toolchain.toml is a great idea. At the moment, if I want to know what versions we use in our CI, and I should expect to work locally, I open a random job (e.g. build-subkey) and inspect a step in the CI process that prints the rust versions. We can do better in this regard :)

alvicsam · 2024-10-22T09:29:28Z

Sure, I absolutely agree. Please fix the check-getting-started ci check and I'm happy to approve this PR.

…nger exists

Wolfenheimm · 2024-10-23T15:52:08Z

@alvicsam I need some input & advice on where it's currently failing, the check-getting-started ci checks pass now, but I'm unsure about the zombienet bridges or malus

ggwpez · 2024-10-23T17:14:38Z

I'm unsure about the zombienet bridges or malus

You can ignore those, they are known to cause issues currently 😄

ggwpez · 2024-10-23T17:26:43Z

/tip small

substrate-tip-bot · 2024-10-23T17:26:48Z

@Wolfenheimm Contributor did not properly post their account address.

Make sure the pull request description (or user bio) has: "{network} address: {address}".

Wolfenheimm · 2024-10-23T19:31:40Z

/tip small

@ggwpez I updated my account with my address, greatly appreciated :)

ggwpez · 2024-10-23T20:15:20Z

/tip small

substrate-tip-bot · 2024-10-23T20:16:01Z

@ggwpez A referendum for a small (20 DOT) tip was successfully submitted for @Wolfenheimm (148p82cCkxEswcmLyQo4jV6kYP3kMRQusWC6B9wPknXMQEH8 on polkadot).

Referendum number: 1253.

substrate-tip-bot · 2024-10-23T20:16:37Z

The referendum has appeared on Polkassembly.

kianenigma · 2024-10-24T11:14:44Z

Sure, I absolutely agree. Please fix the check-getting-started ci check and I'm happy to approve this PR.

Will our CI use the rust-toolchain.toml file too now?

alvicsam

@Wolfenheimm thanks!

Wolfenheimm · 2024-11-02T14:22:46Z

@kianenigma @alvicsam

Additional note after some digging:

At a later time, we'll have to think deeper about how this toolchain can be applied to the global scale of the project. This PR mainly affects the template starter projects via the getting-started script.

We could have a folder in the root containing a nightly + stable toolchain and use those as the one source of truth, allowing devs/checks/deployments to use them interchangibly according to their need. It would standardize requirements across the board and there would no longer be a need to maintain these settings in the pipelines/project. See: 1, 2, 3, 4.
I'm throwing ideas, this is definitely for a later discussion.

I propose that if all goes well and engineering agrees with this PR's change, we open a new issue that touches this topic.

Chore: Update getting-started.sh to install the specific rust version…

d08a2e2

… specified in .github/env file

Wolfenheimm mentioned this pull request Aug 12, 2024

Startup script: Use the CI rust version of polkadot-sdk #5263

Open

Merge branch 'master' into wolfenheimm/getting-started-rust-version

3f0440b

bkchr requested a review from kianenigma August 12, 2024 18:35

bkchr added the R0-silent Changes should not be mentioned in any release notes label Aug 12, 2024

Wolfenheimm added 5 commits August 12, 2024 18:01

Updated to fall back onto the latest rust version if the required ver…

841a1f4

…sion wasn't found.

Merge branch 'wolfenheimm/getting-started-rust-version' of https://gi…

637c3fb

…thub.com/Wolfenheimm/polkadot-sdk into wolfenheimm/getting-started-rust-version

Uncommented curls, typo fix.

b39b1d0

Added better wording to warning message.

bb0c28d

Fixed another typo...

72d3831

Wolfenheimm added 3 commits August 12, 2024 18:16

linting...

688816e

Set comment back to default since there is now an inner description.

3596805

Merge branch 'master' into wolfenheimm/getting-started-rust-version

abe83e7

kianenigma reviewed Aug 14, 2024

View reviewed changes

kianenigma approved these changes Aug 14, 2024

View reviewed changes

Wolfenheimm requested a review from alvicsam August 15, 2024 23:14

alvicsam approved these changes Aug 16, 2024

View reviewed changes

Merge branch 'master' into wolfenheimm/getting-started-rust-version

6284bb3

github-actions bot requested review from alvicsam and kianenigma August 16, 2024 12:07

Merge branch 'master' into wolfenheimm/getting-started-rust-version

1f6b4c5

kianenigma reviewed Aug 19, 2024

View reviewed changes

Fixed an issue where the version was not properly downloaded

8e53a63

github-actions bot requested a review from kianenigma August 19, 2024 17:28

Wolfenheimm added 2 commits August 19, 2024 13:31

Forgot to put 1.77.0 back in

999ac98

removed redundant else, removed echoes

f708f30

Wolfenheimm added 2 commits October 9, 2024 11:37

Fmt

974310f

Merge branch 'master' into wolfenheimm/getting-started-rust-version

0d5e869

Wolfenheimm added 2 commits October 22, 2024 10:26

Merge branch 'master' into wolfenheimm/getting-started-rust-version

6f34eef

Removed no-longer logic from action

5b5a662

Wolfenheimm requested review from a team as code owners October 22, 2024 14:59

Wolfenheimm added 4 commits October 22, 2024 11:25

Remove Setup the Rust environment from all CI build types as it no lo…

f6b7288

…nger exists

Added components to the toolchain

65ae85e

Merge branch 'master' into wolfenheimm/getting-started-rust-version

508e41e

Merge branch 'master' into wolfenheimm/getting-started-rust-version

506b274

Merge branch 'master' into wolfenheimm/getting-started-rust-version

5bf8c62

ggwpez approved these changes Oct 23, 2024

View reviewed changes

alvicsam approved these changes Oct 24, 2024

View reviewed changes

kianenigma approved these changes Oct 25, 2024

View reviewed changes

nazar-pc mentioned this pull request Oct 29, 2024

IntelliJ Rust plugin can't build runtime #6247

Closed

2 tasks

pgherveou mentioned this pull request Nov 2, 2024

[pallet-revive] revive json-rpc small adjustments #6309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update getting-started.sh to install the specific rust version specified in .github/env file #5317

Update getting-started.sh to install the specific rust version specified in .github/env file #5317

Wolfenheimm commented Aug 12, 2024 •

edited

Loading

cla-bot-2021 bot commented Aug 12, 2024

Wolfenheimm commented Aug 12, 2024 •

edited

Loading

kianenigma Aug 14, 2024

kianenigma Aug 14, 2024

alvicsam Aug 14, 2024

kianenigma Aug 14, 2024

alvicsam Aug 14, 2024

kianenigma left a comment

github-actions bot commented Aug 16, 2024

Wolfenheimm commented Aug 16, 2024

kianenigma Aug 19, 2024

Wolfenheimm Aug 19, 2024 •

edited

Loading

kianenigma Aug 20, 2024

Wolfenheimm Aug 20, 2024 •

edited

Loading

Wolfenheimm Aug 20, 2024

Wolfenheimm commented Oct 10, 2024 •

edited

Loading

Wolfenheimm commented Oct 10, 2024 •

edited

Loading

kianenigma commented Oct 21, 2024

alvicsam commented Oct 22, 2024

Wolfenheimm commented Oct 23, 2024 •

edited

Loading

ggwpez commented Oct 23, 2024

ggwpez commented Oct 23, 2024

substrate-tip-bot bot commented Oct 23, 2024

Wolfenheimm commented Oct 23, 2024 •

edited

Loading

ggwpez commented Oct 23, 2024

substrate-tip-bot bot commented Oct 23, 2024

substrate-tip-bot bot commented Oct 23, 2024

kianenigma commented Oct 24, 2024

alvicsam left a comment

Wolfenheimm commented Nov 2, 2024 •

edited

Loading

Update getting-started.sh to install the specific rust version specified in .github/env file #5317

Are you sure you want to change the base?

Update getting-started.sh to install the specific rust version specified in .github/env file #5317

Conversation

Wolfenheimm commented Aug 12, 2024 • edited Loading

Description

Integration

Review Notes

Checklist

cla-bot-2021 bot commented Aug 12, 2024

Wolfenheimm commented Aug 12, 2024 • edited Loading

Updated the code to include installing the latest version of rust if the required version wasn't found.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 16, 2024

Wolfenheimm commented Aug 16, 2024

Choose a reason for hiding this comment

Wolfenheimm Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wolfenheimm Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wolfenheimm commented Oct 10, 2024 • edited Loading

Wolfenheimm commented Oct 10, 2024 • edited Loading

The readme states:

Env setup

These files will be copied by the installer script to the main directory. They are put into this special directory to not interfere with the normal CI.

kianenigma commented Oct 21, 2024

alvicsam commented Oct 22, 2024

Wolfenheimm commented Oct 23, 2024 • edited Loading

ggwpez commented Oct 23, 2024

ggwpez commented Oct 23, 2024

substrate-tip-bot bot commented Oct 23, 2024

Wolfenheimm commented Oct 23, 2024 • edited Loading

ggwpez commented Oct 23, 2024

substrate-tip-bot bot commented Oct 23, 2024

substrate-tip-bot bot commented Oct 23, 2024

kianenigma commented Oct 24, 2024

alvicsam left a comment

Choose a reason for hiding this comment

Wolfenheimm commented Nov 2, 2024 • edited Loading

Wolfenheimm commented Aug 12, 2024 •

edited

Loading

Wolfenheimm commented Aug 12, 2024 •

edited

Loading

Wolfenheimm Aug 19, 2024 •

edited

Loading

Wolfenheimm Aug 20, 2024 •

edited

Loading

Wolfenheimm commented Oct 10, 2024 •

edited

Loading

Wolfenheimm commented Oct 10, 2024 •

edited

Loading

These files will be copied by the installer script to the main directory. They are
put into this special directory to not interfere with the normal CI.

Wolfenheimm commented Oct 23, 2024 •

edited

Loading

Wolfenheimm commented Oct 23, 2024 •

edited

Loading

Wolfenheimm commented Nov 2, 2024 •

edited

Loading