Skip to content

Build distributions action#165

Merged
jared321 merged 31 commits intomainfrom
164BuildWheels
Aug 20, 2025
Merged

Build distributions action#165
jared321 merged 31 commits intomainfrom
164BuildWheels

Conversation

@jared321
Copy link
Contributor

@jared321 jared321 commented Aug 15, 2025

Self-review tasks:

  • Careful review of all changes
  • There was already ample confirmation that the action fails on build failures
  • Intentionally break code so that a test will fail and alter action so that only make-sdist is running tests. Confirm that make-sdist correctly detects and reports test failure.
  • Intentionally break code so that a test will fail and alter action so that only build-wheels is running tests. Confirm that build-wheels correctly detects and reports test failure.
  • Spot check logs of new action to determine that all wheels are being built and tested in an expected way
  • Download artifacts, inspect contents (e.g., does it include external dependencies such as libraries), and manually install and test
    * Tested Python 3.9 to 3.13 on ARM64 macOS. Compiled Cython code only depends on OS libraries according to otool -L
    * Tested Python 3.10 and 3.11 on RedHat Linux 8.10 with x86_64 architecture. Compiled Cython code only depends on OS libraries according to ldd
    * Tested Python 3.10 on Ubuntu 22.04 with x86_64 architecture. Compiled Cython code only depends on OS libraries according to ldd.
  • Ensure that release.yml has been altered so that it only runs on pushes to main
  • Confirm that all actions are passing

@mosesyhc to review. Upload these to Test-PyPI as part of review or wait until next release? The last wheels automatically built are available as artifacts at
https://github.com/bandframework/surmise/actions/runs/17049773899

@jared321 jared321 self-assigned this Aug 15, 2025
@jared321 jared321 changed the title Build distributions action DRAFT: Build distributions action Aug 15, 2025
@jared321 jared321 requested a review from mosesyhc August 18, 2025 22:08
@jared321 jared321 changed the title DRAFT: Build distributions action Build distributions action Aug 18, 2025
@mosesyhc
Copy link
Member

mosesyhc commented Aug 19, 2025

MC:

  • Downloaded artifacts, inspect contents
    • Tested Python 3.11 and 3.12 on Windows 11 OS x64. Compiled Cython code only depends on OS libraries / DLLs, shown by lucasg/Dependencies tool.
    • Noted that Python 3.13 wheel is not built currently.
  • Test upload sample artifacts (from Python 3.11 and source) to test.pypi.org/
    • surmise-0.1.dev1+g6deb4cd0a-cp311-cp311-macosx_10_9_x86_64.whl
    • surmise-0.1.dev1+g6deb4cd0a-cp311-cp311-macosx_11_0_arm64.whl
    • surmise-0.1.dev1+g6deb4cd0a-cp311-cp311-manylinux2014_x86_64.manylinux_2_17_x86_64.manylinux_2_28_x86_64.whl
    • surmise-0.1.dev1+g6deb4cd0a-cp311-cp311-win_amd64.whl
    • surmise-0.1.dev1+g6deb4cd0a.tar.gz
  • During the checking of the wheels and source files with twine, the uploading tool for PyPA/PyPI, using twine check dist/*, the following errors are revealed:
    • The produced wheels contains an incorrect version. I expect it to at least be v0.3.1+dev, instead it shows 0.1+dev.
      • @jared321 Why is the built surmise version 0.1? Is it respecting any particular autoversioning?
    • The wheels did not pass twine check.
      • In setup.py, the long_description_content_type should specify x-rst instead of rst. see #0c35687. The documentation files are inspected and verified to successfully compile and render via tox -e html.
      • After fixing the x-rst error raised by twine, a new wheel is built with tox and checked with twine check again. Now the following error shows up with PyPI rst rendering. The same error shows for all newly built wheels and source (Error pasted at the end to avoid markdown interruption)
      • Tested several things, to no avail, including
        • double-checking tab vs space characters
        • adding ; charset=UTF-8 after text/x-rst
        • checking with docutils locally to verify html rendering
        • unzipping the tar.gz file produced from python -m build --sdist --wheel to examine the description file
    Checking dist/surmise-0.3.1.dev94+gcfcf17b7c.d20250819.tar.gz: FAILED
    ERROR    `long_description` has syntax errors in markup and would not be
             rendered on PyPI.
             line 7: Error: Error in "image" directive:
             no content permitted.
    
             .. image:: https://badge.fury.io/py/surmise.svg
    
                :target: https://badge.fury.io/py/surmise

setuptools-scm uses tags in the project's clone to automatically determine the
version.  It appears that the default shallow clone did not include tags.  This
fix was present in publish.yml, which suggests that release.yml will hopefully
build distributions with the correct version.
@mosesyhc
Copy link
Member

Putting a note here about setuptools version impacting the autoversioning in building the correct release version. In another package mosesyhc/LCGP I needed to upgrade setuptools to setuptools>=80 to correctly build the release version.

The wheels were building with the expected version string in the GH action.
@jared321
Copy link
Contributor Author

jared321 commented Aug 19, 2025

The version issue was with release.yml. It was setup incorrectly so that the checkout action cloned the surmise repo with the default setup, which installs a shallow clone. In particular, I understand that it was not cloning tag information. Without this, it appears that setuptools-scm was making a default guess at the version string.

I fixed release.yml so that checkout (hopefully) installs a full clone. This is, indeed, what is done in test-publish.yml, for example.

I now see the correct version for the wheels and source distribution created by release.yml. Except for the Windows wheels, running twine check on all of these yields PASSING when twine is installed in the nocoverage tox venv in my machine.

When I look at the METADATA file in the *.dist-info folder of macOS and Ubuntu wheels, vi does not show me any explicit carriage return characters in the long description. However, when I look at that file for the Windows wheels, vi shows me ^M. I wonder if this can cause failures when checking wheels that cross the Windows vs. Other OS boundary.

@mosesyhc Can you please try with Test-PyPI again. If it works, then I need to remove some test lines in release.yml before we approve the PR.

@jared321
Copy link
Contributor Author

Moses has suggested that the following triggering event should be added to release.yml before approving this PR so that a new set of distributions is built automatically by the action with the correct version string when a new tag is set on main.

on:
  release:
    types: [ published ]

@mosesyhc
Copy link
Member

I have downloaded the newly produced wheels from the release action. I can verify that twine check works for linux and Mac wheels. twine check on Windows wheel still fails with the same error as before, complaining about the error in image directives inside README.rst. Is there a configuration issue I should be aware of?

@jared321
Copy link
Contributor Author

And that fails when you run twine check on a Windows machine?

It appears that the file command line tool can help in Linux and macOS. The original README.rst does not indicate any issues

README.rst: ASCII text

I can also use vi to visualize different carriage returns (:set list). I only see typical unix carriage returns.

I can also run file on the METADATA files in the different wheels. For macOS and Linux

METADATA: Unicode text, UTF-8 text

While for Windows, I see

METADATA: Unicode text, UTF-8 text, with CRLF, CR line terminators

I also see the use of the two carriage returns in vi.

It seems like build is using both carriage returns when making the Windows wheels.

Did you try publishing them to Test-PyPI to see if PyPI knows how to deal with this automatically?

@mosesyhc
Copy link
Member

mosesyhc commented Aug 19, 2025

Yes. twine check was performed on a Windows machine, in git bash.

When I test upload to testPyPI, I get hit with another error complaining about non-release versions (which I should have expected):

INFO     Response from https://test.pypi.org/legacy/:
         400 Bad Request
INFO     <html>
          <head>
           <title>400 The use of local versions in <Version('0.3.1.dev115+g9c834532f')> is not allowed. See
         https://packaging.python.org/specifications/core-metadata for more information.</title>
          </head>
          <body>
           <h1>400 The use of local versions in <Version('0.3.1.dev115+g9c834532f')> is not allowed. See
         https://packaging.python.org/specifications/core-metadata for more information.</h1>
           The server could not comply with the request since it is either malformed or otherwise incorrect.<br/><br/>
         The use of local versions in &lt;Version(&#x27;0.3.1.dev115+g9c834532f&#x27;)&gt; is not allowed. See
         https://packaging.python.org/specifications/core-metadata for more information.


          </body>
         </html>

It seems like we have to build the release versions before trying twine uploads.

See if loading this file is the source of mixed carriage returns in Windows
wheels.
@mosesyhc
Copy link
Member

I believe your fix already handles it, but for a reference, here is a discussion that tackles the CR/CRLF problem (pypa/twine#495 (comment)).

@jared321
Copy link
Contributor Author

jared321 commented Aug 19, 2025

For the record:

  • Carriage Return - \r and 0x0D (old Mac OS)
  • Line Feed - \n and 0x0A (macOS and Unices)
  • CRLF - \r\n and 0D 0A (Windows)

Using hexdump -C, I see that the README.rst is using 0A. The bad METADATA files created by cibuildwheel for Windows wheels are using 0D 0D 0A. The Windows wheel successfully created by Moses on his machine using build uses 0D 0A, as expected.

One possible explanation for this is that, noting that our files apparently use \n,

  • one application of a porting tool sets replaces \n with \r\n everywhere and
  • a subsequent application of a porting tool replaces \n with \r\n everywhere again.

\n -> \r\r\n

@mosesyhc
Copy link
Member

Solved (but very puzzled.) It seems like the use of codecs.open recognizes the EOL characters but does not replace accordingly. See a brief observation here.

For the most recent built wheels, all twine check passed:


$ twine check dist/*
Checking dist/surmise-0.3.1-cp310-cp310-win_amd64.whl: PASSED
Checking dist/surmise-0.3.1-cp311-cp311-win_amd64.whl: PASSED
Checking dist/surmise-0.3.1-cp312-cp312-win_amd64.whl: PASSED
Checking dist/surmise-0.3.1-cp39-cp39-win_amd64.whl: PASSED

@mosesyhc
Copy link
Member

See TestPyPI results:
https://test.pypi.org/project/surmise/0.3.1/

@mosesyhc
Copy link
Member

Not too essential, but may consider.

Code coverage action fails because coveralls is unresponsive (second time in a week). Some has suggested generating and uploading badges with a back-up action.

@jared321
Copy link
Contributor Author

Strange. I don't understand completely. But I can confirm that the Windows wheels now have the expected line termination according to file

surmise-0.3.1.dist-info/METADATA: Unicode text, UTF-8 text, with CRLF line terminators

and with hexdump -C I see these as 0D 0A.

I'll do some clean-up now.

@jared321
Copy link
Contributor Author

  • Finished clean-up
  • Source distribution and all wheels passing twine check
  • Retested source distribution and macOS wheels as above
    • Also installed from source distribution & tested on all machines
  • Reset release.yml so it no longer runs on pull requests to main
  • Reviewed all changes again
  • All actions passing again

@mosesyhc I'm happy with what we have and I am ready for the next and hopefully last part of your review.

Copy link
Member

@mosesyhc mosesyhc left a comment

Choose a reason for hiding this comment

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

Confirmed that all the required wheels and source distribution pass twine now.
Once set with the next release tag, the package will be ready for release.

@jared321 jared321 merged commit 8634a61 into main Aug 20, 2025
46 checks passed
@jared321 jared321 mentioned this pull request Aug 29, 2025
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