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

[SuiteSparse] Update and split into sub-packages #39297

Closed
wants to merge 98 commits into from

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Jun 14, 2024

SuiteSparse consists of a suite of mostly independent libraries, where each of them:

  • has a distinct versioning scheme,
  • varying license terms (e.g. some are GPL, while others are more permissive),
  • installs a separate CMake config file (i.e. you would do find_package(AMD) to use suitesparse-amd),
  • can have library-specific options, e.g. for CUDA support.

So I think splitting the package into its constituent sub-packages is well-justified.

Fixes #24252.


  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@Neumann-A
Copy link
Contributor

(i.e. you would do find_package(AMD) to use suitesparse-amd),

That looks like a no go. Is there an equivalent of find_package(Suitesparse COPMPONENTS AMD) ?

@valgur
Copy link
Contributor Author

valgur commented Jun 14, 2024

(i.e. you would do find_package(AMD) to use suitesparse-amd),

That looks like a no go. Is there an equivalent of find_package(Suitesparse COPMPONENTS AMD) ?

AFAIK SuiteSparse does not export a SuiteSparse CMake module or config.
Both Ceres and g2o (the only users of the suitesparse port, currently) define a custom FindSuiteSparse.cmake.

@Neumann-A
Copy link
Contributor

Yeah I checked upstream. Their find_package logic is flawed but nothing for you to fix. There is a Suitesparse_config package but that won't help much.

They should really install Suitesparse-config.cmake and use the component approach instead of just taking common abbreviations. SuitesparseAMD-config.cmake would be much better compared to just AMD-config.cmake.

Parallel configure fails due to race conditions with configure_file().
@valgur valgur mentioned this pull request Aug 21, 2024
7 tasks
MonicaLiu0311
MonicaLiu0311 previously approved these changes Aug 27, 2024
ports/colmap/vcpkg.json Show resolved Hide resolved
ports/suitesparse-amd/portfile.cmake Show resolved Hide resolved
ports/suitesparse-config/portfile.cmake Outdated Show resolved Hide resolved
ports/theia/fix-external-dependencies.patch Outdated Show resolved Hide resolved
scripts/ci.baseline.txt Show resolved Hide resolved
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Aug 29, 2024
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 29, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Aug 30, 2024

The graphblas package made me study the package a little more. There are hard problems. (But maybe it is already broken now.)

  • The configuration creates files in the source dir which capture per-build-type settings, incl. the compiler. One build type will see the wrong config unless running serializing config+build differently.
  • Cross-builds with JIT (default, controlled by compact) rely on a cross-built external project for a host tool. In vcpkg, this should come from the host triplet instead.
  • openmp off by default may not match upstreams recommendation printed in the config log:
CMake Warning at CMakeLists.txt:462 (message):
  WARNING: OpenMP was not found (or was disabled with GRAPHBLAS_USE_OPENMP).
  See the GraphBLAS user guide on the consequences of compiling GraphBLAS
  without OpenMP.  GraphBLAS will work but may not be thread-safe, since it
  relies on '#pragma omp flush' to ensure the work performed by one user
  thread is available to another, in GrB_wait.  If OpenMP is not in use, the
  thread-safety of GrB_wait becomes the responsibilty of the user application
  (perhaps through a pthreads construct).  Compiling GraphBLAS without OpenMP
  is not recommended for installation in a package manager (Linux,
  conda-forge, spack, brew, vcpkg, etc).

-DSUITESPARSE_USE_STRICT=ON
-DSUITESPARSE_USE_FORTRAN=OFF
-DSUITESPARSE_DEMOS=OFF
-DGRAPHBLAS_JIT_ENABLE_RELOCATE=ON
Copy link
Contributor

Choose a reason for hiding this comment

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

Another note: This overrides upstream's default, and this is a point where upstream chooses a different default for APPLE.

@dg0yt
Copy link
Contributor

dg0yt commented Aug 31, 2024

More on suitesparse-graphblas:

Postponing compilation to JIT, as modeled with feature "compact", saves a lot of build time. But this also implies that a significant part of the build is moved out of vcpkg, to a different time, maybe to a different machine (with the frozen absolute compiler paths and compilation flags). IMO this is fragile, at least from a more general vcpkg POV. I wonder if there are better models.

And I also wonder if "compact" is additive in a vcpkg feature sense, or if there should be "precompiled" instead, adding the extra build time and robustness when selected. With "core" being cheap, it would also be cheap to depend on the host port for the extra tool needed for JIT (if this dependency is feasible at all).


(Most of this might be relevant already for the current state of suitesparse, so it should not stop the PR. I just want to make sure that the split helps to move to an improved situation, maybe in following PRs.)

@vicroms
Copy link
Member

vicroms commented Sep 3, 2024

We discussed this PR in our last meeting, we think it is a good direction.

This is still pending thorough review, and we are a bit concerned about the upstream package names but will refer to whatever other package managers are doing.

ports/suitesparse-cxsparse/vcpkg.json Show resolved Hide resolved
ports/suitesparse/update_suitesparse.py Show resolved Hide resolved
ports/suitesparse/vcpkg.json Show resolved Hide resolved
@vicroms vicroms removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 12, 2024
@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Sep 12, 2024
@BillyONeal BillyONeal marked this pull request as draft September 12, 2024 19:13
Comment on lines +19 to +20
+ find_package(CHOLMOD CONFIG REQUIRED)
+ find_package(SPQR CONFIG REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

This is not a change request, I'm just curious as to how "transparent" these changes are:

  • Do the changes in this PR require that we do this for all downstream consumers?
  • Could we leave find_package(suitesparse CONFIG REQUIRED) and do the find_package updates in a follow-up PR?

@MonicaLiu0311
Copy link
Contributor

@valgur

@vicroms
Copy link
Member

vicroms commented Oct 28, 2024

@valgur

We are closing this PR due to inactivity. We like the changes here and would like to see them merged when you have time to address the requested changes (mainly moving the generation script to the scripts folder). Feel free to open a new PR when it's ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[suitesparse] Please split up SuiteSparse into its constituent packages
6 participants