Bug 2240777 - De-Bundle Eigen3
Summary: De-Bundle Eigen3
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: CGAL
Version: rawhide
Hardware: Unspecified
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Laurent Rineau
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-09-26 13:11 UTC by Cristian Le
Modified: 2023-10-04 12:51 UTC (History)
1 user (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-09-26 14:08:59 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Cristian Le 2023-09-26 13:11:59 UTC
This package should de-bundle Eigen3 so that it uses the correct package from Fedora. I did not go through the other components, but maybe there are other ones that need to be de-bundled like libSSH.

Reproducible: Always

Comment 1 Laurent Rineau 2023-09-26 13:30:20 UTC
Hi Christian I am the maintainer of CGAL in Fedora, and also an upstream developer of the CGAL project.

As far as I know, CGAL does not bundle Eigen3 or libSSH. Can you please point out what makes you think it does?

Comment 2 Cristian Le 2023-09-26 13:51:27 UTC
If we look at https://fedora.pkgs.org/rawhide/fedora-x86_64/CGAL-devel-5.6-1.fc39.x86_64.rpm.html, you find packaged files like `/usr/share/cmake/CGAL/FindEigen3.cmake`. It is not Eigen3 specifically that is bundled, but the `FindEigen3.cmake`. These files should not be packaged, and instead the upstream config.cmake file should be used. Edge-case is if there is some compatibility for cmake/pkg-config, but when packaging, those should be de-bundled to get the system versions and not affect other downstream users.

Comment 3 Laurent Rineau 2023-09-26 14:08:59 UTC
I agree with you that CGAL should not rely on files like `FindEigen3.cmake` or other similar `Find<pkg>` CMake modules when the software `<pkg>` supports the config mode of the CMake `find_package` command.

That is not a Fedora bug, and it should be reported upstream in the CGAL issue tracker on Github: https://github.com/CGAL/cgal/issues.

If that file `FindEigen3.cmake` really constitutes a problem in the Fedora distribution, please reopen this bug and explain why.

Comment 4 Cristian Le 2023-09-26 14:39:33 UTC
Eigen3 is only one package, the other bundled ones should be checked. In some cases it is a valid strategy, e.g. I am designing like that for octopus in order to check either built-in cmake or pkg-config. But those files will not be present when packaging. That is why these issues should be fixed in the packaging. Granted the `FindEigen3.cmake` is not desisgned with that compatibility in mind, but I did not go through the other one to check if there is one where this is actually recommended.

Comment 5 Laurent Rineau 2023-09-29 09:34:22 UTC
The cmake/CGAL/ directory have Find<pkg> modules with CGAL-specific changes. In the Fedora distribution, we cannot just remove a file from that directory, and expect that nothing will break. That issue must be reported upstream, at https://github.com/CGAL/cgal/issues or directly as a pull-request. The CGAL project will test that pull-request in the CGAL test suite infrastructrure, and then merge it.

Once a pull-request is merged upstream, I can backport it into Fedora current CGAL version, if you want.

Anyway I still do not understand what is your real need. Is that just a complain "Not Quite the Right Thing", about CGAL shipping those files, or do you really have a usage of CGAL that exposes a bug because of that file `cmake/CGAL/FindEigen.cmake`?

Comment 6 Cristian Le 2023-09-29 11:39:02 UTC
Primarily it is "Not Quite the Right Thing". The cmake structure adopted there is hard to navigate and has plenty of issues, e.g. `cmake_minimum_required 3.1` has to be dropped because more recent versions are explicitly dropping the 3.1 compatibility. If I give a review, there will be at least 10 points that will be raised, and would take me half a day to write it up. If you think they are receptive to such a review I can open one up. A few common points could be found in the comments on the cp2k design [1].

About the modules specifically
- These issues will be picked up at the `%build` and `%check` stage (also why are the Testsuite not run in the `%check`?)
- The modules should be investigated manually (at least from time-to-time).
  For example if you check `FindEigen3.cmake` is not authored by the project and instead it is a copy from the cmake module. In such a case the file has to be marked as `bundled` [2], especially as the license is different there (BSD-2-clause).
- The design they have implemented [3] makes it that the provided `Find<PkgName>.cmake` will always take precedence whenever another cmake project uses `find_package(CGAL)` it will always take precedence over the system ones. This has so far not created any issues because there are no dependent packages depending on `CGAL-devel` so these issues are not picked up. But issues can later occur for developing users trying to use `CGAL-devel`.

I can suggest a design upstream that is less intrusive [4,5] if that would help

[1] https://github.com/cp2k/cp2k/issues/2985
[2] https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling
[3] https://github.com/CGAL/cgal/blob/8a4b492f1cb6b8852770df72c76dd503379a7300/Installation/lib/cmake/CGAL/CGALConfig.cmake#L159
[4] https://github.com/LecrisUT/CMakeExtraUtils/blob/FindPackage/cmake/FindPackage.cmake
[5] https://gitlab.com/octopus-code/octopus/-/blob/main/cmake/FindSpglib.cmake

Comment 7 Laurent Rineau 2023-10-03 16:18:58 UTC
(In reply to Cristian Le from comment #6)
> Primarily it is "Not Quite the Right Thing". The cmake structure adopted
> there is hard to navigate and has plenty of issues, e.g.
> `cmake_minimum_required 3.1` has to be dropped because more recent versions
> are explicitly dropping the 3.1 compatibility. If I give a review, there
> will be at least 10 points that will be raised, and would take me half a day
> to write it up. If you think they are receptive to such a review I can open
> one up.

Dear Christian, I think I fully agree with your feedback.

As a matter of fact, I am an upstream developer of CGAL, and the current Release Manager of the CGAL library. I am also one of the main authors of CMake scripts in CGAL, and probably the last "survivor" in the CGAL project: the other main authors of CMake scripts left the CGAL project. I am de facto the maintainer of CMake scripts in CGAL. The support of CMake in CGAL was introduce with CGAL-3.4 in 2009:

  https://www.cgal.org/2009/01/22/cgal-34/

At that time, CMake 2.8 was the latest version, if I remember well, and we had to support CMake 2.4 and 2.6 as well. I do not know if you were already using CMake at that time: the language was quite different, and far from the current version. Since then, there has been several steps of "modernization" of the CMake scripts of CGAL, but we still carry a burden of backward-compatibility, or of incomplete cleaning of now-useless CMake code. And the matter is the lack of dedicated man-power. The proper improvement of CMake scripts in CGAL is always delayed because it is not a priority. The CGAL library is first about implementing good geometry algorithms in C++, and my working time is always first for geometry algorithms, and not the build system... unless something important breaks.

Please report those issues upstream, so that they can eventually be prioritized and handled. I may have sufficient man power to delegate the improvement of our CMake scripts to an intern or a apprentice that I will manage.

(In reply to Cristian Le from comment #6)

> About the modules specifically
> - These issues will be picked up at the `%build` and `%check` stage (also
> why are the Testsuite not run in the `%check`?)

Actually no. CGAL is header only, so `%build` is just a non-op and a copy of the headers.

And `%check` cannot run the CGAL testsuite because it runs for 4-5 hours on a modern x86_64 machine, with at least 8GB of memory, and 3-4 CPU cores. I am not sure Fedora koji builders would appreciate the load.

Comment 8 Cristian Le 2023-10-04 11:30:00 UTC
I will open an issue upstream about the various issues, I'm sorry if it would sound a bit complainy, but there are many modern project standards that are somehow inter-connected. I think most issues can be resolved after a refactor.

Comment 9 Cristian Le 2023-10-04 12:51:55 UTC
If you can share your thoughts as well in https://github.com/CGAL/cgal/issues/7756


Note You need to log in before you can comment on or make changes to this bug.