xplat: Packaging shasum check #3023
Conversation
|
No CI? EDIT: CI showed up a bit late. False alarm |
tools/XPlatInstall/install_ch.sh
Outdated
| PRINT $ERROR_COLOR "] Corrupted binary package." | ||
| PRINT $DEFAULT_COLOR "] Check your network connection." | ||
| PRINT $ERROR_COLOR "] If you suspect from anything else \ | ||
| please reach us at https://github.com/Microsoft/ChakraCore" |
There was a problem hiding this comment.
shall we rather use an email address here?
There was a problem hiding this comment.
Could link to https://github.com/Microsoft/ChakraCore#contact-us
ChakraCore@ exists, but is used as the account email and has a small audience, which is why we previously removed it from the list of ways to contact us.
There was a problem hiding this comment.
good idea, thanks
tools/XPlatInstall/install_ch.sh
Outdated
| @@ -61,12 +60,13 @@ GET_LATEST() | |||
| CHAKRA_VERSION=$(curl -k -s -SL "https://aka.ms/chakracore/version") | |||
|
|
|||
| if [[ "$CHAKRA_VERSION" == *"Error"* ]]; then | |||
| PRINT $ERROR_COLOR "] Download Failed. Do you have an Internet connection?" | |||
| PRINT $ERROR_COLOR "] Download Failed. Do you have Internet connection?" | |||
There was a problem hiding this comment.
Prefer error messages with a clear call to action. Probably change this to: Please ensure you are connected to the internet and try again.
If you want to allow the user to manually verify the connection to the download source you could also add and ensure that ${url} is accessible or ensure that ${domain} is accessible -- to avoid confusing the user if they are connected to the internet but this download still fails, which could be caused by aka.ms or azure being offline.
There was a problem hiding this comment.
network check is beyond the scope of this script but thanks anyway.
| fi | ||
| PRINT $DEFAULT_COLOR "\n] Downloading ChakraCore shasum" | ||
| PRINT $SUCCESS_COLOR "] ${SHASUM_NAME}" | ||
| ___=$(curl -kSL -o "chakracore_s.tar.gz" "${SHASUM_NAME}" 2>&1) |
There was a problem hiding this comment.
This could be a confusing name (extension) because the file is just a plaintext checksum, right?
There was a problem hiding this comment.
no, file is also tar.gzip
| exit 1 | ||
| fi | ||
|
|
||
| PRINT $SUCCESS_COLOR "] Found ChakraCore ${CHAKRA_VERSION//_/.} for ${CURRENT_OS}" | ||
| BINARY_NAME="https://aka.ms/chakracore/${BINARY_NAME}_${CHAKRA_VERSION}.tar.gz" | ||
| BINARY_NAME="https://aka.ms/chakracore/${BINARY_NAME}_${CHAKRA_VERSION}" |
There was a problem hiding this comment.
Does the URL no longer include .tar.gz? Or is that added later on in the script (I didn't see it)?
There was a problem hiding this comment.
aka.ms no longer supports dots.
There was a problem hiding this comment.
Okay, got it. So it's still a .tar.gz file downloaded from a url without dots, and the script saves it with the correct extension.
tools/XPlatInstall/install_ch.sh
Outdated
| CHECK_EXT_FAIL() | ||
| { | ||
| if [[ $? != 0 ]]; then | ||
| PRINT $ERROR_COLOR "] Extracting the compressed file is failed." |
There was a problem hiding this comment.
Failed to extract the compressed file
Or better, pass a parameter with the name of the file and print the error message
Failed to extract '${filename}'
There was a problem hiding this comment.
good idea, will add a todo there.
There was a problem hiding this comment.
actually not needed since we show the filename and contents under this line.
There was a problem hiding this comment.
Okay then. At a minimum change is failed to just failed.
tools/XPlatInstall/install_ch.sh
Outdated
| if [[ ! $SUM1 =~ $SUM2 ]]; then | ||
| PRINT $ERROR_COLOR "] Corrupted binary package." | ||
| PRINT $DEFAULT_COLOR "] Check your network connection." | ||
| PRINT $ERROR_COLOR "] If you suspect from anything else \ |
There was a problem hiding this comment.
Change If you suspect from anything else to If you suspect there is some other problem,
Linux: check if `ld` supports LTO, otherwise build clang toolchain and use that instead
|
cherry picked stale commit into this PR. |
Merge pull request #3023 from obastemur:pkg_fix
Merge pull request #3023 from obastemur:pkg_fix
…um check Merge pull request #3023 from obastemur:pkg_fix
No description provided.