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

don't upload universal bdist to pypi #863

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

2bndy5
Copy link
Contributor

@2bndy5 2bndy5 commented Jun 20, 2024

In short, bdist (binary distributions) created from a pure python project are universal. They cannot be made to use a platform-specific tag (like linux, windows, macos, etc). This changes the release CI workflow to skip creating/uploading a bdist to PyPI.

This library has a number of dependencies conditionally required per machine attributes (like CPU type). But pip (or other python package managers) will not be aware of such conditional dependencies because a universal bdist do not invoke a dynamic resolution of machine-specific dependencies. Universal bdists only resolve platform-specific dependencies (python version, system OS, etc) which isn't sufficient for this library. Instead, this paradigm is better left to a sdist (source distribution) which carry dependency details per machine-specific attributes in the setup.py file.

Additional context

I discovered this library's bdist had flaws when research a solution for #858. The discussion there led to this PR (as requested). The only workaround for the flawed bdists from PyPI was to have users invoke pip's --no-binary arg or the env var PIP_NO_BINARY.

Fear not Raspberry Pi users! The piwheels project is hosting their own bdists that are built (from the sdist hosted on PyPI) on a network of RPi3 and RPi4 machines each using the 32-bit RPi OS; see status info at piwheels.org/project/adafruit-blinka. The RPi OS is already setup to use the piwheels index, so no special configuration is required.

In short, bdist (binary distributions) created from a pure python project are universal. They cannot be made to use a platform-specific tag (like linux, windows, macos, etc). This changes the release CI workflow to skip creating/uploading a bdist to PyPI.

This library has a number of dependencies conditionally required per *machine* attributes (like CPU type). But pip (or other python package managers) will not be aware of such conditional dependencies because a bdist do not invoke a dynamic resolution of machine-specific dependencies. Universal bdists only resolve platform-specific dependencies (python version, system OS, etc) which isn't sufficient for this library. Instead, this paradigm is better left to a sdist (source distribution) which carry dependency details per machine-specific attributes in the setup.py file.
@2bndy5 2bndy5 changed the title don't upload bdist to pypi don't upload universal bdist to pypi Jun 20, 2024
@makermelissa
Copy link
Collaborator

There are some binaries included for Raspberry Pi such as the libgpiod_pulsein in https://github.com/adafruit/Adafruit_Blinka/tree/main/src/adafruit_blinka/microcontroller/bcm283x/pulseio. Will switching to a non-binary distribution still include these files or will they not be included?

@2bndy5
Copy link
Contributor Author

2bndy5 commented Jun 20, 2024

No, any non-py files should be seen as "package-data", and setuptools should automatically include any package data found in the project's source path.

Proof

  1. Run commands locally in repo root
    pip install build
    python -m build -s
    
  2. Open the built sdist located in dist subfolder
    image
    Observe the path in the address bar corresponds to sub-package path adafruit_blinka.microcontroller.bcm283x.pulseio and notice that all binaries are present in the sdist.
  3. Installing from the sdist will demonstrate how the package-data (binaries) are copied into the env's site-packages folder
    pip install .\dist\adafruit_blinka-8.44.5.dev1.tar.gz
    ls .\.env\Lib\site-packages\adafruit_blinka\microcontroller\bcm283x\pulseio\           
    
    
        Directory: C:\dev\Adafruit_Blinka\.env\Lib\site-packages\adafruit_blinka\microcontroller\bcm283x\pulseio    
    
    
    Mode                 LastWriteTime         Length Name
    ----                 -------------         ------ ----
    d-----         6/20/2024   4:48 PM                __pycache__
    -a----         6/20/2024   4:48 PM          20448 libgpiod_pulsein
    -a----         6/20/2024   4:48 PM            116 libgpiod_pulsein.license
    -a----         6/20/2024   4:48 PM          25832 libgpiod_pulsein64
    -a----         6/20/2024   4:48 PM            116 libgpiod_pulsein64.license
    -a----         6/20/2024   4:48 PM           6504 PulseIn.py
    -a----         6/20/2024   4:48 PM              0 __init__.py
    

@2bndy5
Copy link
Contributor Author

2bndy5 commented Jun 21, 2024

You could use a MANIFEST.in file to explicitly include and/or exclude certain files when assembling the sdist. Typically I would use this to trim down the sdist by excluding the folders .github, docs, tests, and files specific to the dev-workflow (.pre-commit-config.yaml, .readthedocs.yaml, .pylintrc, etc).

I noticed this repo has some submodules located in the test folder. If they're supposed to be part of the distribution, then the release CI should be checking out the repo with submodules initialized

- uses: actions/checkout@v4
  with:
    submodules: true

    # I usually let setuptools_scm have the
    # full git history for creating release assets
    fetch-depth: 0

@2bndy5
Copy link
Contributor Author

2bndy5 commented Jun 21, 2024

You'll also notice that the binary files in question are included in the bdist that piwheels has built (for v8.44.4) from the sdist already present on PyPI (for v8.44.4). I can show more pictures, but I believe it'd be just repetitive.

@2bndy5
Copy link
Contributor Author

2bndy5 commented Jun 21, 2024

here's the important deps specs in the piwheels bdist (for v8.44.4) in the file Adafruit_Blinka-8.44.4.dist-info/METADATA:

Requires-Dist: Adafruit-PlatformDetect (>=3.70.1)
Requires-Dist: Adafruit-PureIO (>=1.1.7)
Requires-Dist: binho-host-adapter (>=0.1.6)
Requires-Dist: pyftdi (>=0.40.0)
Requires-Dist: numpy (>=1.21.5)
Requires-Dist: adafruit-circuitpython-typing
Requires-Dist: RPi.GPIO
Requires-Dist: rpi-ws281x (>=4.0.0)
Requires-Dist: sysv-ipc (>=1.1.0) ; sys_platform == "linux" and platform_machine != "mips"

This is different from the METADATA found in the bdist on PyPI (for v8.44.4):

Requires-Dist: Adafruit-PlatformDetect >=3.70.1
Requires-Dist: Adafruit-PureIO >=1.1.7
Requires-Dist: binho-host-adapter >=0.1.6
Requires-Dist: pyftdi >=0.40.0
Requires-Dist: numpy >=1.21.5
Requires-Dist: adafruit-circuitpython-typing
Requires-Dist: sysv-ipc >=1.1.0 ; sys_platform == "linux" and platform_machine != "mips"

See how a universal bdist could be problematic for certain machines? I suspect that your expected behavior on RPi machines has been entirely dependent on the piwheels project without your knowledge.

Copy link
Collaborator

@makermelissa makermelissa left a comment

Choose a reason for hiding this comment

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

Ok cool. That was my main concern, otherwise I'm good with this.

@makermelissa makermelissa merged commit 69290a7 into adafruit:main Jun 21, 2024
1 check passed
@2bndy5 2bndy5 deleted the do-not-upload-bdist branch June 21, 2024 15:12
@makermelissa
Copy link
Collaborator

Ok, we're now getting complaints of additional requirements needed and very slow installs, so I'm going to revert this.

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