Set location of Trilinos package and find it

[raisin]

This change is required to ensure that all tests pass when sacado tests are enabled.

On Ubuntu 22.04 LTS, the very last test fails because the Trilinos package is not loaded, which prevents the sacado speed test from building because the required libraries and corresponding symbols are not present. The minor change I added ensures that Trilinos package is found. The following commands can be used to test the last test on the current version fails, but that all tests with the additional tweak I added pass. Here are the commands to reproduce:

1. sudo apt-get update
2. sudo apt-get upgrade
3. sudo apt install python3 python-is-python3
4. pip3 install xrst
5. source ~/.profile
6. sudo apt-get install clang libclang-dev libopenmpi-dev openmpi-bin libeigen3-dev libboost-all-dev libhwloc-dev libpthread-stubs0-dev libadolc-dev coinor-libipopt-dev libcolpack-dev libomp-dev swig python3-dev libmetis-dev
7. git clone https://github.com/coin-or/CppAD.git cppad.git
8. export LD_LIBRARY_PATH=/home/bulky/explicit_solutions_through_backprop/cppad.git/build/prefix/lib64:/home/bulky/explicit_solutions_through_backprop/cppad.git/build/prefix/lib:$LD_LIBRARY_PATH
9. export LD_RUN_PATH=/home/bulky/explicit_solutions_through_backprop/cppad.git/build/prefix/lib64:/home/bulky/explicit_solutions_through_backprop/cppad.git/build/prefix/lib:$LD_RUN_PATH
10. export CMAKE_PREFIX_PATH=/home/bulky/explicit_solutions_through_backprop/cppad.git/build/prefix
11. export CMAKE_INCLUDE_PATH=$CMAKE_PREFIX_PATH/include
12. export CMAKE_LIBRARY_PATH=$CMAKE_PREFIX_PATH/lib64:$CMAKE_PREFIX_PATH/lib:$CMAKE_LIBRARY_PATH
13. cd cppad.git
14. bin/get_colpack.sh
15. bin/get_adolc.sh
16. bin/get_fadbad.sh
17. bin/get_ipopt.sh
18. bin/get_sacado.sh
19. bin/get_cppadcg.sh
20. cd build
21. ln -s prefix/lib64/libColPack.so prefix/lib/libColPack.so
22. cmake .. -Dinclude_adolc=TRUE -Dinclude_ipopt=TRUE -Dinclude_cppadcg=TRUE -Dcolpack_prefix=./prefix/ -Dfadbad_prefix=./prefix/ -Dsacado_prefix=./prefix/ -Dinclude_doc=TRUE -DCMAKE_BUILD_TYPE=Debug -DTrilinos_DIR=$(pwd)/prefix/
23. make check

Two additional issues not fixed by this pull request:

1. The following command was required:

sudo apt-get remove libmumps-dev libmumps-seq-dev libmumps-seq-5.4

If this command isn't run, cpp_ipopt_test tends to link against libdmumps_seq-5.4.so causing a conflict between two mumps libraries. Uninstalling the system packages above prevents this conflict.
2. The symlink introduced in step 21 could cause issues for people. I shouldn't have to introduce this additional step. I'm not sure it is required on all operating systems. However, I noticed one of the other open Issues was caused by a Coloring Package failure on FreeBSD. Not sure whether this issue fixes that.

Here's the output of make check showing that the commands above indeed pass all the tests:

https://dpaste.org/AwAt7

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[bradbell]

The current version of CppAD requires the user to set sacado_prefix on the cmake command line in order to include the Sacado tests; see package_prefix on
https://cppad.readthedocs.io/latest/cmake.html#package-prefix
Does this work for you ?

Packages that have been converted to use automatic detection of the prefix location now use a true of false include_package flag; e.g., see include_adolc on
https://cppad.readthedocs.io/latest/cmake.html#include-adolc
If you are doing this sort of conversion, I would not expect to have to specify
-DTrilinos_DIR=$(pwd)/prefix/

In addition, the cmake command should detect if the user is using the old style cmake command and provide an error message for to how fix it. See the comment # Arguments that are no longer used in the file
https://github.com/coin-or/CppAD/blob/master/CMakeLists.txt
in particular

FOREACH(pkg adolc eigen ipopt cppadcg)
   IF( ${pkg}_prefix )
      MESSAGE(FATAL_ERROR
         "-D ${pkg}_prefix=value has been changed to -D include_${pkg}=true"
      )
   ENDIF( ${pkg}_prefix )
ENDFOREACH(pkg adolc eigen ipopt cppadcg)

Also, the documentation for the cmake command will need to be modified to reflect this change; e.g., see include_adolc in
https://github.com/coin-or/CppAD/blob/master/xrst/install/cmake.xrst

[raisin]

The current version of CppAD requires the user to set sacado_prefix on the cmake command line in order to include the Sacado tests; see package_prefix on
https://cppad.readthedocs.io/latest/cmake.html#package-prefix
Does this work for you ?

No, this doesn’t work.

Longer answer - In isolation, setting secado_prefix is insufficient. I tried setting secado_prefix by itself. When I did this, the the secado speed test failed because of undefined references due to the fact the Trilinos package was not found.

Ensuring this was found required both the following:

1. -DTrilinos_DIR=$(pwd)/prefix/ (Command line argument for the cmake build as mentioned in step 22)
2. The code change to detect the Trilinos package in this pull request.

[raisin]

Packages that have been converted to use automatic detection of the prefix location now use a true of false include_package flag; e.g., see include_adolc on
https://cppad.readthedocs.io/latest/cmake.html#include-adolc
If you are doing this sort of conversion, I would not expect to have to specify
-DTrilinos_DIR=$(pwd)/prefix/

My intention was not to do automatic detection. From the perspective of user friendliness, I agree that would be ideal. However, my goal was simpler. My goal was to ensure all the tests including the secado speed test passed.

It’s possible the code change works without the command line argument:

-DTrilinos_DIR=$(pwd)/prefix/

I will try without this argument and let you know if it works. Even if it works, it wouldn’t change the code portion of this pull request, but it wouldn’t change the documentation portion.

[raisin]

In addition, the cmake command should detect if the user is using the old style cmake command and provide an error message for to how fix it. See the comment # Arguments that are no longer used in the file
https://github.com/coin-or/CppAD/blob/master/CMakeLists.txt
in particular

FOREACH(pkg adolc eigen ipopt cppadcg)
IF( ${pkg}_prefix )
MESSAGE(FATAL_ERROR
"-D ${pkg}prefix=value has been changed to -D include${pkg}=true"
)
ENDIF( ${pkg}_prefix )
ENDFOREACH(pkg adolc eigen ipopt cppadcg)
Also, the documentation for the cmake command will need to be modified to reflect this change; e.g., see include_adolc in
https://github.com/coin-or/CppAD/blob/master/xrst/install/cmake.xrst

My goal was not to change the Cmake flags. From the perspective of documentation, I agree that the command line argument

-DTrilinos_DIR=$(pwd)/prefix/

should be discussed if it’s necessary. However, this argument may not be required. If it’s not required, then I don’t believe any changes need to be made to this merge request. However, I will edit line 22 in the OP and remove the argument:

-DTrilinos_DIR=$(pwd)/prefix/

Let me know if this clarifies and if you agree with my analysis. Thanks

[bradbell]

In the file
https://github.com/coin-or/CppAD/blob/master/speed/sacado/CMakeLists.txt
See the line

SET(Trilinos_DIR "${sacado_prefix}/${dir}/cmake/Trilinos" )

Perhaps you know how to fix this setting or the documentation for your case.

[raisin]

Thanks for your last comment. Now, it's clear. I think we just need to add

find_package(Trilinos REQUIRED)

after line 47 in the file you linked. Also, I believe the cmake option:

-DTrilinos_DIR=$(pwd)/prefix/ 

can be removed. I'll test it and update this pull request tomorrow evening or Wednesday.

[bradbell]

Maybe CMAkeLists we should say something somethig about searching cmake_install_libdirs
file:///home/bradbell/repo/cppad.git/build/html/cmake.html#cmake-install-libdirs
when the Trilinos package cannot be found ?

[raisin]

Adding

find_package(Trilinos REQUIRED)

after line 47 is insufficient. However, it is possible to simply modify the file

https://github.com/coin-or/CppAD/blob/master/speed/sacado/CMakeLists.txt

to get things to work. I was able to add

set(Trilinos_DIR "./build/prefix/lib64/cmake/Trilinos")
find_package(Trilinos REQUIRED)

to the CMakeLists.txt for speed/sacado to get things to work. I still need to test:

1. if find_package(Trillinos REQUIRED) is actually necessary.
2. How to use ${sacado_prefix} and ${dir} to generate the correct path instead of hard coding it.

Once I create and test a clean pull request that respects the style of the build, I'll modify this pull request. I still need to test a couple of things.

[bradbell]

Please test the following changes to the current master branch:

1. In the top level CMakeLists.txt change

command_line_arg(cmake_install_libdirs lib STRING

to

command_line_arg(cmake_install_libdirs lib;lib64 STRING

2. Change your cmake command to

cmake .. -Dinclude_adolc=TRUE -Dinclude_ipopt=TRUE -Dinclude_cppadcg=TRUE -Dcolpack_prefix=./prefix/ -Dfadbad_prefix=./prefix/ -Dsacado_prefix=./prefix/ -Dinclude_doc=TRUE -DCMAKE_BUILD_TYPE=Debug
``

[raisin]

I tried changing the top level CMakeLists.txt with the suggested change, which was to change the line:

command_line_arg(cmake_install_libdirs lib STRING

to

command_line_arg(cmake_install_libdirs lib;lib64 STRING

Furthermore, I changed the cmake command to

cmake .. -Dinclude_adolc=TRUE -Dinclude_ipopt=TRUE -Dinclude_cppadcg=TRUE -Dcolpack_prefix=./prefix/ -Dfadbad_prefix=./prefix/ -Dsacado_prefix=./prefix/ -Dinclude_doc=TRUE -DCMAKE_BUILD_TYPE=Debug

As stated, these changes did not fix the problem. For some reason, the CMakeCache.txt file still reads as follows:

cmake_install_libdirs:STRING=lib

The changes made to the top-level CMakeLists.txt file don't transfer to the generated CMakeCache.txt file. If I manually change the CMakeCache.txt file line above to the following:

cmake_install_libdirs:STRING=lib;lib64

then, everything works perfectly. However, it is awkward to change a generated CMakeCache.txt file like this. I would need to do more digging to figure out how to ensure that the line generated in CMakeCache.txt could be generated properly.

[bradbell]

What happens if you remove the old CMakeCache.txt file before running cmake; i.e., is this happening because you have the old settings in CMakeCache.txt and hence it is ignoring the command line arguments.

[raisin]

I tried deleting CMakeCache.txt. It gets regenerated with only lib and not lib;lib64. However, changing it manually fixes the problem.

Furthermore, running bin/cppadcg.sh and running cmake both generate CMakeCache.txt. In both cases, the generated CMakeCache.txt has only lib as opposed to lib;lib64.

I haven’t traced the code to determine whether
1.) ;lib64 is being dropped by cmake due to its parsing rules,
2.) It is being pulled from a location that has higher precedence than the top level CMakeLists.txt, or
3.) Some other CMakeLists.txt file is overwriting this value. I searched for this, but I didn’t see it anywhere in the codebase.

[bradbell]

Good catch. I think The problem is in cppadcg.sh. It is a kuldge so that I can test how cppad works in the CppADCodeGen context. The problem here is circular dependency because CppADCodeGen depends on CppAD and so must run the CppAD cmake comamnd.

Please try removing cppadcg.sh from your testing.

[raisin]

To add to my previous comment, I tried string escaping the “;”. In other words, I also tried:

command_line_arg(cmake_install_libdirs lib\;lib64 STRING

That didn’t seem to work either. In the code, I had only one backslash. GitHub is also removing a backslash with its string escaping.

[raisin]

Thanks for the note about cppadcg.sh. It’s possible we had the right method with some of the ideas discussed above but weren’t able to get a working solution because we got tripped up with cppadcg.sh. I’ll retest all the methods above without cppadcg.sh. Thanks.

[raisin]

I force pushed a working version. It's basically what you suggested, which was to modify the line

cmake_install_libdirs:STRING=lib

to

cmake_install_libdirs:STRING=lib;lib64

However, the string lib;lib64 needs to be in quotes. Otherwise, cmake reads lib;lib64 as lib and inserts into CMakeCache.txt, and we don't get the intended effect. Also, my previous attempt of using "lib\;lib64" didn't work. We need simply "lib;lib64". That works nicely with and without bin/get_cppadcg.sh. CMakeCache.txt has the correct line and all tests build and work properly. Thanks for your help with this.

[raisin]

Also, I signed the Contributor License Agreement, but it still shows as pending.