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

Add package for SuiteSparse 5 #14663

Merged
merged 5 commits into from Dec 22, 2022
Merged

Add package for SuiteSparse 5 #14663

merged 5 commits into from Dec 22, 2022

Conversation

mmuetzel
Copy link
Collaborator

@mmuetzel mmuetzel commented Dec 20, 2022

This addresses #14658.

Most of the libraries in SuiteSparse have an API that can be categorized into two groups: One narrow indexing API and one wide indexing API. In version 5 and version 6, the narrow indexing API uses 32-bit integers. In version 5, the wide indexing API used 64-bit integers on 64-bit platforms and 32-bit integers on 32-bit platforms. In version 6, the wide indexing API uses 64-bit independent of the platform. That cannot work for 32-bit platforms. Indices must not be larger than sizeof(void *). That means half of the API is broken on 32-bit platforms for those libraries.

Some libraries (e.g., SPQR) have only the wide indexing API. That means, they are completely broken on 32-bit platforms.

This PR adds a package that is basically the version of SuiteSparse as it was packaged before the update to version 6.
It currently reverts to SuiteSparse 5 only for 32-bit Octave. A couple of other packages probably also need to be rebuilt with the older version of SuiteSparse (likely ceres-solver and python-cvxopt). The other dependers don't need to be rebuilt. But they should probably still depend on SuiteSparse 5 for their 32-bit packages.

I'm not sure if the approach in this PR is the right way to handle this situation.
I'd be happy for any feedback.

@mmuetzel
Copy link
Collaborator Author

@MehdiChinoune: Could you please comment with your thoughts on this (because you pushed the update to SuiteSparse 6)?

@mmuetzel
Copy link
Collaborator Author

Is the condition I used in the PKGBUILD for octave ok? Should we use the same condition in all packages that depend on suitesparse currently?

@lazka
Copy link
Member

lazka commented Dec 21, 2022

Is the condition I used in the PKGBUILD for octave ok? Should we use the same condition in all packages that depend on suitesparse currently?

If you think they are broken too, then yes. But we can also do this in a separate step/PR once octave is happy again.

@lazka
Copy link
Member

lazka commented Dec 21, 2022

Just wondering, are there any plans to drop Windows 32bit support in octave?

@mmuetzel
Copy link
Collaborator Author

mmuetzel commented Dec 21, 2022

Just wondering, are there any plans to drop Windows 32bit support in octave?

We are running into more and more issues with the 32-bit version. There hasn't been a decision yet. But it's not unlikely that support will be dropped at some point in the not so far future. Octave 9 will likely no longer support Windows 32-bit (unless someone motivated will step in and maintain it).
I was still hoping Octave 8 would run on 32-bit Windows. But, I guess we'll see if it does...

Edit: I was hoping we could build for 32-bit Windows until there is a (working) FOSS Fortran compiler for Windows on ARM. But there might not be many users on older versions of that OS anyway...

@mmuetzel
Copy link
Collaborator Author

If you think they are broken too, then yes.

The package grokker noticed that ceres-solver and python-cvxopt needed to be rebuilt for SuiteSparse 6. So, they are likely using the same API that broke for 32-bit applications with the update to SuiteSparse 6.
The other packages probably don't need to be rebuilt. If I understand the provides logic correctly, they would also be happy with suitesparse5 being installed even if they depend on suitesparse. Is that right?

@lazka
Copy link
Member

lazka commented Dec 21, 2022

The other packages probably don't need to be rebuilt. If I understand the provides logic correctly, they would also be happy with suitesparse5 being installed even if they depend on suitesparse. Is that right?

yes, if they are running/working is another question. With the provides being versioned they could switch to "suitesparse>=6" for that to no longer work.

Real packages get always preferred for installation compared to provides, btw.

@mmuetzel
Copy link
Collaborator Author

yes, if they are running/working is another question. With the provides being versioned they could switch to "suitesparse>=6" for that to no longer work.

I'd guess they use the part of the API that didn't change with the update. So, there is probably no need to restrict to newer versions. Adding the version condition would probably also cause issues if users would like to install packages that need SuiteSparse 5 together with package that really don't care which version is there.
If we care about that all packages are actually built with the version of SuiteSparse with which they will be used eventually, we should probably prefer switching all 32-bit packages to suitesparse5. (But that is probably not mandatory.)

@lazka
Copy link
Member

lazka commented Dec 21, 2022

Note that if they use any new symbols then they will fail to link at runtime, even if they never use them normally. (I have no clue about suitesparse etc, please keep that in mind :) )

@mmuetzel
Copy link
Collaborator Author

mmuetzel commented Dec 21, 2022

Another thing to keep in mind: SuiteSparse is a collection of libraries. Some of those libraries that didn't build with SuiteSparse 5 do build with SuiteSparse 6.
Some dependers might check on compile time which of those libraries are present and working (e.g., Octave does). Depending on that test results, features might be enabled if the "normal" suitesparse package is installed that won't work with the suitesparse5 package (because it might not include the respective libraries).

To be on the safe side, we could rebuild "everything". Or we could wait until issues are reported...
Or as a third option: We could already change the PKGBUILD but not bump the pkgrel yet.

@mmuetzel
Copy link
Collaborator Author

I installed the build artifacts for MINGW32 from this PR. Using them, the example in #14658 no longer crashes Octave.

@mmuetzel mmuetzel marked this pull request as ready for review December 21, 2022 19:25
@mmuetzel
Copy link
Collaborator Author

mmuetzel commented Dec 22, 2022

Is there a way to have the packages built with the suitesparse5 package on 32-bit, but at the same time allow (some of) them to run with the suitesparse package (which is currently at version 6.x)?
Assuming this is possible somehow: Would that be a reasonable approach to allow packages that require suitesparse5 to be installed side-by-side with packages that would also work with suitesparse?

Edit: Would adding suitesparse5 to makedepends while keeping suitesparse in depends do that for those packages?

@lazka
Copy link
Member

lazka commented Dec 22, 2022

Is there a way to have the packages built with the suitesparse5 package on 32-bit, but at the same time allow (some of) them to run with the suitesparse package (which is currently at version 6.x)?

If the versions are parallel installable, sure. But it doesn't look like they are since there is no version in any of the filenames.

Assuming this is possible somehow: Would that be a reasonable approach to allow packages that require suitesparse5 to be installed side-by-side with packages that would also work with suitesparse?

Edit: Would adding suitesparse5 to makedepends while keeping suitesparse in depends do that for those packages?

I assume they are not ABI compatible, no?

@lazka lazka merged commit e32b682 into msys2:master Dec 22, 2022
@mmuetzel
Copy link
Collaborator Author

If the versions are parallel installable, sure. But it doesn't look like they are since there is no version in any of the filenames.

They are not parallel installable. But they should (mostly) work interchangeably.

I assume they are not ABI compatible, no?

Half of the interface should be ABI compatible between versions. The other half (that is broken in version 6) is not ABI compatible.
Programs that only use functions from the half that didn't break should be working with either version of SuiteSparse.

@lazka
Copy link
Member

lazka commented Dec 22, 2022

That all seems very brittle to me, I'd rather have things run against the things they build tbh. Note that we don't have many 32bit users left, so limiting what they can install in parallel might not be that bad, as long as things are not in the same dependency chain.

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

Successfully merging this pull request may close these issues.

None yet

2 participants