adafruit / Adafruit_Blinka

Add CircuitPython hardware API and libraries to MicroPython & CPython devices
https://learn.adafruit.com/circuitpython-on-raspberrypi-linux
MIT License
453 stars 340 forks source link

don't upload universal bdist to pypi #863

Closed 2bndy5 closed 3 months ago

2bndy5 commented 3 months ago

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.

makermelissa commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

makermelissa commented 2 months ago

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