Fix new suitesparse version and Apple Clang compiling

[AlttiJokinen]

https://swift-nav.atlassian.net/browse/OC-729

Handle new Suitesparse version and dix Apple Clang compiling

Submodule links need to be corrected for Cmake and eigen after these PRs are merged
swift-nav/cmake#171
swift-nav/eigen#4

[jbangelo]

I'm going to let @peddie review this since he's much more familiar with the uses of suitesparse here. I'd prefer to not have preprocessor macros sprinkled throughout the code, but I'm not sure if there's a better option.

Could we enforce a specific version of suitesparse instead of trying to support multiple versions?

[AlttiJokinen]

@jbangelo The problem without macros is that we will break someone's build setup, if we force to use newer version.
Or another option would be removing these asserts and do handling like the new version, but not sure, if it will break something with old version.

My solution is at least safe and not breaking backward compatibility.

[peddie]

@AlttiJokinen thanks for identifying this and making a fix; sorry the details of this are so annoying.

@isaactorz can you think of any reasonable way to solve this at the cmake level rather than in the C++ code? can we enforce that everyone upgrade to suitesparse v5 somehow? can Bazel always use v5?

[jungleraptor]

@AlttiJokinen thanks for identifying this and making a fix; sorry the details of this are so annoying.

@isaactorz can you think of any reasonable way to solve this at the cmake level rather than in the C++ code? can we enforce that everyone upgrade to suitesparse v5 somehow? can Bazel always use v5?

Couldn't we just bump the bazel version so it's the same as whatever the latest homebrew is?

albatross/WORKSPACE.bazel

Line 83 in 95579dd

sha256 = "4cd3d161f9aa4f98ec5fa725ee5dc27bca960a3714a707a7d12b3d0abb504679",

Alternatively we could use a submodule instead (as much as I generally hate the idea of adding more submodules) - so we drop relying on the system altogether.

Either of those are preferable to me than having a macro and running different code depending on which platform/build system you're using.

[jungleraptor]

@AlttiJokinen thanks for identifying this and making a fix; sorry the details of this are so annoying.
@isaactorz can you think of any reasonable way to solve this at the cmake level rather than in the C++ code? can we enforce that everyone upgrade to suitesparse v5 somehow? can Bazel always use v5?

Couldn't we just bump the bazel version so it's the same as whatever the latest homebrew is?

Ahhh nevermind that would probably break linux w/ cmake because we don't really know which version is shipped with their package manager

[peddie]

Is it feasible to at least fail to generate build files and tell the user to go manually install suitesparse 5? is that going too far with cmake?

[jungleraptor]

Is it feasible to at least fail to generate build files and tell the user to go manually install suitesparse 5? is that going too far with cmake?

So my view here when it comes to cmake is that if you're writing Find scripts you should declare the version you're expecting and not try to hack around supporting every package manager:

find_package(suiteparse 7.4 REQUIRED)

That's idiomatic IMO - it clearly tells the person whose trying to build your software what version they need to install.

If it's in their package manager - great, and if not they can build and install from source - which feels geriatric - but that's life in the C++ development ecosystem.

But since writing Find scripts is hard, and it generally sucks for all parties involved to do dependency management this way - it's super common to just also vendor the dependencies (i.e. via a third_party folder + git submodules)

But obviously we're aware of the downsides of that approach when taken to the extreme.

I think it's a pick your poison situation here. I'd probably prefer either of those to having a macro in the code.

[peddie]

My preference is probably to leave the user on her own to install the required version in this case -- bazel is the default build system anyway, and I think at Swift at least we have more Mac users than linux users. @AlttiJokinen would that work for you?

[AlttiJokinen]

@peddie So you would prefer breaking backward compatibility by supporting just suitesparser 7.4 ?

How about Jenkins pipeline, I think that Linux test using cmake would fail ? Or is cmake Linux also taking latest suitesparser ?

I am just wondering that is it worth of benefit to break backward compatibility, because failures in test pipeline ? Who would update necessary Jenkins configurations ?

[peddie]

I'd prefer breaking backwards compatibility by moving to support just suitesparse 7.5. The serialised objects are already not going to be compatible across versions (they have different fields and different behaviors). So what is the concern about compatibility?

I think we would have to get cmake to require suitesparse 7.5 or newer (harder) and fix the Jenkins environment to provide suitesparse 7.5 (easier than working with cmake, I suspect).

[AlttiJokinen]

@peddie I changed in the way like you wanted. However, I don't have idea that how to check version in cmake file, but I added check to suitesparse.hpp
static_assert(SUITESPARSE_VERSION >= SUITESPARSE_VER_CODE(7, 4),
"Suitesparse version 7.4 or greater is required.");

@peddie @isaactorz How do have idea that why Bazel build fails ?
https://github.com/swift-nav/albatross/actions/runs/7497937237/job/20412288195?pr=473

Are these build files somewhere in Swift-repo or they are Suistesoparse ?
ERROR: /home/runner/.cache/bazel/_bazel_runner/f55491f6729ea2dff68834eaead9174e/external/suitesparse/BUILD.bazel:414:11: Middleman _middlemen/@suitesparse_S_S_Ccholmod-cc_library-compile failed: missing input file '@suitesparse//:CHOLMOD/Cholesky/t_cholmod_lsolve.c'

I used Suitesparse 7.4, because it is still latest on in Homebrew. I can see that they just published 7.5 in their site.

[jbangelo]

Someone else wrote a CMake find package for suite sparse, and have the logic to try extracting the version number from the headers here. If we want to check when running CMake we could lift that bit of logic, and probably try to translate it into Bazel as well?

[jungleraptor]

Someone else wrote a CMake find package for suite sparse, and have the logic to try extracting the version number from the headers here. If we want to check when running CMake we could lift that bit of logic, and probably try to translate it into Bazel as well?

In bazel we build from source so no need to translate.

The sources are fetched using an http_archive rule in the WORKSPACE.bazel file:

albatross/WORKSPACE.bazel

Line 81 in 95579dd

name = "suitesparse",

This avoids needing a submodule, and having to deal with finding the packages on the system - but has the downside that we had to write the build files for the library ourself, which are defined here: https://github.com/swift-nav/rules_swiftnav/blob/main/third_party/suitesparse.BUILD.

It looks like if we bump the version we'll have to re-write this build file to match the new sources.

[AlttiJokinen]

Thanks.

@isaactorz I don't see https://github.com/swift-nav/rules_swiftnav as albatross submodule.
If that is true, how I can do changes in synchronized manner ?

If I change https://github.com/swift-nav/rules_swiftnav first before albatross, it will break build for everyone, because old suitesparse is used, but I cannot change albatross before rules_swiftnav, because it will break builds as well.

Is the necessary to create completely new build file for suitesparse 7.4 in https://github.com/swift-nav/rules_swiftnav that it would not break builds ?

[peddie]

@AlttiJokinen rules_swiftnav is brought in from the WORKSPACE.bazel file in albatross, not via a git submodule. When you merge a change that requires a newer version of rules_swiftnav, you change the corresponding hash imported in the WORKSPACE.bazel file.

We don't have to create a completely new build file for suitesparse 7.4 in rules_swiftnav. We just need to update the URL that it fetches the code from and update lists of source / header files to match the new 7.4 file structure.

Side note: 7.5 is out now, so perhaps it's worth going straight to that? I don't fully understand the Macintosh packaging situation that led to this change, but it would be a shame if the versions of dependencies for the whole project were dictated by whatever homebrew happens to be doing.

[AlttiJokinen]

@peddie Yes, Homebrew has also now Suitesparse 7.51

Do you have some automated way to generate suitesparse.BUILD ?
I did changes manually for renaming Core to Utility, but it there are so many changes that this is hard to do manually:
https://github.com/swift-nav/rules_swiftnav/pull/129/files

Also how I can test this new build file ? How I can make this PR to use build file from swift-nav/rules_swiftnav#129 ?

[peddie]

OK, no problem for now. Sorry for the runaround -- didn't realise it would turn into such a time sink.