xJonathanLEI / starkli

Starkli (/ˈstɑːrklaɪ/), a ⚡ blazing ⚡ fast ⚡ CLI tool for Starknet powered by 🦀 starknet-rs 🦀
https://book.starkli.rs
Apache License 2.0
164 stars 47 forks source link

feat(starkliup): install specific version #8

Closed glihm closed 1 year ago

glihm commented 1 year ago

The current PR is motivated by the fact that different versions may propose different compiler versions, and the versions are not exhaustive as we depend on the cairo-starknet-xxx versioning.

This PR proposes the use of -v|--version option + add fallback to wget if curl is not supported.

The proposed behavior is the following:

# As before, by default the latest version if fetched and installed.
$ starkliup

# Specifying a tag, which will fail if the tag is invalid.
$ starkliup --version v0.1.1
$ starkliup -v v0.1.1

# This is supported too.
$ starkliup -v latest 

We can remove the v and only ask the use 0.1.2 for instance.

@xJonathanLEI WDYT?

glihm commented 1 year ago

As the download was re-factorized in a function, I found it convenient to add two lines in order to support wget as mentioned as a TODO in the code.

To keep the PR related to the version only, I'll drop those lines as you mentioned.

xJonathanLEI commented 1 year ago

I meant simply restore the diffs added for the wget support in the first place, not just removing the wget lines from the final diffs.

I prefer keeping PRs simple and do one thing only.

glihm commented 1 year ago

I meant simply restore the diffs added for the wget support in the first place, not just removing the wget lines from the final diffs.

I prefer keeping PRs simple and do one thing only.

Got you, thanks for the feedback I'll take care of that. :+1:

However, regarding curl, in the previous implementation there were no error if the version wasn't found. That's why in the download method there is error handling a bit different from the original code.

curl -# -L https://github.com/xJonathanLEI/starkli/releases/download/v8.8.8/starkli-aarch64-unknown-linux-gnu.tar.gz -o /tmp/test

This command will not fail, the $? is 0. However, the content of /tmp/test is a text file with Not Found inside.

For this reason I moved to a function to handle the download and associated errors and next evolution of the code related to download.

Do you want me to don't use the function and only add the appropriate error handling in place? But IMHO we do need to notify the user if the given version is not found.

xJonathanLEI commented 1 year ago

Oh for that you can just use the --fail flag of curl I think.

I forgot to add the flag cuz that URL always exists.

xJonathanLEI commented 1 year ago

So maybe just restore and add that flag? (unless if that flag isn't sufficient in detecting error)

glihm commented 1 year ago

Take an example with the following command:

starkliup --version v8.0.8

The --fail flag used like this:

curl -# --fail -L $FILE_URL -o $TEMP_FILE_NAME

causes the program to exit with the following error printed to the user:

curl: (22) The requested URL returned error: 404

And we can't add any echo after it as the program exits on curl fail. Where the current download_release_file method provides:

  1. A check that curl is installed
  2. A more comprehensive error message:
    Couldn't download release file for tag 'v8.0.8' from GitHub [404].

So we may keep the download function to output a better error message, if curl is not installed and also if the version tag is not correct. :+1: