-
Notifications
You must be signed in to change notification settings - Fork 34
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
3.28.1: why is build own copy of cmake? 🤔 #451
Comments
Wheels can not have any logic in them. They simply are unzipped to the correct directories. And what are the chances the system will have exactly the same version of cmake that you are installing? Or would you expect I think what you want is a smart build backend that only adds |
I'm expecting that this module would be just wrapper checking cmake executable version according to its version and that's it ..
I don't see at all building own copy of cmake it it provides only interface to use cmake. |
The The entire point of the cmake module is to distribute cmake. It does not wrap the command (other than providing a very, very simple and not quite perfect cmake runner wrapper to make it easier to get the paths right).
The chances of it exactly matching are really small. If you have 3.28.1 and the cmake module is What's worse, the builder can't tell if you are installing or making a wheel (modern packaging - there used to be a Python packaging is not compatible with this sort of design. A package should not build different wheels based on its environment. A user expects to get the version of CMake they asked for. And you can't tell what a user actually asked for! There's a very simple and easy solution for building package with CMake: Only ask for the Python package if you need a copy of cmake! As I've said, scikit-build-core does this, and there's an example in scikit-build's docs on how to do it yourself, it's less LoC than most setuptools customizations used to be. meson-python does this with This was discussed for ninja-python-distributions multiple times with the same conclusion: fix the builders to only ask for what they need instead of making lying packages that don't include what they are supposed to include based on the system they were built on. By the way, 95% of the time you should get a binary; nothing will build. We provide wheels for every system we can. It only builds if you make it build (as shown above) or if you are on a system like Pyodide / FreeBSD / Android / iOS, where we can't provide wheels. Again, the fix is easy - only request it in |
Waste of time. |
I catalog changes in cmake on this page: https://cliutils.gitlab.io/modern-cmake/chapters/intro/newcmake.html And the versions available on various systems on this page: https://cliutils.gitlab.io/modern-cmake/chapters/intro/installing.html And there are massive, huge changes in each version. 3.28 added support for C++20 modules. So if I want to use those, I have to set cmake>=3.28. The first version to properly support FindPython was 3.18.2 (with important changes as recently as 3.26, like support for Stable ABI), which you'll find is not available on even the newest LTS Ubuntu. And the next version coming out (RC1 came out a couple of days ago) will finally have the ability to make the test target depend on ALL! If you want to make a "cmake-if-needed" SDist only package, go ahead. There's nothing stopping you. But we can't break everyone (including most scikit-build-core users!) who need a modern CMake and request it (scikit-build-core requests it only if actually needed, responsibly!) by not shipping cmake in a cmake package. No one is asking This is not the right solution to the problem. The correct solution is to fix those packages to only request cmake if the system doesn't provide one. The mechanism for doing exactly that was standardized in 2016. As I said on the ninja discussions, I'd also be okay with an opt-in way to make a dummy package, but we are about to move the build backend, so it would be best to wait on that. I also want to change the way the binary is provided, which will happen after the move but before any dummy option. |
Also, 3.27 removed (sort-of) the old FindPythonLibs/FindPythonInterp. So the CMake version really matters to Python users. |
Please show me ONE python module which depends on some cmake features added after first cmake 3.0.
Amongst those modules there ONLY SIX
which are using cmake and none are using your module. cmake 3.18.2 has been released almost FOUR years ago. It was the time when still python 2.x was in use, |
A little more than one example: All our C++ projects with Python bindings (e.g., https://github.com/cda-tum/mqt-core) require CMake>=3.19 and might, at some point, jump to 3.24 for better |
Why do you want to use |
We do not use FetchContent to get CMake, but other project dependencies. |
Name of those modules. |
For example,
By making use of the modern |
https://pypi.org/project/gtest/ is no longer maintained. Homepage shows 404. https://github.com/nlohmann/json/ it is not python module.
I've not been asking for what is |
Yeah. And I never claimed that. These are dependencies of our C++ code that is then exposed as a Python module via pybind11 and distributed to PyPI.
... 🙃
Yes.
By having the required dependencies installed on the system before the build. |
So WHY you don't require have installed cmake before build your module? 🤔 |
That is not a question I am here for to answer. You were asking for examples of Python modules that depend on modern CMake features and that is what I tried to show you. |
Because you provided bogus exampled it allowed me BACK to main question which is: why your module cannot just use system installed cmake? 🤔 |
It does use the system installed cmake if the corresponding version is new enough. That is precisely what scikit-build-core does for you and what Henry alluded to in the very first reply to your initial issue. |
OK I give up .. I've not been asking about what scikit-build-core ids doing form me. |
Thanks everyone for commenting by sharing perspectives, details, links and resources, I learnt a lot reviewing these 🙏
Once we are done integrating #312, it seems like the particular issue reported by @kloczek would be addressed.
@kloczek To help refocus, is there a particular package you are tying to distribute as Python wheels ? |
Yes .. https://github.com/AcademySoftwareFoundation/OpenTimelineIO/ build currently depends on |
Great. Do you have already a branch to update the project to use |
That project requires CMake 3.18.2+: https://github.com/AcademySoftwareFoundation/OpenTimelineIO/blob/d22935e0033510bd834abbeb42e95b95ea94fab6/CMakeLists.txt#L3 It also uses the So all you need to do is build without isolation (which I would recommend when packaging for a distribution anyway, isolated == needs internet) and avoid validating build dependencies (which is an extra flag with pip). This is what many conda packages do. |
Seems you did not red what I've already wrote that some build systems are intentionally cut off from access to the public network. |
To move forward with this, I suggest you modernize the OpenTimelineIO project to remove the use of Let us know if you need help moving forward with this. |
@jcfr, I believe @kloczek is trying to make an RPM for OpenTimelineIO, and is not otherwise associated with the project. @JeanChristopheMorinPerso is. An "isolated build" in PEP 517 terms is one that is isolated from the current Python environment by making a temporary Python environment and downloading dependencies from the network! I know it's the reverse of what you might think of as "isolated", but isolated PEP 517 builds == access to network. So you want to turn off isolation so that you can use your current, prebuilt build environment (it's still a PEP 517 build). All third party distributions should be disabling build isolation, that's only for users that have access to the internet and are getting the package directly from PyPI and do not have a build recipe. Conda-forge is a distro - it has a strong Python focus, but it's just a redistributor; I was using that as an example of where this problem is already solved by using the technique I'm outlining here. Add cmake (the RPM) to the dependencies, add the If there's a problem doing this, let me know, I can help. |
Correct.
As I'm building my own packages I'm not interested to test what I'm packaging against random modules downloaded as .whl archives bu ONLY against what I have already packaged (as I wrote I have now almost ~1.25k such packaged modules) to test that what I've already packaged is OK. This is why I';m using obligatory rule to execute rpm %check in which usually is pytest execution (currently 99% those packages are tested that way .. pytest can handle [tkloczko@pers-jacek SPECS]$ ls -1 python-*spec | wc -l; grep ^%pytest python-*spec|wc -l
1246
1234 So currently only 12 of those packages are not using pytest.
Which additionally on packaging stuff as rpm packages adds only tons of additional build dependencies.
Nope .. that is not true because I hope that above provides full context of what I'm doing (why, using what kind assumptions, and what kind of methodology). |
What Henry suggests here is to add Also, I'll add that you don't need to use the python building tools to compile OTIO (including the python bindings). OTIO was developed in an industry where C++ is heavily used and where Python gets embedded into C++ apps. You can simply call
@kloczek Having If you have any OTIO related questions, please feel free to open an issue with the project at https://github.com/AcademySoftwareFoundation/OpenTimelineIO/issues. |
Build requires the |
Here are my spec files for Summary: CMake is an open-source, cross-platform family of tools designed to build, test and package software
Name: python-cmake
Version: 3.28.3
Release: 2%{?dist}
License: Apache-2.0 (https://spdx.org/licenses/Apache-2.0.html)
URL: https://pypi.org/project/cmake/
VCS: https://github.com/scikit-build/cmake-python-distributions/
Source: %{VCS}/archive/%{version}/%{name}-%{version}.tar.gz
Patch: %{name}-man3_level.patch
BuildRequires: cmake
BuildRequires: gcc-c++
BuildRequires: openssl-devel
BuildRequires: python3dist(build)
BuildRequires: python3dist(installer)
BuildRequires: python3dist(setuptools-scm)
BuildRequires: python3dist(scikit-build)
BuildRequires: python3dist(sphinx)
BuildRequires: python3dist(wheel)
# ChcekRequires:
BuildRequires: python3dist(pytest)
Requires: cmake
%description
CMake is used to control the software compilation process using simple platform
and compiler independent configuration files, and generate native makefiles and
workspaces that can be used in the compiler environment of your choice.
%prep
%autosetup -p1 -n cmake-python-distributions-%{version}
%build
%pyproject_wheel
%sphinx_build_man
%install
%pyproject_install
%__install -Dm644 build/sphinx/man/*.3 -t %{buildroot}%{_mandir}/man3
%check
%pytest
%files
%{_mandir}/man3/*
%{python3_sitearch}/cmake
%{python3_sitearch}/cmake-*.*-info Summary: Editorial interchange format and API
Name: python-OpenTimelineIO
Version: 0.15
Release: 2%{?dist}
License: Apache-2.0 (https://spdx.org/licenses/Apache-2.0.html)
URL: https://pypi.org/project/OpenTimelineIO/
VCS: https://github.com/AcademySoftwareFoundation/OpenTimelineIO/
Source: %{VCS}/archive/v%{version}/%{name}-%{version}.tar.gz
Patch: %{name}-man3_level.patch
BuildRequires: gcc-c++
BuildRequires: python3dist(build)
BuildRequires: python3dist(cmake)
BuildRequires: python3dist(installer)
BuildRequires: python3dist(setuptools)
BuildRequires: python3dist(sphinx)
BuildRequires: python3dist(wheel)
# CheckRequires:
#BuildRequires: python3dist(pyaaf2) >= 1.4
BuildRequires: python3dist(pytest)
%description
Editorial interchange format and API
%prep
%autosetup -p1 -n OpenTimelineIO-%{version}
%build
%pyproject_wheel
%sphinx_build_man
%install
%pyproject_install
%__install -Dm644 build/sphinx/man/*.3 -t %{buildroot}%{_mandir}/man3
%check
%pytest
%files
%{_mandir}/man3/*
%{python3_sitelib}/* As you see al your suggestions are already on place.
So you want to me to shoot in my own foot to obey PEP517 build time dependencies checks to be surprised in the future by on next upgrade to be surprised that rpm build procedure did not bark that those dependencies has been changed? 🤔 |
If you are missing a build dependency, you will not be able to build. You are really just trading a slightly more readable error message for an import error or similar message. |
So you want to say that you don't know that |
OpenTimelineIO uses This project is only a PyPI redistribution of cmake binaries. |
@kloczek It would be appreciated if you could be gentle and respectful with people in this thread that are trying to help you. The last comment you made regarding my only reply in this thread is disrespectful. |
Someone is trying to convince |
I'm not sure what point you are trying to make here. At this point, I feel attacked and I don't feel comfortable continuing this conversion with you. I don't think this conversation is very productive and I will unsubscribe from this issue. |
Maybe I'm doing that wrong way .. however how would approach to someone who is not even trying to answer on VERY SIMPLE question: why |
This the point of having the In the context of the RPM packaging, as already hinted by @henryiii, we simply suggest to install the regular cmake package, there is no need for Assuming the version of CMake available as RPM is at least Hope this helps clarifies. |
So where is that point? 🤔 |
Hi @kloczek , It appears that your expectation is that the |
As I've already wrote in such cases it is better to remove |
It looks like rpm spec files support what you want by adding a patch tag line and using the %patch macro to apply a patch file that removes the |
build output
Why it is not checked cmake command availability add is added only wrapper on system
cmake
command which checks only version of the systemcmake
? 🤔It does not make ANY sense.
The text was updated successfully, but these errors were encountered: