Packager improvements #136

specious · 2021-11-26T12:54:51Z

As noted in #131 (comment), on Alpine we have grep implemented by busybox by default, and busybox grep doesn't know -F as --fixed-strings.

This pull request fixes that and also simplifies the logic for getting shared module paths from the system package manager query that lists what the installed libc package contains.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…e query result

…n multiarch

bmoffatt

LGTM, cc @marcomagdy since this is a followup to #131

marcomagdy

Thanks for doing this. Overall LGTM, but we need to be careful with bash changes because we don't have great CI that covers our back.
So, let's take out the unnecessary changes for now.

marcomagdy · 2021-11-30T04:47:59Z

packaging/packager

-        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


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.

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.

marcomagdy · 2021-11-30T04:48:58Z

packaging/packager

-    if grep --fixed-strings "Alpine Linux" < /etc/os-release > /dev/null; then
+    if grep -F "Alpine Linux" < /etc/os-release > /dev/null; then


I'm grudgingly OK with this. But please add a comment. I, for one, don't know what -F stands for and I have to look it up.

In this particular case it seems like the long form name of the flag doesn't explain its function too well either.

I added a comment that explains it: c1480f7

marcomagdy · 2021-11-30T04:49:30Z

packaging/packager

-function pluck_so_files() {
+function find_so_files() {


marcomagdy · 2021-11-30T04:50:17Z

packaging/packager

-    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


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

I'd still like to propose that the simpler version is more robust. Maybe we can test it to make sure it works.

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.

Personally I would merge this after a careful review.

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

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.

Ship it. Thank you!

marcomagdy · 2021-12-01T02:26:53Z

packaging/packager

-    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


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.

* Add demo project * Add build-demo CI job * Revert "Simplified method for picking out shared libraries from system package query result (#136)" This reverts commit 5fb60b9. --------- Co-authored-by: Bryan Moffatt <[email protected]> Co-authored-by: Bryan Moffatt <[email protected]>

specious added 2 commits November 25, 2021 18:46

busybox grep doesn't know -F as --fixed-strings

935f4f5

Simplified method for picking out shared libraries from system packag…

ff0c001

…e query result

specious mentioned this pull request Nov 26, 2021

packager: fetch glibc corresponding to the host architecture on debian multiarch #126

Merged

packager: fetch glibc corresponding to the host architecture on debia…

d63870c

…n multiarch

bmoffatt approved these changes Nov 30, 2021

View reviewed changes

marcomagdy suggested changes Nov 30, 2021

View reviewed changes

Added comment that explains -F flag

c1480f7

bmoffatt mentioned this pull request Nov 30, 2021

Enable alpine ci #137

Merged

marcomagdy suggested changes Dec 1, 2021

View reviewed changes

specious requested a review from bmoffatt December 1, 2021 20:30

bmoffatt added 5 commits December 2, 2021 17:55

Merge branch 'master' into packager-improvements

797c34b

Merge branch 'master' into packager-improvements

eabc04c

Merge branch 'master' into packager-improvements

b125159

Merge branch 'master' into packager-improvements

22e3623

Merge branch 'master' into packager-improvements

7e24629

bmoffatt approved these changes Aug 27, 2022

View reviewed changes

bmoffatt merged commit 5fb60b9 into awslabs:master Aug 29, 2022

bmoffatt mentioned this pull request Feb 27, 2023

Revert "Simplified method for picking out shared libraries from syste… kkarbowiak/aws-lambda-cpp#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packager improvements #136

Packager improvements #136

specious commented Nov 26, 2021

bmoffatt left a comment

marcomagdy left a comment •

edited

Loading

marcomagdy Nov 30, 2021

specious Nov 30, 2021

marcomagdy Nov 30, 2021

specious Nov 30, 2021

marcomagdy Nov 30, 2021

marcomagdy Nov 30, 2021

specious Nov 30, 2021

marcomagdy Dec 1, 2021

specious Dec 1, 2021

specious Dec 1, 2021

bmoffatt Aug 27, 2022

marcomagdy Aug 29, 2022

marcomagdy Dec 1, 2021

		if grep --fixed-strings "Alpine Linux" < /etc/os-release > /dev/null; then
		if grep -F "Alpine Linux" < /etc/os-release > /dev/null; then

Packager improvements #136

Packager improvements #136

Conversation

specious commented Nov 26, 2021

bmoffatt left a comment

Choose a reason for hiding this comment

marcomagdy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcomagdy left a comment •

edited

Loading