[Fix] nvm_get_arch: proper value for alpine linux
#3212
Conversation
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
How do you want to do it, modify existing ones, or create a separate test in tests folder? |
|
@asolopovas please make a new file for each test |
|
@ljharb how can I create test for |
|
There won't be an |
As far as I know #!/bin/bash
OS=$(awk '/^ID=/' /etc/os-release | sed -e 's/ID=//' -e 's/"//g' | tr '[:upper:]' '[:lower:]')
NODE_VERSION=${NODE_VERSION:-18.18.0}
NVM_VERSION=${NVM_VERSION:-v0.39.5}
NVM_DIR="$HOME/.nvm"
FORCE=${FORCE:-false}
if [ "$FORCE" = "true" ]; then
echo "Force install nodejs"
rm -rf $NVM_DIR
fi
PROFILE=/dev/null bash -c "curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/${NVM_VERSION}/install.sh | bash"
if [ "$OS" = "alpine" ]; then
chmod +x $NVM_DIR/nvm.sh
sed -i '/nvm_get_arch() {/,/^}$/c\nvm_get_arch() { nvm_echo "x64-musl"; }' $NVM_DIR/nvm.sh
fi
source "$NVM_DIR/nvm.sh"
nvm install $NODE_VERSIONso assuming if NVM_NODEJS_ORG_MIRROR=https://unofficial-builds.nodejs.org/download/release, the download file will correctly resolve at https://unofficial-builds.nodejs.org/download/release/v18.18.0/node-v18.18.0-linux-x64-musl.tar.xz |
|
Sorry, let me clarify. Here's the 4 tests I'd expect:
That way, the tests should pass on both a non-alpine and an alpine machine. |
|
Thanks. I think I understood you now |
|
I managed to write test both to test in fake file system with chroot as well as I did an example where it tries to retrive |
|
The failing tests suggest that this change actually breaks installation on alpine :-/ |
It shouldn't I didn't touch the test suite but it should contain env variable NVM_NODEJS_ORG_MIRROR=https://unofficial-builds.nodejs.org/download/release for tests to pass. Do you want me to look into it? |
|
The issue is that |
Ok, I think I found the culprit, I mixed musl-x64 with x64-musl even though had it correct in my script don't know how I missed it. tests and nvm.sh: as to the test in order to modify test I need to edit your |
|
Modifying existing tests makes it a breaking change; if official node versions can be installed in alpine then that's not something we can break. |
|
its not exactly breaking anything this should clarify what I mean: wsl_matrix:
name: 'WSL nvm install'
defaults:
run:
shell: wsl-bash {0}
runs-on: windows-latest
env:
WSLENV: NVM_INSTALL_GITHUB_REPO:NVM_INSTALL_VERSION:/p
strategy:
fail-fast: false
matrix:
wsl-distrib:
- Debian
- Ubuntu-18.04
npm-node-version:
- '--lts'
- '14'
- '12'
- '11'
- '10'
method:
- ''
- 'script'
steps:
- uses: Vampire/setup-wsl@v1
with:
distribution: ${{ matrix.wsl-distrib }}
additional-packages: bash git curl ca-certificates wget
- name: Retrieve nvm on WSL
run: |
if [ -z "${{ matrix.method }}" ]; then
curl -fsSLo- "https://raw.githubusercontent.com/${NVM_INSTALL_GITHUB_REPO}/${NVM_INSTALL_VERSION}/install.sh" | bash
else
curl -fsSLo- "https://raw.githubusercontent.com/${NVM_INSTALL_GITHUB_REPO}/${NVM_INSTALL_VERSION}/install.sh" | METHOD="${{matrix.method}}" bash
fi
. "$HOME/.nvm/nvm.sh"
nvm install ${{ matrix.npm-node-version }}
wsl_matrix_unofficial:
name: 'WSL nvm install'
defaults:
run:
shell: wsl-bash {0}
runs-on: windows-latest
env:
WSLENV: NVM_INSTALL_GITHUB_REPO:NVM_INSTALL_VERSION:/p
NVM_NODEJS_ORG_MIRROR: https://unofficial-builds.nodejs.org/download/release
strategy:
fail-fast: false
matrix:
wsl-distrib:
- Alpine
npm-node-version:
- '--lts'
- '14'
- '12'
- '11'
- '10'
method:
- ''
- 'script'
steps:
- uses: Vampire/setup-wsl@v1
with:
distribution: ${{ matrix.wsl-distrib }}
additional-packages: bash git curl ca-certificates wget
- name: Retrieve nvm on WSL
run: |
if [ -z "${{ matrix.method }}" ]; then
curl -fsSLo- "https://raw.githubusercontent.com/${NVM_INSTALL_GITHUB_REPO}/${NVM_INSTALL_VERSION}/install.sh" | bash
else
curl -fsSLo- "https://raw.githubusercontent.com/${NVM_INSTALL_GITHUB_REPO}/${NVM_INSTALL_VERSION}/install.sh" | METHOD="${{matrix.method}}" bash
fi
. "$HOME/.nvm/nvm.sh"
nvm install ${{ matrix.npm-node-version }}because Alpine supported only with unofficial NVM_NODEJS_ORG_MIRROR in order not to change anything in Ubuntu and Debian we have to test it separately. |
|
If I'm understanding right, you want to stop testing the official mirror on alpine, and test the unofficial one instead. i'm saying that installs currently work with the official mirror, which is why that's what's tested, and that needs to continue to work. |
|
official mirror does not support alpine. it hasn't got the musl build. |
|
so what is the point of testing it against official mirror if build for alpine does not exist in it? |
|
the tests currently successfully download and install a version of node on alpine. Does that node version not work? |
|
in reality it would be good to support the hole list of them, even they are unofficial they are built and hosted by nodejs why should it matter If I want to use alpine for my docker thats the only way I get it if not build it from source : |
|
I don't know how test can be successful if Alpine test can't pass without musl built. |
|
I understand that you're saying "without compiling", but if it currently works with compiling, then that's functionality we can't break. |
|
hmm, i'm trying out verifying in these tests that |
|
your test failing not because it doesn't build it but because it trys to pull it from wrong location, with separate test it will not be failing but passing: |
|
right, i understand that with your change, and the unofficial mirror, the tests will pass. i'm trying to understand why the fact that your change makes the existing tests fail, is acceptable. is that perhaps because currently, with the official mirror, nvm installs node fine except that the node it installs ends up broken and unusable? |
|
this is what it does currently on alpine docker images: nvm install 18.18.0 |
|
in docker, scoping can get weird; what if you run |
|
bash-5.1# pwd |
|
look I been using node in alpine for very long time it does not work with official version and official download, it only works with musl builds |
|
and by the way node file has executable permissions it just it throws this strange error |
|
Thanks for bearing with me, I'm just trying to understand, since I don't use alpine :-) It seems like I'll be removing those Alpine tests on master, and then rebasing this PR, which should then pass. I'll also add alpine tests with the unofficial mirror as part of rebasing. |
|
No problem at all, I have already updated everything if you want I can push it? |
|
Thanks, that'd be great! |
|
tests still are not picking up mirror var |
ljharb
changed the title
nvm_get_arch: proper value for alpine linux
@ljharb I have updated, nvm_get_arch to return proper value for alpine, please let me know if that would work.