Skip to content

Move to pyproject.toml build#218

Merged
atztogo merged 11 commits intospglib:rcfrom
LecrisUT:fix/217
Jan 20, 2023
Merged

Move to pyproject.toml build#218
atztogo merged 11 commits intospglib:rcfrom
LecrisUT:fix/217

Conversation

@LecrisUT
Copy link
Copy Markdown
Collaborator

@LecrisUT LecrisUT commented Jan 15, 2023 •
edited
Loading

This PR has a few changes:

  • Rename and switch the ctest CI to be purely cmake. Didn't make much sense to be python dependent there
  • Switch the build to use pyproject.toml environment. Some equivalent commands to the setup.py ones are:
    • setup.py build -> python -m build (need to install pip install build). This is mostly unnecessary unless you want to fail quickly
    • setup.py install -> pip install .
  • Temporarily removed the nanoversion script until we think of how to implement it with dynamic variable Extract metadata from cmake scikit-build/scikit-build-core#172

We need to do a few tests on this:

  • Test the build CI. Setting the target branch to develop initially to trigger these (CI job finished)
  • Mac build passes with scikit-build-core
  • Test pypi-weel-builds. We'll change the target branch to rc if develop works to test it there (and if necessary back to develop to keep the branches synced)

@atztogo please let me know how the mac build goes. You can manually set it -DCMAKE_OSX_DEPLOYMENT_TARGET=... in pyprojec.toml if it doesn't work automatically and I'll write a PR upstream for that fix.

About making editable installs, this should be doable via pip install --editable .. But for this we need scikit-build/scikit-build-core#171

Closes #217
Currently, cibuildwheel still doesn't work locally. Added upstream issue: scikit-build/scikit-build-core#173. Here's the setup to debug locally assuming you have a docker/podman environment:

$ CIBW_CONTAINER_ENGINE=podman cibuildwheel . --platform linux

@LecrisUT LecrisUT marked this pull request as draft January 15, 2023 16:35
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 15, 2023 •
edited
Loading

Codecov Report

Base: 64.00% // Head: 93.61% // Increases project coverage by +29.60% 🎉

Coverage data is based on head (07f0867) compared to base (6b4332c).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@             Coverage Diff              @@
##           develop     #218       +/-   ##
============================================
+ Coverage    64.00%   93.61%   +29.60%     
============================================
  Files           18       15        -3     
  Lines         1328      908      -420     
============================================
  Hits           850      850               
+ Misses         478       58      -420
Impacted Files Coverage Δ
test/vasp.py
test/test_magnetic_dataset.py
test/test_niggli_delaunay.py
test/test_reciprocal_mesh.py
test/test_collinear_spin.py
test/conftest.py
test/test_benchmark.py
spglib/__init__.py
test/test_layergroup.py
test/test_spglib.py
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@LecrisUT LecrisUT changed the base branch from develop to rc January 15, 2023 18:51
@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Jan 15, 2023 •
edited
Loading

Worth pointing out. I have tested moving back to the old approach with setup.py, but we get again cmake not detecting python in cibuildwheel. There is something more fundamentally different here...

Edit: Oh wow, "the fix" is so stupid...

@LecrisUT LecrisUT force-pushed the fix/217 branch 2 times, most recently from 8cf0302 to 18a122c Compare January 16, 2023 10:57
@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Jan 16, 2023

Sorry about the amount of CI actions. Cmake linking on windows is soo weird

@LecrisUT LecrisUT force-pushed the fix/217 branch 4 times, most recently from c4064b7 to ddef3e4 Compare January 16, 2023 14:18
@LecrisUT
Fix Windows build python api
aad8cca 
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Jan 16, 2023 •
edited
Loading

@atztogo Finally got it working. Weirdly, the ubuntu tests take half an hour to perform. Any idea on that?

Edit: That was because it was running emulated aarch64 on x86_64

@LecrisUT LecrisUT marked this pull request as ready for review January 16, 2023 17:29
@LecrisUT
Bump version 2.1.0
d95793f 
Signed-off-by: Cristian Le <[email protected]>
@atztogo
Copy link
Copy Markdown
Collaborator

atztogo commented Jan 19, 2023

@LecrisUT, thanks for your great effort!

@atztogo please let me know how the mac build goes. You can manually set it -DCMAKE_OSX_DEPLOYMENT_TARGET=... in pyprojec.toml if it doesn't work automatically and I'll write a PR upstream for that fix.

I don't see what you mean. What can I do?

@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Jan 19, 2023

Like what we did in #212. The CIs pass so I suspect it works, but confirm that please

lan496
lan496 reviewed Jan 19, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@lan496 lan496 left a comment

Choose a reason for hiding this comment

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

@LecrisUT Great works, thanks! I have learned a lot about CMake and Python packaging from your PR. I wrote tiny comments.

@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Jan 19, 2023

Note that last commit is a [skip ci] so use merge/squash instead of rebase to trigger the github action again with it uploading to Pypi as well.

@atztogo
Copy link
Copy Markdown
Collaborator

atztogo commented Jan 19, 2023

Like what we did in #212. The CIs pass so I suspect it works, but confirm that please

Yes, on macOS arm64, I confirmed that it works. A warning was printed out in the build process by

pip install . -vvv

which is

[ 98%] Building C object python/CMakeFiles/python_api.dir/_spglib.c.o
  /Users/togo/code/spglib/python/_spglib.c:179:1: warning: unknown attribute 'dllexport' ignored [-Wunknown-attributes]
  DLL_PUBLIC PyObject* PyInit__spglib(void)
  ^~~~~~~~~~
  /Users/togo/code/spglib/python/_spglib.c:177:36: note: expanded from macro 'DLL_PUBLIC'
  #define DLL_PUBLIC __attribute__ ((dllexport))
                                     ^~~~~~~~~
  1 warning generated.

@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Jan 19, 2023

Great, this should be ready now. I've cleaned the warning a bit for more general systems.

Also I've removed the Python2 compatibility macros since it has been sunset-ed for 3 years and we do not test them. Build system would not work for it also.

@atztogo atztogo merged commit 9c5b02c into spglib:rc Jan 20, 2023
@atztogo
Copy link
Copy Markdown
Collaborator

atztogo commented Jan 20, 2023

Merged to rc. Thanks a lot @LecrisUT!

@lan496 lan496 mentioned this pull request Jan 20, 2023
Merged
@LecrisUT LecrisUT deleted the fix/217 branch January 20, 2023 09:36
@atztogo atztogo mentioned this pull request Jan 21, 2023
Closed
@LecrisUT LecrisUT added this to the 2.1 milestone Feb 21, 2023
@LecrisUT LecrisUT added the enhancement Significant ehancements label Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lan496 lan496 lan496 approved these changes

Assignees

No one assigned

Labels

enhancement Significant ehancements

Projects

None yet

Milestone

2.1

Development

Successfully merging this pull request may close these issues.

PyPI build failure on github workflow

4 participants

@LecrisUT @codecov-commenter @atztogo @lan496