Skip to content
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.

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

Alpine linux tuning #1337

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

PeterDaveHello
Copy link
Collaborator

There is no binary for Alpine Linux right now, should set nobinary=1 right now, could do the same thing as Solaris to determine if there is binary since certain version.

@@ -1443,7 +1443,13 @@ nvm_get_os() {
NVM_UNAME="$(command uname -a)"
local NVM_OS
case "$NVM_UNAME" in
Linux\ *) NVM_OS=linux ;;
Linux\ *)
if [ -f "/etc/alpine-release" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

this seems like a brittle test - couldn't there be an Alpine without this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think someone will try to remove that file, I tested on Alpine 3.3, 3.4 and edge image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about using /etc/issue? Though I see no reason to touch them both.

FYR:

$ for ver in 2.6 2.7 3.1 3.2 3.3 3.4 3.5 3.6 3.7 3.8; do docker run --rm -it alpine:$ver grep 'Alpine Linux' /etc/issue; done
Welcome to Alpine Linux 2.6
Welcome to Alpine Linux 2.7
Welcome to Alpine Linux 3.1
Welcome to Alpine Linux 3.2
Welcome to Alpine Linux 3.3
Welcome to Alpine Linux 3.4
Welcome to Alpine Linux 3.5
Welcome to Alpine Linux 3.6
Welcome to Alpine Linux 3.7
Welcome to Alpine Linux 3.8

nvm.sh Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Nov 30, 2016

In general, all this check does, on FreeBSD (and now Alpine) is skip a single http request - I'm kind of inclined to just remove the check, and always check for a binary.

@PeterDaveHello
Copy link
Collaborator Author

As we can see right now, there may never be a FreeBSD binary at the moment, so the binary check for FreeBSD will be meaningless but make nvm slower, I believe that Alpine part will still need a while and may not have the builds for old versions , we can change the code to the current solaris style to do the old versions check locally and check the new ones online, what do you think?

@PeterDaveHello
Copy link
Collaborator Author

@ljharb are you good with current pattern just like FreeBSD and would be like solaris? I updated the PR :D

@ljharb
Copy link
Member

ljharb commented Dec 4, 2016

I think a better approach would be removing this check entirely, and improving the error message from this:

Downloading https://nodejs.org/dist/v0.10.10/node-v0.10.10-freebsd-x64.tar.gz...
######################################################################## 100.0%
Computing checksum with shasum -a 256
Provided checksum to compare to is empty.
tar: Unrecognized archive format
tar: Error exit delayed from previous errors.
Binary download failed, trying source.

to determine when it's a 404.

I think the extra cost of an http request to determine if there is a binary archive is worth it, on both freebsd and alpine. (that said, i'm still totally down with adding alpine detection to nvm_get_os, even if we don't have a use for it yet)

@PeterDaveHello
Copy link
Collaborator Author

Okay, let me work on this.

@PeterDaveHello
Copy link
Collaborator Author

@ljharb one critical problem, we don't know the naming at all, and they probably will never release binary package, could we just do that once the binary naming is out? I think it's not only about the overhead.

@ljharb
Copy link
Member

ljharb commented Dec 4, 2016

I'm not sure why you're thinking they never will release one - both of those are under active discussion right now.

Either way I'd prefer to just pick a name and delegate the authority to nodejs.org, since that's easier to update than nvm is.

@PeterDaveHello
Copy link
Collaborator Author

I think that's not the BSD style, as you can see in our very early discussion. So would you like to just ask them about the name?

@PeterDaveHello
Copy link
Collaborator Author

Can we just make this work and once we have some conclusion on the naming, then make a new change? Maybe we don't need to stuck by that :)

@ljharb
Copy link
Member

ljharb commented Dec 15, 2016

Is this particularly urgent? The only effect I see is that nvm install will be slightly faster on alpine linux.

@PeterDaveHello
Copy link
Collaborator Author

Not urgent at all but I think maybe we don't need to stuck for that reason :)

@PeterDaveHello
Copy link
Collaborator Author

Just wondering if an OS being supported can be found in the index.tab file instead of determined by 404?

@ljharb
Copy link
Member

ljharb commented Jan 21, 2019

@PeterDaveHello hm, that sounds like an awesome idea for an improvement - i'm not sure if the index.tab properly lists all files that are actually present though. Worth looking into!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants