-
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 4 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,41 +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() { | ||
if grep --fixed-strings "Alpine Linux" < /etc/os-release > /dev/null; then | ||
# -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 | ||
if [[ $(dpkg-query --listfiles libc6 | wc -l) -gt 0 ]]; then | ||
dpkg-query --listfiles libc6 | pluck_so_files | ||
fi | ||
dpkg-query --listfiles libc6:$(dpkg --print-architecture) | find_so_files | ||
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. If it's not broken let's not try to be clever. I really don't want to have churn in bash files because of some arcane syntax in some distro. We already don't have good CI for different distros, so please let's not change what is currently 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 understand, there's a bit of fragility here and it would be undesirable to introduce a regression. My thoughts were that this is actually a more straightforward (and less hacky) approach in this particular case. Admittedly, my testing wasn't totally comprehensive, but I did some runs with the packager in both a debian and fedora docker container to make sure that it properly does the job here and picks up the shared modules before submitting the PR. To be precise, in the loop further down that iterates over the items, I added: echo "module: $i" So I could see what modules are being picked up. Since it's natural that people will be reproducing this pattern to add support for other distros, taking the time to set a good example seems worth considering. |
||
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.
:)