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

Review get-substrate.sh Script #218

Closed
danforbes opened this issue Jul 28, 2020 · 12 comments
Closed

Review get-substrate.sh Script #218

danforbes opened this issue Jul 28, 2020 · 12 comments
Assignees

Comments

@danforbes
Copy link
Contributor

According to @bkchr, the get-substrate.sh script is likely very out-of-date. It may be worth reevaluating its value and soundness. For starters, notice that the following line installs git twice:
https://github.com/paritytech/scripts/blob/master/get-substrate.sh#L35

@danforbes danforbes self-assigned this Jul 28, 2020
@danforbes
Copy link
Contributor Author

See here for more discussion paritytech/polkadot#1479

@riusricardo
Copy link
Contributor

riusricardo commented Jul 28, 2020

According to @bkchr, the get-substrate.sh script is likely very out-of-date. It may be worth reevaluating its value and soundness. For starters, notice that the following line installs git twice:
https://github.com/paritytech/scripts/blob/master/get-substrate.sh#L35

I remember that some VMs don't have git as default deployment. This is why it is part of the packages, it won't do a thing if it is already installed.

EDIT: I see that it is twice 🤦

@riusricardo
Copy link
Contributor

I agree with @bkchr, we should deprecate the script.

@danforbes
Copy link
Contributor Author

Ricardo, other than Knowledge Base, Tutorials, Recipes and Node Template do we reference this script anywhere?

@danforbes
Copy link
Contributor Author

I checked on the Polkadot Wiki and didn't see it there but admittedly I didn't look too hard.

@danforbes
Copy link
Contributor Author

@joepetrowski do you know of any other places where the get-substrate.sh script may appear?

@riusricardo
Copy link
Contributor

We should keep it in the repo and fix it but stop referencing and encouraging it. IMO, we don't want to break workshops/videos that used this method.

@thiolliere
Copy link

when latest rust nightly breaks substrate this script will fail, when this happens some ppl have no clue how to go from it.

At least when case cargo build fails, we should print a message pointing to some url which explains the complete process to install substrate.

(Also we could improve by setting a specific nightly toolchain but then we should make some CI to be sure this nightly doesn't get too old, but I would rather deprecate with some more message for ppl stuck)

@TriplEight
Copy link
Contributor

FWIW it makes sense to test this script nightly in CI.
And btw, why it's still here and not in the substrate repo?

@riusricardo
Copy link
Contributor

And btw, why it's still here and not in the substrate repo?

@TriplEight We are deprecating the script. @thiolliere suggestions are good and we should print information stating this and where they should find the latest installation instructions.

@nuke-web3
Copy link

I have #253 opened for those interested in updating/improving/expanding 🤞🏼 this script

@rcny
Copy link
Contributor

rcny commented May 18, 2023

Closing as stale.

@rcny rcny closed this as not planned Won't fix, can't repro, duplicate, stale May 18, 2023
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

No branches or pull requests

6 participants