-
Notifications
You must be signed in to change notification settings - Fork 92
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
feat(binstall): error on missing glibc #494
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the error message isn't very helpful but does describe the problem pretty exactly, and should be searchable on the occasion that someone doesn't understand it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the error message isn't very helpful but does describe the problem pretty exactly, and should be searchable on the occasion that someone doesn't understand it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for thinking about this! The error will make a much better error experience at least for now imo
You mentioned possibly installing glibc as part of rover installation. When you write adebian
compliant installer, for example, you can officially add something like glibc
as a dependency to the control
file which is a very transparent way of doing this sort of installation. Because we pretty much hand roll ours, I find that I'd rather we not automatically install it, just as you have concluded.
If we do end up creating a .deb
or .rpm
archives, we can talk about doing this installation for the users as part of that process.
247a431
to
c1bbd1d
Compare
c1bbd1d
to
284996c
Compare
Fixes #393 - though it doesn't quite do what the issue description describes, as we already do check for unsupported platforms and provide a reasonable error message.
TL;DR: This PR adds a check for
glibc
on linux machines when installing rover and errors if it doesn't exist. it also removes the local install scripts since they are quite redundant, and we can now test things over the internet since github releases actually exist now.Discussion:
Unfortunately, since we added
deno_core
as a dependency to Rover, we no longer build fully static binaries and need to link againstglibc
which exists on most Linux platforms, but not on alpine.If you just download and extract the
rover
binary from GitHub releases and try to execute it, you'll get a super confusing message telling you it can't find the file.If i install
libc6-compat
which claims to improve compatibility withglibc
, I get a slightly different, but still obtuse errors:clearly that's not enough of a workaround for running rover on alpine.
We could install glibc for folks in the curl installer ourselves with an approach like the one outlined in this stackoverflow answer, but this seems like a super bad idea (lol).
that same issue let me know that there is a hard-coded file path for
linux-gnu
executables at/lib/x86_64-linux-gnu/libc.so.6
, so this PR adds a check for that file and provides an error message if it doesn't exist.folks attempting to
curl -sSL https://rover.apollo.dev/nix/latest | sh
on alpine will get an error:ERROR: rover needs to dynamically link against glibc
.this... isn't the most descriptive we could be here, but I assume that folks attempting to use alpine are familiar with shortcomings here. let me know if y'all think we could use a better error message here or if this is sufficient (it's at the very least an improvement!)
also i just want to shout out to WSL for letting me just.. install alpine linux on my windows machine to test this all out without needing docker?? pretty nifty