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

Fixes #259: Finish Feature Request by Adding Support for musllinux Wheels and Testing Script for Alpine Compatibility #261

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Pxli9130
Copy link

@Pxli9130 Pxli9130 commented Nov 24, 2024

Fixes #259

Description:

This pull request introduces support for building musllinux wheels, enabling users on Alpine Linux and other musl-based systems to install timezonefinder without the need for compiling from source. It also adds a testing script to verify the package's compatibility with Alpine Linux.


Changes Introduced:

  1. Updated pyproject.toml:

    • Added cibuildwheel as a development dependency.
    • This facilitates building wheels for multiple platforms, including musllinux wheels.
  2. Added GitHub Actions Workflow make-musllinux-wheels:

    • Created a new workflow to build musllinux wheels using cibuildwheel.
    • The workflow targets Python versions 3.8 to 3.12, matching the package's supported Python versions.
    • Musllinux wheels are now automatically built and can be uploaded during the release process.
  3. Updated poetry.lock:

    • Reflects changes made to pyproject.toml by updating the lock file.
    • Ensures that all dependencies are correctly locked for reproducible builds.
  4. Added Testing Script test_musllinux_wheel.sh:

    • Introduced a script to test the musllinux wheels in an Alpine Linux Docker container.
    • Automates the process of verifying that the wheels work correctly on musl-based systems.
    • Instructions for using the script are included in the updated contributing guide.

Benefits:

  • Enhanced Compatibility:

    • Users on Alpine Linux and other musl-based distributions can install pre-built wheels.
    • Eliminates the need for users to compile the package from source, which can be challenging due to compilation dependencies.
  • Improved Developer Experience:

    • Simplifies the process for contributors to test the package on musl-based systems.
    • Encourages wider testing and adoption by providing easy-to-use tools.
  • Automated Builds:

    • Ensures that musllinux wheels are consistently built and available with each release.
    • Reduces manual effort required to produce and distribute wheels for different environments.

Testing Performed:

  • Built musllinux Wheels:

    • Successfully built musllinux wheels for Python versions 3.8 to 3.12 using the updated GitHub Actions workflow.
  • Verified Functionality:

    • Used the test_musllinux_wheel.sh script to install and test the musllinux wheels in an Alpine Linux Docker container.
    • Confirmed that the package installs without errors and functions correctly by returning the expected timezone data.

Request for Review (Nov 23):

I kindly request the maintainers to review these changes. The addition of musllinux wheel support and the testing script should enhance the usability of timezonefinder for users on musl-based systems and streamline the development process for contributors.

Please let me know if there are any questions or if further adjustments are needed. I'm happy to collaborate to ensure these changes align with the project's goals and standards.


Request for Review (Nov 25):

I have made the requested changes based on your feedback:

  • Combined the Wheel-Building Jobs:

    • Merged the make-wheels and make-musllinux-wheels jobs into a single make-wheels job.
    • The job now builds both manylinux and musllinux wheels using [email protected].
  • Ensured Code Consistency:

    • Both manylinux and musllinux wheels are built using the same cibuildwheel action and configuration.
  • Defined Python Versions Globally:

    • Introduced a global environment variable CIBW_BUILD_VERSIONS to specify the supported Python versions, right now it's Python 3.8 to 3.13.
    • Now, the Python versions only need to be updated in one place, preventing inconsistencies.

Please review the updated changes. I believe these adjustments enhance the build process and align with the project's goals. Let me know if there's anything else you'd like me to adjust. I'm happy to collaborate to ensure these changes meet your expectations.


Request for Review (Nov 29):

I have addressed the issue based on the feedback, where the musllinux builds failed due to the use of yum, which is unavailable in musllinux (Alpine-based) images.

Changes:

  • Updated build.yml to use apk add for musllinux builds instead of yum.
  • Introduced separate pre-build commands for manylinux (yum) and musllinux (apk) to ensure compatibility with their respective environments.

These changes ensure that the musllinux builds work seamlessly, allowing the project to support musl-based systems effectively. The relevant changes for this issue are included in commit bd26666.

Please review the updates. I believe these changes further improve the build process and enhance compatibility for timezonefinder. Let me know if there are any questions or additional feedback!


Thank you for considering this contribution!

@Pxli9130 Pxli9130 changed the title Add Support for musllinux Wheels and Testing Script for Alpine Compatibility Fixes #259: Add Support for musllinux Wheels and Testing Script for Alpine Compatibility Nov 24, 2024
@Pxli9130 Pxli9130 changed the title Fixes #259: Add Support for musllinux Wheels and Testing Script for Alpine Compatibility Fixes #259: Finish Feature Request by Adding Support for musllinux Wheels and Testing Script for Alpine Compatibility Nov 24, 2024
@jannikmi
Copy link
Owner

Thank you for the contribution and the nice summary of changes.
I have a couple of questions:

  • at the first glimpse the introduced build job looks very similar to the make-wheels job. Can the two jobs be combined into one?
  • if not, could you please modify the make-musllinux-wheels such that the code matches the one used in the make-wheels job? (for example use uses: pypa/[email protected])
  • I am afraid that the supported python versions will run out of sync with new versions >3.13, when one forgets to update the versions in all of the different places. Could you please find out if cibw_build: ["cp38-* ... cp313-*"] can be defined globally (something like a constant) in the GitHub Actions workflow .yaml file?

Thank you

@Pxli9130
Copy link
Author

@jannikmi hi, thanks for the review! I've merged the make-musllinux-wheels job into the existing make-wheels job. This ensures code consistency and uses pypa/[email protected]. And i've defined CIBW_BUILD_VERSIONS globally with required py versions. Could u take a look?

@jannikmi
Copy link
Owner

@Pxli9130 Thanks, that now looks a lot cleaner (less repetition).

Could you please have a look at the actions run:
https://github.com/jannikmi/timezonefinder/actions/runs/12019546358/job/33508601975
For some reason this command fails:

sh -c 'yum install -y libffi-devel clang make'
  sh: yum: not found

Could you please look into this? Why is yum missing? It has been used before.

@Pxli9130
Copy link
Author

@Pxli9130 Thanks, that now looks a lot cleaner (less repetition).

Could you please have a look at the actions run: https://github.com/jannikmi/timezonefinder/actions/runs/12019546358/job/33508601975 For some reason this command fails:

sh -c 'yum install -y libffi-devel clang make'
  sh: yum: not found

Could you please look into this? Why is yum missing? It has been used before.

sure, i'll check it

Pxli9130 and others added 2 commits November 29, 2024 10:15
…is is caused by the absence of `yum` in Alpine-based musllinux images
@Pxli9130
Copy link
Author

@Pxli9130 Thanks, that now looks a lot cleaner (less repetition).

Could you please have a look at the actions run: https://github.com/jannikmi/timezonefinder/actions/runs/12019546358/job/33508601975 For some reason this command fails:

sh -c 'yum install -y libffi-devel clang make'
  sh: yum: not found

Could you please look into this? Why is yum missing? It has been used before.

@jannikmi The issue occurs because the musllinux Docker images used in the build are based on Alpine Linux, which uses apk as the package manager instead of yum. Previously, when using manylinux images (CentOS-based), yum was available. To fix this, it needs to update the build.yml to use apk add for musllinux builds.The changes for this issue were addressed in commit bd26666.

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.

Provide musllinux wheel
2 participants