fix: update install script for alpine #3707
Conversation
|
I don't have access to Alpine 64-bit. @code-asher can you test? (Or is there a way for me to get access to some VM running Alpine 64-bit? |
install.sh
Outdated
| @@ -473,6 +484,11 @@ distro() { | |||
| ) | |||
| return | |||
| fi | |||
|
|
|||
| if [ -f /etc/alpine-release ]; then | |||
There was a problem hiding this comment.
It's too big of a change for this, I just wanted to mention that there's a standard file that contains distro info, os-release, which also seems to work on Alpine:
15:17 coder@jawnsy-m ~/projects/m $ docker run --rm -it fedora cat /etc/os-release | grep "^ID="
ID=fedora
15:17 coder@jawnsy-m ~/projects/m $ docker run --rm -it ubuntu cat /etc/os-release | grep "^ID="
ID=ubuntu
15:17 coder@jawnsy-m ~/projects/m $ docker run --rm -it debian cat /etc/os-release | grep "^ID="
ID=debian
15:17 coder@jawnsy-m ~/projects/m $ docker run --rm -it alpine cat /etc/os-release | grep "^ID="
ID=alpine
Here's the info about it: https://www.freedesktop.org/software/systemd/man/os-release.html
There's also a doc link earlier in the file here, do we want to use the new docs site instead? https://github.com/cdr/code-server/blob/5a6af177256056e316d0056b291fb3a8159c5776/install.sh#L70
There was a problem hiding this comment.
Ooo we use that a few lines up actually. @jsjoeio let's expand the case statement to include alpine instead of checking /etc/alpine-release. Actually I'm not sure why we need the case at all. Let's just return the ID as-is.
There was a problem hiding this comment.
Wait I misread. We only have the case for ID_LIKE then we fall back to ID. So this should already be returning alpine which means no changes are necessary in distro (can remove the /etc/alpine-release block).
There was a problem hiding this comment.
There's also a doc link earlier in the file here, do we want to use the new docs site instead?
Good catch! I'll update those.
There was a problem hiding this comment.
I just wanted to mention that there's a standard file that contains distro info, os-release, which also seems to work on Alpine
TIL that's a thing! Thanks for the tip @jawnsy 🙌
So this should already be returning alpine which means no changes are necessary in distro (can remove the /etc/alpine-release block).
Ah, didn't realize that. Sweet! Less code to add. Removed.
| @@ -419,7 +430,7 @@ install_npm() { | |||
| echoh | |||
| echoerr "Please install npm or yarn to install code-server!" | |||
| echoerr "You will need at least node v12 and a few C dependencies." | |||
| echoerr "See the docs https://github.com/cdr/code-server/blob/v3.10.2/docs/install.md#yarn-npm" | |||
| echoerr "See the docs https://coder.com/docs/code-server/v3.10.2/install#yarn-npm" | |||
There was a problem hiding this comment.
Are you planning to bump the version number each release, or should we link to /latest/ here?
There was a problem hiding this comment.
If so, I think we should replace latest with the version number in this case actually so that old docs will link to the correct branch.
I think @bpmct and @BrunoQuaresma thought we should use the version (source).
We do have a script called release-prep.sh which handles updating these links for us. See here
There was a problem hiding this comment.
Yes, we want to keep the version so the script release-prep can handle the update for us.
Codecov Report
@@ Coverage Diff @@
## main #3707 +/- ##
=======================================
Coverage 61.55% 61.55%
=======================================
Files 35 35
Lines 1813 1813
Branches 365 365
=======================================
Hits 1116 1116
Misses 588 588
Partials 109 109 Continue to review full report at Codecov.
|
jsjoeio
force-pushed
the
jsjoeio-fix-alpine-install-script
branch
from
686ecd8 to
908b2c8
Compare
| @@ -2,7 +2,7 @@ | |||
| set -eu | |||
|
|
|||
| # code-server's automatic install script. | |||
| # See https://github.com/cdr/code-server/blob/main/docs/install.md | |||
| # See https://github.com/cdr/code-server/blob/main/docs/install | |||
There was a problem hiding this comment.
Should this also go to coder.com? Without .md it seems to 404.
There was a problem hiding this comment.
Dang it. I did a serach for .md and must have accidentally done a search and replace. I'll fix.
This PR fixes the install script to work on Alpine 64-bit. Our
archfunction doesn't recognize 32-bit Alpine, which fallsback tonpm/yarninstall (which is why it works on 32-bit Alpine), but tries to use the standalone install for 64-bit, which doesn't work.This fixes that.
Fixes #3421
Also, thank you to @b-trav for filing an issue about the broken links in
install.sh!