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

Add pyproject.toml and unify PyPI CI builds #1407

Merged
merged 10 commits into from
Feb 27, 2024

Conversation

rockstorm101
Copy link
Collaborator

Two major changes on this PR:

  • Modernize build procedure to use a 'pyproject.toml' file. The idea being to keep all static project metadata within 'pyproject.toml' while all build configuration and dynamic metadata remains at 'setup.py'. The reason for this is to comply with PEP 517 and PEP 518.
  • Integrate all current CI actions building wheels and sdist for PyPI into a single file using cibuildwheel. The use of the current action 'RalfG/python-wheels-manylinux-build' is deprecated in favor of PyPA/cibuildwheel. Hopefully this merge will ease maintenance of the PyPI builds plus it allows us to easily produce wheels to support Apple Silicon and ARM architectures.

I labeled this PR as a Work In Progress as it will need testing particularly on Windows and macOS before it can be merged with confidence. I did not detect any issues on Linux so far, but I did not test wheels on architectures other than amd64. If it is too much of a change it can wait to be integrated on the next release cycle.

As always, this is open for discussion. Any and every feedback is appreciated.

The philosophy being to keep all static project metadata within
'pyproject.toml' while all build configuration and dynamic metadata
remains at 'setup.py'.

The 'long_description' is no longer needed because we link to the README
on 'pyproject.toml'.

A 'license' field is not required because it is already set on
'classifiers'.
This way no code is run when 'setup.py' is loaded until the setup
function is called.
The in-place build of the 'gcoder_line' extension is no longer
recommended. A command such as `python setup.py build_ext --inplace` is
deprecated. Therefore its outputs should no longer be ignored and should
be considered cruft that needs to be removed.
Integrate all current CI actions building wheels and sdist for PyPI into
a single file using `cibuildwheel`. The use of action
'RalfG/python-wheels-manylinux-build' is deprecated in favor of
`PyPA/cibuildwheel`.
.gitignore Outdated
@@ -10,9 +10,6 @@ prontserve-env
pronterface.spec
pronsole.spec
plater.spec
printrun/gcoder_line.c
printrun/gcoder_line*.so
printrun/gcoder_line*.pyd
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not delete as these files will be generated when compiling. This will prevent to upload those files to our repository

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough, I'll revert this change.

@DivingDuck
Copy link
Collaborator

I did only a quick view and will take a closer look on this as soon I finish the translation part.

@DivingDuck
Copy link
Collaborator

@rockstorm101,
I can't see anything against implementing for windows. The protocols are looking good and the binaries are running fine on my machine so fare I can see for now.

We should ask @neofelis2X as well for a quick look.

@neofelis2X
Copy link
Contributor

We should ask @neofelis2X as well for a quick look.

Sure, I tried the artifacts on intel and M2 Mac and they work flawlessly. Everything looks fine!

But there is indeed one tiny thing, that I can't unsee. On macos the system sees the app as 'pronterface' and not 'Pronterface' with capital 'P'. Renaming doesn't work, it seems to be the 'internal' name of the app.
pf_screen

PyInstaller Docs: "...the .. script .. supplies the name for the spec file and for the executable folder or file."

I believe this can be fixed by using the --name command with pyi-makespec (line 36) and changing pronterface.spec to Pronterface.spec / Pronterface.app
Sorry if this seems too nitpicky 😁 but it just looks off to me.

@DivingDuck
Copy link
Collaborator

LOL, I had never seen it written with an capital P. Looks like macOS users are the better linguists. :)

@rockstorm101
Copy link
Collaborator Author

I tried the artifacts on intel and M2 Mac and they work flawlessly. Everything looks fine!

Am I right in thinking that both macOS runners produce exactly the same app? I can't test them but the sizes are identical. Same goes for the generated wheels, which seem to generate the same files at a first glance:

$ tree cibw-wheels-macos-11-2
cibw-wheels-macos-11-2
├── Printrun-2.0.1-cp310-cp310-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp310-cp310-macosx_10_9_x86_64.whl
├── Printrun-2.0.1-cp310-cp310-macosx_11_0_arm64.whl
├── Printrun-2.0.1-cp311-cp311-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp311-cp311-macosx_10_9_x86_64.whl
├── Printrun-2.0.1-cp311-cp311-macosx_11_0_arm64.whl
├── Printrun-2.0.1-cp312-cp312-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp312-cp312-macosx_10_9_x86_64.whl
├── Printrun-2.0.1-cp312-cp312-macosx_11_0_arm64.whl
├── Printrun-2.0.1-cp38-cp38-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp38-cp38-macosx_10_9_x86_64.whl
├── Printrun-2.0.1-cp38-cp38-macosx_11_0_arm64.whl
├── Printrun-2.0.1-cp39-cp39-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp39-cp39-macosx_10_9_x86_64.whl
└── Printrun-2.0.1-cp39-cp39-macosx_11_0_arm64.whl

1 directory, 15 files

$ tree cibw-wheels-macos-12-3
cibw-wheels-macos-12-3
├── Printrun-2.0.1-cp310-cp310-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp310-cp310-macosx_10_9_x86_64.whl
├── Printrun-2.0.1-cp310-cp310-macosx_11_0_arm64.whl
├── Printrun-2.0.1-cp311-cp311-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp311-cp311-macosx_10_9_x86_64.whl
├── Printrun-2.0.1-cp311-cp311-macosx_11_0_arm64.whl
├── Printrun-2.0.1-cp312-cp312-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp312-cp312-macosx_10_9_x86_64.whl
├── Printrun-2.0.1-cp312-cp312-macosx_11_0_arm64.whl
├── Printrun-2.0.1-cp38-cp38-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp38-cp38-macosx_10_9_x86_64.whl
├── Printrun-2.0.1-cp38-cp38-macosx_11_0_arm64.whl
├── Printrun-2.0.1-cp39-cp39-macosx_10_9_universal2.whl
├── Printrun-2.0.1-cp39-cp39-macosx_10_9_x86_64.whl
└── Printrun-2.0.1-cp39-cp39-macosx_11_0_arm64.whl

1 directory, 15 files

Both macos runners produce the same wheels and are therefore redundant.
@neofelis2X
Copy link
Contributor

Yes they are effectively the same.
I don’t have a device with macos 11, so I can’t test if it makes a difference there specifically.

@rockstorm101
Copy link
Collaborator Author

Yes they are effectively the same.

Thanks. I've removed the macOS 11 runner then.

On macos the system sees the app as 'pronterface' and not 'Pronterface' with capital 'P'. [...] I believe this can be fixed by using the --name command with pyi-makespec (line 36) and changing pronterface.spec to Pronterface.spec / Pronterface.app

Could you please give a try to the latest artifact, see if it now shows up with a capital 'P' now?

@neofelis2X
Copy link
Contributor

Hi @rockstorm101,
already did! Perfect, the latest artifact 'printrun-test_macos-12_x64_py3.10' works. And the naming is now consistent. :D

@rockstorm101 rockstorm101 changed the title [WIP] Add pyproject.toml and unify PyPI CI builds Add pyproject.toml and unify PyPI CI builds Feb 10, 2024
@rockstorm101 rockstorm101 removed the WIP label Feb 10, 2024
@rockstorm101
Copy link
Collaborator Author

I consider this PR merge-ready now. Should anyone present know of any reason that this PR should not be merged, speak now (or next week) or forever hold your peace.

@rockstorm101 rockstorm101 merged commit 6b56bf8 into kliment:master Feb 27, 2024
12 checks passed
@rockstorm101 rockstorm101 deleted the pyproject-cibuild branch May 28, 2024 20:17
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.

3 participants