Python Build - #292
Conversation
Luthaf
left a comment
Luthaf
left a comment
There was a problem hiding this comment.
Looks good overall, I'm not convinced we should switch to uv in CI, and we should for sure do it in a separate PR anyway
|
Thank you for the review! You're right, the switch to |
Luthaf
left a comment
Luthaf
left a comment
There was a problem hiding this comment.
The wheel (as created by python -m build python/metatomic_core) contains configuration files to find nlohmann::json, which I don't think we want to make available through the to external users here.
[...]
adding 'metatomic/share/cmake/nlohmann_json/nlohmann_jsonConfig.cmake'
adding 'metatomic/share/cmake/nlohmann_json/nlohmann_jsonConfigVersion.cmake'
adding 'metatomic/share/cmake/nlohmann_json/nlohmann_jsonTargets.cmake'
adding 'metatomic/share/pkgconfig/nlohmann_json.pc'
[...]
Ideally they should not even be installed by the main metatomic CMakeLists.txt (this is not specific to Python) when we fetch the code from github ourself.
Can you have a look?
|
Yes, you're right. 27675fd sets |
|
We still need to install the headers, just not the cmake config / |
765ae6c to
8dc3972
Compare
|
I added the headers in 8dc3972. Also, I tried making the |
| if (NOT nlohmann_json_FOUND) | ||
| # nlohmann_json was fetched and vendored by us: install its headers (used | ||
| # by metatomic's own public headers), but remove the CMake package config | ||
| # and pkg-config files it installs, since we don't want to advertise a | ||
| # system-wide nlohmann_json package to external users. | ||
| install(CODE " | ||
| file(REMOVE_RECURSE \"\${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_DATADIR}/cmake/nlohmann_json\") | ||
| file(REMOVE \"\${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_DATADIR}/pkgconfig/nlohmann_json.pc\") | ||
| ") | ||
| endif() |
There was a problem hiding this comment.
Sorry for the back and forth here, I did not realized we were using the cmake files from our own metatomic-config.cmake …
Maybe the best solution here would be to install these to a different directory, i.e. to CMAKE_INSTALL_PREFIX/lib/metatomic/external instead of CMAKE_INSTALL_PREFIX? This way we can find the headers with normal cmake configuration, and also hide them from most users by default.
@RMeli what do you think here?
a13f4cc to
173345c
Compare
c1498bd to
034a829
Compare
0c4521a to
1637cfb
Compare
034a829 to
7577f1b
Compare
2 participants
This PR adds the infrastructure needed to build the
metatomic_corePython wheel. Essentially, it's a port of the same functionality inmetatensor(setup.py,CMakeLists.txt, andpackage-core.sh). The only difference is that we importmetatensorincmake_ext'srunmethod to get thecmake_prefix_pathwhich is required to build the native library.The PR also makes an important change to the CI setup: now, containers for testing are built with Rust, similar to what
metatensordoes.📚 Download documentation for this pull-request
⚙️ Download Python wheels for this pull-request (you can install these with pip)