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

upgrade shipped suitesparse components to 7.8.0 #37204

Merged
merged 11 commits into from
Sep 3, 2024

Conversation

kiwifb
Copy link
Member

@kiwifb kiwifb commented Jan 31, 2024

This is to fix #37192 and supersedes #34206
The new cmake superbuild is leveraged to only build the suitesparse components of interest

  • amd
  • camd
  • colamd
  • ccolamd
  • cholmod
  • umfpack

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

lgtm

@dimpase
Copy link
Member

dimpase commented Jan 31, 2024

please feel free to adjust tags accordingly

@dimpase
Copy link
Member

dimpase commented Feb 1, 2024

the failing doctest

sage -t --long --random-seed=286735480429121101562228604801325644303 sage_setup/clean.py  # 1 doctest failed

looks irrelevant to this PR

@kiwifb
Copy link
Member Author

kiwifb commented Feb 1, 2024

Sorry, I have been busy. I think I need to explain some of the options I set with cmake a little bit more before shipping it.

@kiwifb
Copy link
Member Author

kiwifb commented Feb 1, 2024

Since all I have done in the last commit is add some text, I will label it positive review as approved by Dima.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 1, 2024

Has this been tested on more than one machine?

"CI Linux Incremental" is only being run now that the label "c: packages: standard" is set.
(See #37222 for that.)

@kiwifb
Copy link
Member Author

kiwifb commented Feb 1, 2024

Well, some CI ran before, but yes, I do not think it ran on Mac OS before for example. I'll just push your comma change and remove the positive review label until the full CI ran.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 1, 2024

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2024

Various failures:
"CMake 3.22 or higher is required. You are running version 3.20.2" on https://github.com/mkoeppe/sage/actions/runs/7748353404/job/21130825606#step:14:490 (standard-pre / linux (almalinux-9-python3.11, standard))

In file included from /sage/local/var/tmp/sage/build/suitesparse-7.6.0/src/CHOLMOD/Include/cholmod.h:308,
                   from /sage/local/var/tmp/sage/build/suitesparse-7.6.0/src/CHOLMOD/Include/cholmod_internal.h:33,
                   from /sage/local/var/tmp/sage/build/suitesparse-7.6.0/src/CHOLMOD/Check/cholmod_check.c:65:
  /sage/local/var/tmp/sage/build/suitesparse-7.6.0/src/SuiteSparse_config/SuiteSparse_config.h:583:65: error: expected ')' before ',' token
    583 |     #define SUITESPARSE_BLAS(name,NAME) SUITESPARSE_FORTRAN(name,NAME)
        |                                                                 ^

(https://github.com/mkoeppe/sage/actions/runs/7748353404/job/21130813895#step:14:1077, minimal-pre / linux (ubuntu-lunar, minimal))

Likewise on many other platforms. This sounds familiar from the last attempt. I'll take a closer look in the next days.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2024

The cmake version tightening was already on the old branch, can just cherry pick from there - sagemath/sagetrac-mirror@6f4032e

@kiwifb
Copy link
Member Author

kiwifb commented Feb 2, 2024

Sounds like something that's overdue in any case. I will deal with it ASAP.

@kiwifb
Copy link
Member Author

kiwifb commented Feb 2, 2024

There are three build failure of suitesparse 7.6.0 so far but I do not have enough of the build log to make a diagnostic.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2024

You can see the full build log in the section "Print out logs for immediate inspection", e.g. https://github.com/sagemath/sage/actions/runs/7752508002/job/21142181161?pr=37204, clock on the triangle to expand the collapsed portion

@kiwifb
Copy link
Member Author

kiwifb commented Feb 2, 2024

Thanks, found it. There is mismatch between system gcc (13) and sage's gfortran (12). I am suspecting that's the issue. I think turning off fortran should be OK, so I'll try that.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2024

IIRC, that was also the issue last time. It may be worth trying if just disabling LTO would solve this.

@kiwifb
Copy link
Member Author

kiwifb commented Feb 2, 2024

What Flag would that be?

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2024

-fno-lto, I think; untested

@kiwifb
Copy link
Member Author

kiwifb commented Feb 3, 2024

-fno-lto (which should be correct according to documentation had no effect. Let's see if removing fortran support has an effect.

Copy link

github-actions bot commented Feb 3, 2024

Documentation preview for this PR (built with commit c53fac1; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@kiwifb
Copy link
Member Author

kiwifb commented Feb 5, 2024

The last conda tests appear to be stuck. The failing tests so far look unrelated.

@kiwifb
Copy link
Member Author

kiwifb commented Aug 19, 2024

OK, retrying here, will move 7.8.0 (7.8.1 is identical apart from graphblas which we do not use).

@kiwifb
Copy link
Member Author

kiwifb commented Aug 19, 2024

This PR is now about suitesparse 7.8.0

@mkoeppe mkoeppe changed the title upgrade shipped suitesparse components to 7.6.0 upgrade shipped suitesparse components to 7.8.0 Aug 21, 2024
@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 21, 2024

Portability tests (with a bunch of other upgrades) at https://github.com/mkoeppe/sage/actions/runs/10484245959

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 21, 2024

Builds and tests OK on macOS (x86_64) with homebrew

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 21, 2024

Looking good, thanks for preparing this upgrade!

@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 21, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 27, 2024
    
<!-- ^^^^^
Upgrade the shipped suitesparse components to 7.6.0
-->
This is to fix sagemath#37192 and supersedes sagemath#34206
The new cmake superbuild is leveraged to only build the suitesparse
components of interest
* amd
* camd
* colamd
* ccolamd
* cholmod
* umfpack

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies


<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#37204
Reported by: François Bissey
Reviewer(s): Dima Pasechnik, Matthias Köppe
@vbraun vbraun merged commit 4dffe20 into sagemath:develop Sep 3, 2024
19 of 39 checks passed
@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 3, 2024

@vbraun

  [suitesparse-7.8.0] https://ftp.sun.ac.za/ftp/pub/mirrors/www.sagemath.org/spkg/upstream/suitesparse/SuiteSparse-7.8.0.tar.gz
  [suitesparse-7.8.0] [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx]
  [suitesparse-7.8.0] ERROR [transfer|run:135]: [Errno socket error] [Errno 404] Not Found: '//ftp.sun.ac.za/ftp/pub/mirrors/www.sagemath.org/spkg/upstream/suitesparse/SuiteSparse-7.8.0.tar.gz'
  [suitesparse-7.8.0] Traceback (most recent call last):
  [suitesparse-7.8.0]   File "/home/runner/work/sage/sage/build/bin/sage-package", line 40, in <module> [....]
  [suitesparse-7.8.0] sage_bootstrap.tarball.FileNotMirroredError: tarball does not exist on mirror network

https://github.com/sagemath/sage/actions/runs/10691572603/job/29638332632#step:4:3332

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update SuiteSparse to v.7.6, allow its headers to be in suitesparse/ subdir
4 participants