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

Skbuild #44

Merged
merged 83 commits into from
Aug 1, 2024
Merged

Skbuild #44

merged 83 commits into from
Aug 1, 2024

Conversation

robertodr
Copy link
Contributor

@robertodr robertodr commented Jun 2, 2023

This PR will "hide" CMake usage behind pip install thanks to scikit-build-core. This means that to work with CPPE from its source, you'll have to run pip install .. The first warning in the top CMakeLists.txt explains it:

This CMake file is meant to be executed using 'scikit-build'. Running
it directly will almost certainly not produce the desired result. If
you are a user trying to install this package, please use the command
below, which will install all necessary build dependencies, compile
the package in an isolated environment, and then install it.
=====================================================================
$ pip install .
=====================================================================
If you are a software developer, and this is your own package, then
it is usually much more efficient to install the build dependencies
in your environment once and use the following command that avoids
a costly creation of a new virtual environment at every compilation:
=====================================================================
$ pip install scikit-build-core[pyproject] cmake ninja pybind11 setuptools_scm
$ pip install --no-build-isolation -ve .
=====================================================================
You may optionally add -Ceditable.rebuild=true to auto-rebuild when
the package is imported. Otherwise, you need to re-run the above
after editing C++ files.

@robertodr
Copy link
Contributor Author

Seems to be working quite well, except that Windows testing is again broken with Windows fatal exception: access violation 🤦🏻

@maxscheurer
Copy link
Owner

Cool!

One issue: I've only made bad experiences with automatic versioning so far... what if you distribute a .tar.gz file which is not a git repo - how's the version "generated" in that case?

@robertodr
Copy link
Contributor Author

robertodr commented Jun 4, 2023

Current status:

  • I can compile and test on Linux and macOS
  • I can build the source distribution (though the automated versioning gives 0.1 as version 😢)
  • I can build pre-compiled wheels for Linux x86_64, macOS x86_64 and macOS arm64 (though the automated versioning gives 0.1 as version 😢)
  • On Linux, I can build both for CPython and PyPy, with manylinux.
  • On Windows I can build, but the testing fails with Windows fatal exception: access violation I tried deactivating OpenMP, using MSVC directly or both, to no avail.
  • The wheel build for Windows fails because it can't find Eigen. I'm installing it, but I'm having a hard time figuring out how to get CMake to find it 🤷🏻

@maxscheurer
Copy link
Owner

Thanks 😄 About the versioning... that's exactly what I was talking about above and the reason I always stuck with bump2version. It's not fancy but at least you get the correct version 😢

@robertodr
Copy link
Contributor Author

I think I figured out the issue with the versioning. Now I get:
2023-06-04_11-02
which I think it's correct.

@robertodr
Copy link
Contributor Author

@maxscheurer I'm essentially stuck here. While I would like to, I do not have the time to debug the windows build issues in a VM. Similarly, I've only made pip install work without using skbuild on toy projects, so I'm also not in a position where I can harmonize the new CMake infrastructure with what was available in setup.py.

@maxscheurer
Copy link
Owner

Sounds quite tricky and annoying 🙈 I wonder at which point one should just give up... I always thought cppe was lightweight enough such that the step from CMake to setup.py/skbuild should be doable, but as you said, debugging Win locally is such a time sink.

@robertodr
Copy link
Contributor Author

The issue is indeed windows. I managed to get scikit-build-core and cibuildwheel off the ground with a very reasonable amount of iterations...

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@robertodr
Copy link
Contributor Author

robertodr commented Sep 13, 2023

Outstanding problems:

@robertodr
Copy link
Contributor Author

For the cibuildwheel problem, I followed the suggestion of downloading Eigen with FetchContent instead of pre-installing it. This has the advantage that now it is bundled with the sdist and thus there is no need to pre-install Eigen when building from source from it.

@robertodr
Copy link
Contributor Author

It's possible to also produce the aarch64 and ppc64le wheels with cibuildwheel as well 🤓 Thought that slows down CI a lot. I need to split out each single wheel build into a different worker:

@robertodr
Copy link
Contributor Author

Now each wheel is built by a separate worker in the workflow. I've removed the ppc64le architecture wheels as some testing dependencies (scipy) would need to be built from source.

@maxscheurer maxscheurer mentioned this pull request Jan 27, 2024
@robertodr
Copy link
Contributor Author

@maxscheurer I think you need to add some settings in the project's PyPI page to get the upload to work: https://docs.pypi.org/trusted-publishers/adding-a-publisher/

@maxscheurer
Copy link
Owner

I'll take a look, thanks! 😊

@robertodr
Copy link
Contributor Author

The source distribution has become 100x larger :/ I guess it's because it's including all of the Eigen source files. I'll take a look...

fetching eigen from tarball (not the git repo) brings the size of the sdist down to a more modest 5 MiB, instead of 100 MiB. 😸

@maxscheurer
Copy link
Owner

maxscheurer commented Jul 30, 2024

I fixed the project settings for test.pypi and normal pypi 😁

Should the pypa action be named uses: pypa/gh-action-pypi-publish@release/v1? NVM, I noticed I can commit to your branch myself.

@maxscheurer
Copy link
Owner

maxscheurer commented Jul 31, 2024

Looks like we cannot deploy from the fork: https://github.com/maxscheurer/cppe/actions/runs/10159733886/job/28095059187?pr=44

Any idea what to do? Should we merge this and open a separate PR to try the deployment?

@robertodr
Copy link
Contributor Author

Annoying... What if you merge (if you're happy with the rest!) such that it gets tested? Once we know it works I can patch the workflow to only run on releases

@maxscheurer
Copy link
Owner

Once we know it works I can patch the workflow to only run on releases

Yes, only on releases and deployment to the real pypi, not the test one I guess.

I'm happy with the rest. One question regarding versioning: that's automatically taken care of by git tags, right? I'm used to bump2version manual bumping the versions...

@robertodr
Copy link
Contributor Author

Yes, setuptools-scm takes the tag and computes the version like versioneer does (as in psi4) So to make a release it's enough to tag it on GitHub.

@maxscheurer
Copy link
Owner

Yes, setuptools-scm takes the tag and computes the version like versioneer does (as in psi4) So to make a release it's enough to tag it on GitHub.

Thanks for clarifying! I've never understood how that works in source distributions, i.e., once you don't have a git repo anymore 😁 But yeah I'll try to figure that out myself haha.

@maxscheurer maxscheurer merged commit 1d2d0be into maxscheurer:master Aug 1, 2024
37 of 38 checks passed
@robertodr robertodr deleted the skbuild branch August 1, 2024 06:31
@robertodr
Copy link
Contributor Author

That's taken care of by .git_archival.txt and .gitattributes

@maxscheurer
Copy link
Owner

I think I need to push a tag to try the PyPI upload: https://github.com/maxscheurer/cppe/actions/runs/10193155306/job/28197630303

@robertodr
Copy link
Contributor Author

Screenshot_20240801-083520
I think it kind of worked. It's failing because the computed version has the commit description and thus is not in "canonical" format

@robertodr
Copy link
Contributor Author

If you can make a 0.3.3a0 tag, we'll know more

@maxscheurer
Copy link
Owner

Should the tag be 0.3.3a0 or v0.3.3a0?

@robertodr
Copy link
Contributor Author

v0.3.3a0 should be fine :)

@maxscheurer
Copy link
Owner

Fingers crossed then...

@robertodr
Copy link
Contributor Author

It didn't trigger :( maybe the syntax of the workflow for the triggers needs to be extended

@robertodr
Copy link
Contributor Author

There's a difference between published and created https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=published#release If you make a release in the GitHub ui (using the existing tag) the workflow should trigger

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