Skip to content

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

Merged
merged 9 commits into from
Aug 29, 2022
17 changes: 5 additions & 12 deletions packaging/packager
Original file line number Diff line number Diff line change
Expand Up @@ -56,43 +56,36 @@ if ! type zip > /dev/null 2>&1; then
exit 1
fi

function pluck_so_files() {
function find_so_files() {
Copy link
Contributor

Choose a reason for hiding this comment

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

:)

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. Don't change what is already working.

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'd still like to propose that the simpler version is more robust. Maybe we can test it to make sure it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I would merge this after a careful review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps this PR can stay open so people can test it out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With the Alpine changes having been cherry-picked into #137, this PR is now well-scoped into only removing the if [[ and wc -l guards on "RHEL-like" and "Debian-like" distros, making the logic consistent with "Arch-like" and Alpine. In my understanding, these guards were redundant with the pipelining to find_so_files.

I think this is merge-able now with the CI added since this comment @marcomagdy - LMK if you see some other risky aspect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ship it. Thank you!

fi
}

Expand Down