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

Update build mechanism #52

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PeterJCLaw
Copy link
Member

@PeterJCLaw PeterJCLaw commented Jan 14, 2023

This moves to using more current Python packaging mechanisms which are now needed for recent versions of setuptools (which come by default with recent Python).

See individual commits for details.

I've tested manually that this preserves the support for man sr-* pages after install.

TODO:

  • an interesting validation here might be to diff the sdist and wheel produced on main with the one produced here (for a setup where both worked, of course)

@PeterJCLaw PeterJCLaw force-pushed the update-build-mechanism branch from 5c7f626 to 54e6eba Compare January 14, 2023 16:08
This enables building wheels using the latest setuptools and
related tooling, while still distributing the man pages.
This both clarifies that it's static and better prepares this
package for the future of python packaging. Notably this avoids
importing the package as part of the build to get the description
and version, which is already not supported in some cases.
@PeterJCLaw PeterJCLaw force-pushed the update-build-mechanism branch from 54e6eba to f40877e Compare January 14, 2023 16:13
@PeterJCLaw PeterJCLaw requested a review from trickeydan January 14, 2023 16:14
@PeterJCLaw PeterJCLaw marked this pull request as ready for review January 14, 2023 16:14
@trickeydan
Copy link
Contributor

trickeydan commented Jan 14, 2023

$ python3 setup.py bdist_wheel
/opt/homebrew/lib/python3.10/site-packages/setuptools/config/setupcfg.py:508: SetuptoolsDeprecationWarning: The namespace_packages parameter is deprecated, consider using implicit namespaces instead (PEP 420).
  warnings.warn(msg, warning_class)
running bdist_wheel
...
copying sr/tools/inventory/inventory.py -> build/bdist.macosx-12-arm64/wheel/sr/tools/inventory
running install_data
running build_sphinx
error: invalid command 'build_sphinx'

I think it's probably worth moving to PEP420 in this PR as well. (See error above)

What am I missing to build sphinx here? Surely it should pull it in?

Have you considered using other build tools? e.g (poetry or build). poetry is used for quite a lot of kit team projects these days. Build is used for april_vision, although we haven't fully evaluated how well it works in comparison to poetry yet.

@PeterJCLaw
Copy link
Member Author

Huh, that was definitely working for me locally and seems to be working in CI. I had expected that it would be pulled in by what's in the pyproject.toml. If I put a fresh clone in a fresh venv I do reproduce the error you're seeing however :(

In terms of alternatives, I had originally hoped this would be a simple minimal fix just to get the build working so CI would pass, rather than the larger change it turned into.

I don't have strong feelings about moving to another build tool if there is one which will work for us here. In order to not break things we need one which supports dots in package names (poetry unfortunately no longer does, as we found out when trying to publish sr.robot just before Kickstart). It would also be great to maintain the behaviour of man sr-$tool working after an install -- that's the source of the faff around Sphinx here (i.e: why we don't just use python -m sphinx), though as long as there's a way to hook the sphinx build as part of the "binary" builds and then arrange for those to end up as "data" files in the wheel that shouldn't be too much of an issue.

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