Skip to content
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

[LinearSolver.Direct] Fix metis dependency #4450

Merged
merged 17 commits into from
Feb 27, 2024

Conversation

olivier-roussel
Copy link
Contributor

@olivier-roussel olivier-roussel commented Jan 22, 2024

Continuation of #4266

As required by conda-forge/staged-recipes#23085, we must avoid static libraries to comply conda-forge rules.
Tests with more recent versions of metis (such as one available on conda-forge) seems to introduce some bugs (@bakpaul @alxbilger Do you have some inputs to provide here ?).
For now, the solution we converged to is to dynamically link metis with Sofa.


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@olivier-roussel olivier-roussel added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request pr: backport todo This PR will be backported into the release preceeding its milestone. topic for next dev-meeting PR to be discussed in sofa-dev meeting labels Jan 22, 2024
@olivier-roussel olivier-roussel removed the pr: status to review To notify reviewers to review this pull-request label Jan 22, 2024
@olivier-roussel olivier-roussel marked this pull request as draft January 22, 2024 17:46
@alxbilger
Copy link
Contributor

Yes, I don't have the exact commit but, after the version 5.1.0, some changes lead to a crash. Before the changes, it works.
It has been fixed later in this commit: KarypisLab/METIS@36262ad. It means the versions in between both commits cannot be used without a crash.

Note also that the version number (in the code) has not been updated properly along the history of the repository: KarypisLab/METIS@f5ae915. It went from 5.1.0 to 5.2.1, whereas there is a 5.1.1 release.

Finally, less importantly, in SOFA we need a portable random generator in METIS for reproductible results. It is enabled via a compilation option METIS-GKLIB_GKRAND. This is not enabled by default in releases.

@hugtalbot hugtalbot added the pr: status to review To notify reviewers to review this pull-request label Jan 23, 2024
@fredroy
Copy link
Contributor

fredroy commented Jan 23, 2024

[ci-build][with-all-tests]

@olivier-roussel
Copy link
Contributor Author

The bug introduced in metis somewhere between v5.1.0 and v5.2.1 looks similar to conda-forge/gtsam-feedstock#21.
As it is fixed in v5.2.1, we should probably wait that this version comes in conda forge, there is a quite recent and active PR on this: conda-forge/metis-feedstock#41.

@olivier-roussel
Copy link
Contributor Author

@bakpaul This would mean that you don't need to port v5.1.0 branch on your fork, we would rely on v5.2.1 both for external package (including conda) and for the fetch of your fork, which already has a v5.2.1 branch.

@olivier-roussel olivier-roussel changed the title [LinearSolver.Direct] Dynamically link with embedded metis [LinearSolver.Direct] Fix metis dependency Jan 25, 2024
@olivier-roussel
Copy link
Contributor Author

@bakpaul I'm afraid the current PR to have a metis 5.2.1 package on conda-forge (conda-forge/metis-feedstock#41) is stalling... and as the official repository is not active anymore (PR not merged anymore neither) and the official build chain is broken, it can take a lot of time before a 5.2.1 comes up on conda-forge.
I think we would better be safe and ensure that we have a working solution, so we would need you to backport the changes of your metis fork on the 5.1.0 branch, in order to have a consistent version of used metis, wether it's external or fetched dependency.

@bakpaul
Copy link
Contributor

bakpaul commented Feb 12, 2024

I've added the v5.1.0-ModernInstall branch to our fork. Actually this code wasn't on the repo itself so I just copied the files presents in SOFA and added my CMake configuration files. It compiles, you can try to add the find or fetch mechanism using this

message(SEND_ERROR "Metis version ${Metis_VERSION} found in ${Metis_INCLUDE_DIR}, "
"but exact version ${Metis_FIND_VERSION} is required")
endif()
# message(STATUS "Metis version found: ${Metis_VERSION} in ${Metis_INCLUDE_DIR}, ${Metis_FIND_VERSION} was required ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to message or not? If not, please remove this line

@olivier-roussel olivier-roussel marked this pull request as ready for review February 19, 2024 14:56
@olivier-roussel olivier-roussel marked this pull request as draft February 19, 2024 14:56
@bakpaul
Copy link
Contributor

bakpaul commented Feb 19, 2024

We will also need to install metis library to distribute it with the binaries. I didn't have time to make it work, I'll try tomorrow.

@bakpaul bakpaul added this to the v23.12 milestone Feb 20, 2024
@bakpaul
Copy link
Contributor

bakpaul commented Feb 20, 2024

[ci-build][with-all-tests][force-full-build]

@bakpaul
Copy link
Contributor

bakpaul commented Feb 21, 2024

So now the linkage of metis is PRIVATE in Sofa.Component.LinearSolver.Direct and every other target requiring metis finds the package itself (e.g. SofaMatrix) .

One problem is that FetchContent_MakeAvailable make the target available for the current target and its child (that is why the test in Sofa.Component.LinearSolver.Direct is able to link to metis without the need of finding it) but other unrelated targets cannot find it. So we cannot keep it Public because it will not be found by the other target linking to Sofa.Component.LinearSolver.Direct because a lot of them doesn't require metis

@bakpaul bakpaul marked this pull request as ready for review February 21, 2024 16:20
Copy link
Contributor

@fredroy fredroy left a comment

Choose a reason for hiding this comment

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

Looks fine to me otherwise 🤷‍♂️

applications/plugins/SofaMatrix/CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Frederick Roy <fredroy@users.noreply.github.com>
@bakpaul bakpaul requested review from alxbilger and fredroy February 23, 2024 14:06
@bakpaul bakpaul merged commit 6fab76a into sofa-framework:master Feb 27, 2024
7 of 11 checks passed
bakpaul added a commit that referenced this pull request Feb 27, 2024
* Link dynamically with embedded metis to comply conda-forge rules

* export all symbols (for MSVC)

* Use external 5.1.0 metis or fallback to embedded code linked dynamically

* fix comments in metis cmake module

* ADD find or fetch mechanism

* Remove embeded metis and put linkage to private

* Fix metis dependency but still not the best fix

* test

* Put back public linkage

* Revert "Put back public linkage"

This reverts commit 71baa47.

* Clean

* cleaning #2

* Use SOFA_ALLOW_FETCH_DEPENDENCIES cmake option for metis

* Add missing CImg to list of fetchable dependencies in doc

* Update applications/plugins/SofaMatrix/CMakeLists.txt

Co-authored-by: Frederick Roy <fredroy@users.noreply.github.com>

---------

Co-authored-by: Frederick Roy <froy@lnrobo.com>
Co-authored-by: Paul Baksic <30337881+bakpaul@users.noreply.github.com>
Co-authored-by: bakpaul <bakpaul@hotmail.fr>
Co-authored-by: Paul Baksic <paul.baksic@outlook.fr>
Co-authored-by: Frederick Roy <fredroy@users.noreply.github.com>
@bakpaul bakpaul added pr: backport done This PR has been backported into the release before its milestone. and removed pr: backport todo This PR will be backported into the release preceeding its milestone. labels Feb 29, 2024
@hugtalbot hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: backport done This PR has been backported into the release before its milestone. pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed topic for next dev-meeting PR to be discussed in sofa-dev meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants