Allow users to specify lib & include paths for all dependencies

[andrjohns]

Summary

This PR adds the ability for users to specify the include and library paths for all dependencies, identical to that provided by the TBB_INC and TBB_LIB flags.

This will also eventually be used for linking against system-installed versions of the dependencies.

The new flags are:

• EIGEN_INC
• BOOST_INC
• SUNDIALS_INC
• SUNDIALS_LIB
• OPENCL_LIB
• OPENCL_INC

This PR also implements some basic version-checking for compatibility, and automatically setting defines for the sundials and TBB libraries.

Tests

An additional Github Actions workflow has been added for testing fwd/rev/mix functions with system libraries.

Side Effects

The compiler flags have been altered so that the stan/math/version.hpp file is included first, so that the library versions and flags can be correctly tested/set

Release notes

Added EIGEN_INC, BOOST_INC, SUNDIALS_INC, SUNDIALS_LIB, OPENCL_LIB, OPENCL_INC Makeflags for linking to system/external libraries

Checklist

• Math issue Allow Math library to link to system-installed libraries/headers #2518

• Copyright holder: Andrew Johnson

  The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
  - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
  - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

• the basic tests are passing

  • unit tests pass (to run, use: ./runTests.py test/unit)
  • header checks pass, (make test-headers)
  • dependencies checks pass, (make test-math-dependencies)
  • docs build, (make doxygen)
  • code passes the built in C++ standards checks (make cpplint)

• the code is written in idiomatic C++ and changes are documented in the doxygen

• the new changes are tested

[WardBrian]

This is awesome! Very cool to also have it tested in CI

[WardBrian]

The tests are failing due to the issue #2836 resolved, if you're able to merge develop in it should go away

[andrjohns]

@WardBrian when you've got a minute would you be able to check whether my last updates fix sundials for you?

[WardBrian]

@andrjohns the instances of LIBSUNDIALS as a dependency in make/tests all need to be updated to depend on SUNDIALS_TARGETS and have LDLIBS += $(LIBSUNDIALS). But besides that, yes, it now works!

E.g., for the algebra solver tests on like 86

$(ALGEBRA_SOLVER_TESTS) : $(SUNDIALS_TARGETS)
$(ALGEBRA_SOLVER_TESTS) : LDLIBS += $(LIBSUNDIALS)

Additionally, before this is usable from CmdStan, cmdstan/makefile line 266 (build) needs to change LIBSUNDIALS to SUNDIALS_TARGETS, as does cmdstan/make/program line 55 (%$(EXE)). It's safe to do that in two stages with the way you currently have this set up, though, so we can just open a PR whenever this is done

[andrjohns]

Good catch! Pushed that update now, and then should be ready to review (assuming tests pass!)

I'll update those cmdstan makefiles at the same time as adding support for external CLI11 headers (and rapidjson for stan)

[WardBrian]

I've read over the makefile changes, which all seem reasonable. I'm not confident enough to say whether the use of -include stan/math/version.hpp is a good idea/idiomatic, and I am not familiar with sundials to read over the version-specific code changes

[andrjohns]

Thanks for the help so far @WardBrian!

@wds15 When you've got a minute would you be able to look over the backwards-compatibility changes to the SUNDIALS code in this PR? It's just #ifdef-ing the use of the new context parameters based on whether the SUNDIALS version is 6.0 or greater

@bob-carpenter and @rok-cesnovar could I ask for your higher-level perspective on whether this version-checking approach fits in the Stan ecosystem? Background below:

As part of allowing users to provide external dependencies we need to perform some compatibility-checking (either erroring outright, or setting flags for the pre-processor). We can't do this purely through the Makefile since we have to check the values/macros set by the external dependencies (e.g., SUNDIALS_VERSION_MAJOR or EIGEN_VERSION_AT_LEAST). My approach for this is to perform the compatibility checks in the stan/math/version.hpp header and then include that first as part of the CXXFLAGS only if external dependencies are provided. So the compilation behaviour/flags is only different for those using external dependencies.

Does that sound reasonable or is there a better approach that I'm missing? Thanks!

[bob-carpenter]

Don't the libraries already have environment variables in the makefiles for their location?

This PR also implements some basic version-checking for compatibility

I didn't understand the description of the solution or what exactly was being proposed for version.hpp. Is there a practical problem users are running into that this is intended to solve? For instance, better warning messages or automatically finding versions or something?

[wds15]

Are these sundials ifdefs introduced to make things work with older sundials versions?

not sure we want to support multiple versions..the next sundials version bump will require again changes to the code due to their new logger stuff. So keeping things working with the latest sounds like enough to worry about for me.

[andrjohns]

Are these sundials ifdefs introduced to make things work with older sundials versions?

Yep. The versions of SUNDIALS packaged in most distros at their current/latest release is still at 5.8.0:

• Ubuntu 22.10
• Debian Testing
• Fedora 37
• Enterprise Linux 8 (CentOS 8, RHEL 8, Rocky Linux 8, AlmaLinux 8)

If there are major API changes in the next Sundials versions and the benefits are worth sacrificing backwards-compatibility completely, we can restrict the minimum Sundials version. But I'd prefer not to do that if possible - since it makes packaging and portability for users a bit trickier

[andrjohns]

Is there a practical problem users are running into that this is intended to solve? For instance, better warning messages or automatically finding versions or something?

There have been several requests for linking/compiling Stan Math against external/system versions of the dependencies, rather than the bundled Eigen/Boost/Sundials/TBB.

The issue here is that the versions of these dependencies in package repos (or on cluster) can lag behind, so we need to include compatibility checks if a user attempts to use the Math library with a version of the dependency that's too old.

Don't the libraries already have environment variables in the makefiles for their location?

These are defined as macros in the library's C/C++ headers, not environment variables, so they need to be checked by the pre-processor before it trying to compile.

I didn't understand the description of the solution or what exactly was being proposed for version.hpp

Because these version macros need to be processed/checked before the compiler encounters incompatible code, my solution was to force the compilation to process the macros first. The macro checks are implemented in stan/math/version.hpp, and passing -include stan/math/version.hpp to CXXFLAGS (only if there are external dependencies used) results in that header (and the compatibility checks) being processed before any other includes.

[wds15]

Is it really needed that all libraries are coming from the OS? Why can't we make Sundials being linked from the OS only if the version is new enough and all other cases we just take the shipped one? Sundials is very unproblematic as we link statically to it.

And yes, the next Sundials version comes with improvements which could be nice to have ... and it requires again changes to the source (I tried already to upgrade and it failed given our current source).

[WardBrian]

Sundials is very unproblematic as we link statically to it.

Sundials has been the hang up for me in my previous couple attempts to compile Stan with MSVC on windows. That said, I can use the same version we do from something like Conda, I’m not relying on system package repos.

I think that the version.hpp trick is useful even if we don’t want to support older versions, since detecting them and then using #error is still useful

[andrjohns]

That's fair, I can drop the Sundials changes and just set the version check to error.

My eventual goal is to package CmdStan, Stan, and Math for linux distros, with packages for the headers repos and cmdstan binaries like:

• libstanmath-dev
• libstan-dev
• libcmdstan-dev
• cmdstan

So for Stan Math I can just have patches like those currently in this PR for Sundials compatibility.

Note that I'll be opening a design doc for this once I've added external dependency linking to both cmdstan and Stan, so that everyone can sign-off on structure and approach before any packages get submitted to repos

[andrjohns]

@hsbadr do you have any suggestions/issues with this PR? You were the one that originally introduced the external TBB compatibility, so let me know if any of this conflicts with your perspective of how things should be handled

[andrjohns]

@syclik would you be able to review this PR? For allowing the Math library to link to external dependencies

[syclik]

Yup.

[syclik]

@andrjohns, I'm going to leave a bunch of comments.

Mind if we have a chat? or can you summarize the goals of what you're trying to accomplish?

I see something like this and I know it's not possible:

My eventual goal is to package CmdStan, Stan, and Math for linux distros, with packages for the headers repos and cmdstan binaries like:

• libstanmath-dev

at least not the way things are done right now. So... before spinning wheels a whole lot, let's chat about it.

Let me know how you want to meet. Slack? Video?

[syclik]

@andrjohns, it looks like most of the PR isn't necessary; I think we can just use the existing makefile without most of these changes.

At the very least, we should be documenting it better so you know that this is all possible.

[syclik]

[EIGEN_INC]: We don't need a new variable. This is literally the existing EIGEN make variable. Unless we want this to be renaming all the includes.

[syclik]

[BOOST_INC]: this should just be BOOST

[syclik]

This is missing the proper include directory.

[syclik]

[TBB_INC]: this is the same as the TBB variable. We should just use that. Also, this isn't pointing to the proper include directory.

[syclik]

this looks wrong for the TBB include library (By inspection). An example from doxygen:

TBB_LIB="$TBB/lib/intel64/gcc4.8"

I'd expect there to be a TBB somewhere.

[syclik]

[OPENCL_INC]: this should just be OPENCL and the include is not correct.

[syclik]

We should remove this variable in favor of just using the BOOST variable. There's no need to do this if statement.

Let's chat if you need help understanding how make works. (The doc is really tough to understand, so happy to walk you through it.) There are two things that really stand out that we shouldn't do here:

1. ifdef BOOST_INC ... the ?= is the operator you want to use.
2. CXXFLAGS... we're not including the C++ includes in our CXX flags. We've managed to get things consistenly split out by having INC_* and separating that from CXXFLAGS_*. There really aren't codified rules for make anywhere, but let's follow how make itself uses these variables and their naming convention so we look more standard than just inventing a bunch of stuff that makes it harder to manage than it already is.

[syclik]

same comment as above

[syclik]

??? what's SUNDIALS_INTERFACE_OLD?

this looks like some testing that got included.

[syclik]

this shouldn't be necessary.

[syclik]

This looks like it'll be useful, but maybe not done this way? I'm not sure if this is where we should check. Maybe it is. Can you walk through the logic?

[syclik]

@andrjohns, I think you should be able to do what you're trying to do with the existing makefile. Were you not able to?

[WardBrian]

I know that our existing makefiles aren't able to link against sundials as a shared library, which this makes it able to

[syclik]

I know that our existing makefiles aren't able to link against sundials as a shared library, which this makes it able to

Thanks for the example. Mind being more specific? I don't see anything in this PR that would enable that behavior where the original didn't... I could be missing something.

(if there's a discourse thread or something to show the shared library linking instructions, let me know and I'll try to replicate.)

[WardBrian]

It was discussed earlier in this PR: #2834 (comment)

The short version is we currently link to sundials by explicitly passing the paths to the .a files. This seems to be done so that we can use the same make variable as a target for building Sundials. If you install sundials from either your system package manager or something like conda, it might come as a shared library

[syclik]

@WardBrian, thanks for the link to the comment.

• It looks like when LIBSUNDIALS was introduced (or changed), it was added with :=, which ignores everything else from the outside.
• I think the easier fix is to replace that with ?= and that would accomplish what you're looking for. But we'd have to test it to make sure.

At the very least, it'd be more consistent with the rest of the flags by using ?=.