Skip to content
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

Build musllinux wheels #1392

Merged
merged 2 commits into from
Dec 29, 2021
Merged

Build musllinux wheels #1392

merged 2 commits into from
Dec 29, 2021

Conversation

lithammer
Copy link
Contributor

@lithammer lithammer commented Nov 18, 2021

I'm back for some more musllinux support. Since psycopg2 is still needed for Django, I figured I'd try to add support here as well.

The artifacts can be found here: https://github.com/lithammer/psycopg2/actions/runs/1564148387

@lithammer lithammer marked this pull request as ready for review November 18, 2021 14:43
@dvarrazzo
Copy link
Member

Uhm, on psycopg 3 we build for 3.9 and 3.10 too, then we run tests, which pass. Do you think we should take a closer look at them? We should check the libraries bundled with the binary packages and we can compare the times to pass the test version-to-version to check if we are affected by a regression too. But that's a psycopg 3 matter.

Thinking about psycopg2 instead: do we want to switch to cibuildwheel instead?

@lithammer
Copy link
Contributor Author

lithammer commented Nov 25, 2021

Uhm, on psycopg 3 we build for 3.9 and 3.10 too, then we run tests, which pass. Do you think we should take a closer look at them? We should check the libraries bundled with the binary packages and we can compare the times to pass the test version-to-version to check if we are affected by a regression too. But that's a psycopg 3 matter.

Since it seems like this will be fixed in Alpine in due time, maybe no action is needed for now...?

Thinking about psycopg2 instead: do we want to switch to cibuildwheel instead?

That's probably a good idea. That way it's easier to take advantage of whatever fixes they make to the build process (i.e. just upgrade the GitHub Action).

Do you still want this change meanwhile or should I close it?

@dvarrazzo
Copy link
Member

Do you still want this change meanwhile or should I close it?

It's up to you. Copying and adapting the existing script seems a much easier work. I personally don't wish to spend a lot of work converting the pipeline to cibuildwheel, but I don't want to stop you if you think that it would make easier to generate musl wheels.

@lithammer
Copy link
Contributor Author

lithammer commented Nov 29, 2021

Do you still want this change meanwhile or should I close it?

It's up to you. Copying and adapting the existing script seems a much easier work. I personally don't wish to spend a lot of work converting the pipeline to cibuildwheel, but I don't want to stop you if you think that it would make easier to generate musl wheels.

Ultimately I would like to try and convert to cibuildwheel. But it's hard to say when I'll find the motivation, so I think it's better to merge this now instead of waiting indefinitely for musllinux support. Especially since all upstream Alpine issues have been fixed.

@lithammer
Copy link
Contributor Author

I've removed the Alpine 3.14-3.15 hack now as well.

@lithammer
Copy link
Contributor Author

Gentle nudge @dvarrazzo ☝️

Copy link
Member

@dvarrazzo dvarrazzo left a comment

Choose a reason for hiding this comment

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

I think we should amend the docs somewhere (install? readme?) to declare Alpine linux supported. Please grep around for musl or alpine to check if we can adjust something.

Can you also add an entry to the news file to record that Alpine packages are to be added from 2.9.3?

scripts/build/build_musllinux_1_1.sh Show resolved Hide resolved
scripts/build/build_musllinux_1_1.sh Show resolved Hide resolved
scripts/build/build_musllinux_1_1.sh Show resolved Hide resolved
scripts/build/build_musllinux_1_1.sh Show resolved Hide resolved
scripts/build/build_musllinux_1_1.sh Outdated Show resolved Hide resolved
@dvarrazzo
Copy link
Member

dvarrazzo commented Dec 10, 2021

Gentle nudge @dvarrazzo ☝️

Hi, sorry, I was of the impression that there was still something to do here. Sent you a review with something to fix, then happy to release those binaries.

@lithammer
Copy link
Contributor Author

Gentle nudge @dvarrazzo ☝️

Hi, sorry, I was of the impression that there was still something to do here. Sent you a review with something to fix, then happy to release those binaries.

No worries! I'll look into fixing the review comments.

@lithammer lithammer force-pushed the musllinux branch 2 times, most recently from 8c59e63 to beeaf0a Compare December 10, 2021 15:41
@lithammer
Copy link
Contributor Author

I've fixed everything except this 👇 I couldn't really find a decent place for it. I tried seeing if there was explicit mention of Windows support but couldn't really find something like that either (except for maybe the build status at the bottom of the README).

I think we should amend the docs somewhere (install? readme?) to declare Alpine linux supported. Please grep around for musl or alpine to check if we can adjust something.

The wheels can found here.

PS: Stripping the .so file reduced the size from 1490488 to 313760 on x86_64. Which is quite substantial.

@@ -0,0 +1,68 @@
#!/bin/bash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

diff --git a/scripts/build/build_manylinux_2_24.sh b/scripts/build/build_musllinux_1_1.sh
index 742014ad..62860dcb 100755
--- a/scripts/build/build_manylinux_2_24.sh
+++ b/scripts/build/build_musllinux_1_1.sh
@@ -1,6 +1,6 @@
 #!/bin/bash
 
-# Create manylinux_2_24 wheels for psycopg2
+# Create musllinux_1_1 wheels for psycopg2
 #
 # Look at the .github/workflows/packages.yml file for hints about how to use it.
 
@@ -27,16 +27,16 @@ if [[ "${PACKAGE_NAME:-}" ]]; then
 fi
 
 # Install prerequisite libraries
-curl -k -s https://www.postgresql.org/media/keys/ACCC4CF8.asc | apt-key add -
-echo "deb http://apt.postgresql.org/pub/repos/apt stretch-pgdg main" \
-    > /etc/apt/sources.list.d/pgdg.list
-apt-get -y update
-apt-get install -y libpq-dev
+apk update
+apk add postgresql-dev
+# Add findutils because the Busybox version lacks the `-ls` flag, used by the
+# `strip_wheel.sh` script.
+apk add findutils
 
 # Create the wheel packages
 for pyver in $PYVERS; do
     pybin="/opt/python/${pyver}/bin"
-    "${pybin}/pip" wheel "${prjdir}" -w "${prjdir}/dist/"
+    "${pybin}/python" -m build -w -o "${prjdir}/dist/" "${prjdir}"
 done
 
 # Bundle external shared libraries into the wheels
@@ -45,11 +45,8 @@ for whl in "${prjdir}"/dist/*.whl; do
     auditwheel repair "$whl" -w "$distdir"
 done
 
-# Make sure the libpq is not in the system
-for f in $(find /usr/lib /usr/lib64 -name libpq\*) ; do
-    mkdir -pv "/libpqbak/$(dirname $f)"
-    mv -v "$f" "/libpqbak/$(dirname $f)"
-done
+# Make sure the postgresql-dev is not in the system
+apk del postgresql-dev
 
 # Install packages and test
 cd "${prjdir}"
@@ -69,8 +66,3 @@ for pyver in $PYVERS; do
 
     "${pybin}/python" -c "import tests; tests.unittest.main(defaultTest='tests.test_suite')"
 done
-
-# Restore the libpq packages
-for f in $(cd /libpqbak/ && find . -not -type d); do
-    mv -v "/libpqbak/$f" "/$f"
-done

@lithammer
Copy link
Contributor Author

I have no more changes planned.

@dvarrazzo
Copy link
Member

I apologise, I lost track of this task. Building test packages and merging.

@dvarrazzo dvarrazzo merged commit 94ba067 into psycopg:master Dec 29, 2021
@lithammer
Copy link
Contributor Author

No worries! These are busy times of the year. Thanks for taking your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants