libmambapy: Switch build backend to scikit-build-core
#3802
Conversation
LecrisUT
force-pushed
the
scikit-build-core
branch
from
e7d8cad to
3c886e9
Compare
dev/environment-dev.yml
Outdated
| - scikit-build | ||
| - scikit-build-core | ||
| - setuptools-scm | ||
| - pybind11-stubgen <1.0 |
There was a problem hiding this comment.
Does conda still not have any integration with python build systems (PEP517)? Manually referencing them like this is very error-prone, particularly since some build dependencies are dynamically generated, setuptools-scm in this case.
LecrisUT
force-pushed
the
scikit-build-core
branch
from
3c886e9 to
c34102b
Compare
|
This is important for Fedora now that scikit-build has been dropped. Could this get rebased and moved forward? |
|
I'm trying to adapt the Fedora libmamba package to build with this but not having any luck: work is here: https://src.fedoraproject.org/rpms/libmamba/pull-request/4 |
I will try to investigate the PR over the week. But a quick thing you can try is to pass |
|
I'm am passing that, and I think I updated the environment variable used to pass in cmake options properly. So not sure what's up. |
LecrisUT
force-pushed
the
scikit-build-core
branch
3 times, most recently
from
e2f8bc1 to
d2e4110
Compare
There was a problem hiding this comment.
This must be done at a point, but we unfortunately are already swamped with work, and we have other priorities to honour.
Can you test this PR by opening another one on https://github.com/conda-forge/mamba-feedstock/ with the patch?
.git_archival.txt
Outdated
There was a problem hiding this comment.
Can you motivate the changes made to this file using an inline-comment?
Also, can you indicate whether this is safe regarding the discussions given in pypa/setuptools-scm#806.
There was a problem hiding this comment.
I don't know if setuptools-scm accepts comments or any other tools that would also read this file, so if possible it would be better to address it in this PR.
Edit: Actually I will write a few words in the .gitattributes part.
Also, can you indicate whether this is safe regarding the discussions given in pypa/setuptools-scm#806.
Yes, I made the PR in the upstream to indicate that ref-names must be removed because that is the dangerous part. describe-name could in theory also be problematic if there are multiple tags added to the same commit that match the regex, but I don't think it's the case here
| templates = { | ||
| "libmamba": "libmamba/include/mamba/version.hpp.tmpl", | ||
| "micromamba": "micromamba/src/version.hpp.tmpl", | ||
| "libmambapy": "libmambapy/src/libmambapy/version.py.tmpl", |
There was a problem hiding this comment.
This file is required.
Can you revert this change?
There was a problem hiding this comment.
This file is instead generated by setuptools_scm. It is generated every time the python build system is called and it tracks better the git distance and such. For the CMake only build it indeed would not have that file, but there are other issues with building the CMake-only build of the python bindings, primarily the lack of the dist-info metadata which would allow to pip uninstall.
Are there other pitfalls that the lack of this file could have?
LecrisUT
force-pushed
the
scikit-build-core
branch
from
d2e4110 to
b42ff41
Compare
Of course, just want to make sure it doesn't fall through the cracks. I am also rapidly distracted so I also forget to follow up on PRs.
I can never remember how to do that, hope this one works: conda-forge/mamba-feedstock#341 |
|
Looks like this needs to get rebased with the latest version number bump. Any chance this could get merged? Any other concerns that need to be addressed? |
|
The feedstock with the patch is failing, I need to check why. I'll have a look next week, hopefully we can fix the failures and get this merged. |
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Also move the Config.cmake binary files to the top of the build-dir Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
LecrisUT
force-pushed
the
scikit-build-core
branch
from
b42ff41 to
43deeb1
Compare
setuptools_scm does not seem to read the file if it is from git root, it has to be next to pyproject.toml
Sorry, I've said that I would try to build it locally, but never got around to it. I've did a quick build now and found the issue ( |
There was a problem hiding this comment.
Great work setting that up!
My only point is about version.py. If possible I would prefer NOT switch to setuptools-scm as it does not play well with the rest of the release process.
I don't think there is a point of having something smart if we still have the other templated files. A single definition is better until we can improve globally.
EDIT: Just saw
Used setuptools-scm because it is simpler to configure for now. I don't think the reading version from python file is supported in scikit-build-core yet
And I found that we can get it from regex, I would much rather use that than mixing our release with VCS and git archival things.
Yes, I thought about it a bit, and it is possible to do it, and there would be different approaches depending on how it is stored. Either regex or dynamic provider would be used. But I need assistance to point me to the source. Note: that generation of $ pip install "libmambapy@git+https://github.com/mamba-org/mamba"(and derivatives) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3802 +/- ##
=======================================
Coverage 63.99% 63.99%
=======================================
Files 303 303
Lines 38938 38938
Branches 2863 2871 +8
=======================================
Hits 24918 24918
Misses 13953 13953
Partials 67 67 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@LecrisUT should I push a commit here with my proposal? |
Feel free to do so |
AntoinePrv
force-pushed
the
scikit-build-core
branch
from
1b02e83 to
97d2ae5
Compare
|
That's my take on it. I'm off for the weekend, feel free to keep making changes, revert etc if you need to. |
|
Looks fine with me 👍. Didn't look into your release procedure, but w.r.t. pip installability from other sources it is ok. The |
|
Note about pip installability that the current python build assumes to find Not sure about the prerelease either. To me there should not be a |
If you're not sure about the format to use there, here is a quick reference. If it's like |
|
Had to double check if >>> import packaging.version
>>> packaging.version.Version("1.3.5.alpha4")
<Version('1.3.5a4')>However, keep in mind that the real package version, artifacts, etc. will use the normalize format, i.e. |
AntoinePrv
force-pushed
the
scikit-build-core
branch
from
8e58acc to
3490069
Compare
There was a problem hiding this comment.
Thank you, @LecrisUT and @AntoinePrv for adaptation.
I think we could have a pre-release version after this PR is merged to test that the new build system works properly. If it does not, we can bug-fix before the official release.
What do you think?
|
Feedstock CI is ✅ |
|
Thank you for your work and your patience @LecrisUT ! |
6 participants
setuptools-scmbecause it is simpler to configure for now. I don't think the reading version from python file is supported inscikit-build-coreyetsrc/libmambapy/bindingstobindingsbecause it interferes withpip install -e. It is cleaner to have the compilation source outside of the pure python files.Other than that it should be effectively identical.
I did not review the build system, but at least there is an immediate issue that needs to be resolved
This breaks all sdist builds because the archive starts from
libmambapyfolder. In practice, this is a purely developer option and there are neater ways to create one using targets instead. But if you really wish to keep it in that form, a solution is to make a symlink of the cmake folder.Closes #3798