-
Notifications
You must be signed in to change notification settings - Fork 94
Packager improvements #136
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
Changes from all commits
935f4f5
ff0c001
d63870c
c1480f7
797c34b
eabc04c
b125159
22e3623
7e24629
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,43 +56,36 @@ if ! type zip > /dev/null 2>&1; then | |
exit 1 | ||
fi | ||
|
||
function pluck_so_files() { | ||
function find_so_files() { | ||
sed -E '/\.so$|\.so\.[0-9]+$/!d' | ||
} | ||
|
||
function package_libc_alpine() { | ||
# -F matches a fixed string rather than a regex (grep that comes with busybox doesn't know --fixed-strings) | ||
if grep -F "Alpine Linux" < /etc/os-release > /dev/null; then | ||
if type apk > /dev/null 2>&1; then | ||
apk info --contents musl 2>/dev/null | pluck_so_files | sed 's/^/\//' | ||
apk info --contents musl 2>/dev/null | find_so_files | sed 's/^/\//' | ||
fi | ||
fi | ||
} | ||
|
||
function package_libc_pacman() { | ||
if grep --extended-regexp "Arch Linux|Manjaro Linux" < /etc/os-release > /dev/null 2>&1; then | ||
if type pacman > /dev/null 2>&1; then | ||
pacman --query --list --quiet glibc | pluck_so_files | ||
pacman --query --list --quiet glibc | find_so_files | ||
fi | ||
fi | ||
} | ||
|
||
function package_libc_dpkg() { | ||
if type dpkg-query > /dev/null 2>&1; then | ||
architecture=$(dpkg --print-architecture) | ||
if [[ $(dpkg-query --listfiles libc6:$architecture | wc -l) -gt 0 ]]; then | ||
dpkg-query --listfiles libc6:$architecture | pluck_so_files | ||
fi | ||
dpkg-query --listfiles libc6:$(dpkg --print-architecture) | find_so_files | ||
fi | ||
} | ||
|
||
function package_libc_rpm() { | ||
arch=$(uname -m) | ||
|
||
if type rpm > /dev/null 2>&1; then | ||
if [[ $(rpm --query --list glibc.$arch | wc -l) -gt 1 ]]; then | ||
rpm --query --list glibc.$arch | pluck_so_files | ||
fi | ||
rpm --query --list glibc.$(uname -m) | find_so_files | ||
Comment on lines
-90
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. Don't change what is already working. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd still like to propose that the simpler version is more robust. Maybe we can test it to make sure it works. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tested Amazon Linux, Arch Linux, Manjaro, etc.? I don't see the benefits worth the risk here. If you're right, it still works the same, and if you're wrong we end up with a lot of PR churn and less confidence in the project overall. I'm sorry. Please just fix the dpkg arch part and revert the rest. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I would merge this after a careful review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this PR can stay open so people can test it out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With the Alpine changes having been cherry-picked into #137, this PR is now well-scoped into only removing the I think this is merge-able now with the CI added since this comment @marcomagdy - LMK if you see some other risky aspect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ship it. Thank you! |
||
fi | ||
} | ||
|
||
|
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.
:)