Fix and test sdist builds; reduce sdist size

[ajjackson]

Currently the release-building action tests the binary wheels (as part of cibuildwheel workflow) but not the source distribution.

The source-build release test is failing on (only?) windows so perhaps something is not quite right. Here is a workflow to look into that.

[ajjackson]

Ok, d3e40c5 reproduces the build error! Now we have a bit more freedom to try and fix it.

[github-actions]

Test Results

    29 files  +    29      29 suites  +29   1h 47m 51s ⏱️ + 1h 47m 51s
 1 066 tests + 1 066   1 060 ✅ + 1 060   6 💤 + 6  0 ❌ ±0 
13 780 runs  +13 780  13 698 ✅ +13 698  82 💤 +82  0 ❌ ±0 

Results for commit b353ef8. ± Comparison against base commit 3feac5c.

♻️ This comment has been updated with latest results.

[ajjackson]

@oerc0122 Looks like the problem is a bug in version.py; it will error out if any of the proposed git names is unavailable. (i.e. on Mac/Linux if git is unavailable, on Windows if any of git, git.cmd or git.exe is unavailable.)

Currently the sdist is not tested in the package-building workflow. Properly testing this sort of thing is tricky:

• we can't really use a Github action that doesn't have git installed, and hiding it from the PATH would be quite fiddly. We get some coverage by testing on Windows where the multiple versions come into play, but the "no git" case seems impractical. Perhaps using a container?
• the tests should not be in the sdist, so testing from a bare sdist install needs to also have a copy of the repo available and make sure it isn't exposed during testing. The best way to do that is probably with another tox setup 😭

The new action in this PR was mostly intended for troubleshooting and doesn't actually run tests. Maybe it should be migrated to a manual-dispatch workflow that targets a particular git commit and checks if the Ubuntu sdist -> Windows tests loop works, to be used when tweaking the build process.

Would it be very irresponsible to just roll out the version.py fix from this branch into a new release, without creating a bunch of new CI?

[oerc0122]

@oerc0122 Looks like the problem is a bug in version.py; it will error out if any of the proposed git names is unavailable. (i.e. on Mac/Linux if git is unavailable, on Windows if any of git, git.cmd or git.exe is unavailable.)

Currently the sdist is not tested in the package-building workflow. Properly testing this sort of thing is tricky:

• we can't really use a Github action that doesn't have git installed, and hiding it from the PATH would be quite fiddly. We get some coverage by testing on Windows where the multiple versions come into play, but the "no git" case seems impractical. Perhaps using a container?
• the tests should not be in the sdist, so testing from a bare sdist install needs to also have a copy of the repo available and make sure it isn't exposed during testing. The best way to do that is probably with another tox setup 😭

The new action in this PR was mostly intended for troubleshooting and doesn't actually run tests. Maybe it should be migrated to a manual-dispatch workflow that targets a particular git commit and checks if the Ubuntu sdist -> Windows tests loop works, to be used when tweaking the build process.

Would it be very irresponsible to just roll out the version.py fix from this branch into a new release, without creating a bunch of new CI?

Well, now I feel like an idiot. I think I originally had a FileNotFound except and presumed subprocess would never raise it and removed it.

I think that excluding the files from the sdist is easy enough and I think you're allowed to pass a directory to checkout, so we can probably do a bare checkout in the CI and call the run_tests.py to use the installed sdist version.

In terms of irresponsibility? It needs fixing. We just did a major build-system overhaul, we knew from the start things like this were liable to happen and would need attention. We did the best we could. I thought, though, that cibuildwheel would test the sdist for us, so I'm surprised we didn't catch this in that.

[ajjackson]

cibuildwheel isn't being used to build the sdist!

      - name: Build wheels
        uses: pypa/[email protected]
        env:
          CIBW_BUILD_FRONTEND: build
          CIBW_BUILD: ${{ matrix.version-tag }}-*
          CIBW_ARCHS: ${{ matrix.cibw_archs }}
          CIBW_SKIP: "*-musllinux*"

          CIBW_REPAIR_WHEEL_COMMAND_MACOS: ""

          CIBW_TEST_EXTRAS: "test,brille,phonopy_reader,matplotlib"
          CIBW_TEST_COMMAND: python {package}/tests_and_analysis/test/run_tests.py

        with:
          output-dir: wheelhouse

      - name: Create source distribution
        if: matrix.os == 'ubuntu-latest' && matrix.python-version == '3.11'
        shell: bash -l {0}
        run: |
          python -m build --sdist .

There's an interesting discussion here where people suggest building an sdist and then using that as the source for cibuildwheel:

pypa/cibuildwheel#173

[ajjackson]

The cibuildwheel docs suggest making the sdist in a separate step to the wheels, and having upload depend on both. That doesn't seem a bad idea, and would let some sdist tests run at the same time as other builds are being put together.

https://cibuildwheel.pypa.io/en/stable/deliver-to-pypi/#github-actions

[ajjackson]

Here is the CI run that nearly pushes to PyPI (so lets us see what files it would have used...) https://github.com/pace-neutrons/Euphonic/actions/runs/12253702919/job/34183074026?pr=337

I've now restored the usual triggers.

[ajjackson]

Not sure how this was created. I don't think I did...

[oerc0122]

Mine now says:

ajjackson temporarily deployed to pypi 2 hours ago — with GitHub Actions

[ajjackson]

I don't see a weird release or files on PyPI, anyway 🤷‍♂️

[oerc0122]

Do you want to update the sdist excludes here?

[ajjackson]

It looks like #338 will make some further changes to the workflow, as cibuildwheel doesn't do well at testing its builds from sdist (as opposed to git checkout).

We could merge that into this branch first, or merge this branch first and retarget #338 . Any preference?

[oerc0122]

It looks like #338 will make some further changes to the workflow, as cibuildwheel doesn't do well at testing its builds from sdist (as opposed to git checkout).

We could merge that into this branch first, or merge this branch first and retarget #338 . Any preference?

It'll automagically retarget so it doesn't make too much difference, but I would probably say merge #338 as this is really one job to master.

[ajjackson]

Ok, I think remaining tasks here are:

• strip out the docs as well
• update changelog again
• One last run of the workflow 🤞
• Final sanity check from @oerc0122

[ajjackson]

Pipeline looks good, if you're happy we can merge this and I'll have a crack at a release-number-updating manual action. (They need bumping anyway, may as well have a go at setting it up.)

[oerc0122]

All looks sane to me, let's hope it works.