Skip to content

fix: update install script for alpine #3707

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 2, 2021
Merged

fix: update install script for alpine #3707

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a6af17
fix: modify install script for alpine
jsjoeio Jul 1, 2021
908b2c8
fix: use new docs links in install script
jsjoeio Jul 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ Usage:
- If Homebrew is not installed it will install the latest standalone release
into ~/.local

- For FreeBSD, it will install the npm package with yarn or npm.
- For FreeBSD or Alpine, it will install the npm package with yarn or npm.

- If ran on an architecture with no releases, it will install the
npm package with yarn or npm.
Expand Down Expand Up @@ -238,6 +238,17 @@ main() {
return
fi

if [ "$OS" = "linux" ] && [ "$(distro)" = "alpine" ]; then
if [ "$METHOD" = standalone ]; then
echoerr "No precompiled releases available for alpine."
echoerr 'Please rerun without the "--method standalone" flag to install from npm.'
exit 1
fi
echoh "No precompiled releases available for alpine."
install_npm
return
fi

CACHE_DIR="$(echo_cache_dir)"

if [ "$METHOD" = standalone ]; then
Expand Down Expand Up @@ -473,6 +484,11 @@ distro() {
)
return
fi

if [ -f /etc/alpine-release ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@code-asher code-asher Jul 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@code-asher code-asher Jul 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

echo "alpine"
return
fi
}

# os_name prints a pretty human readable name for the OS/Distro.
Expand Down