Interface fixes and enhancements

guestieng commented 2 years ago

The following problems have been fixed:

there has been missing a "proper" (non-depreciated, non-pkgconfig) cmake interface missing; particularly, targets are not exported (now, by depending projects, very simple usage of cppad via find_package(cppad <...>) and target_link_libraries(<...> cppad <...>) is possible)
linking error (not finding definition of local::temp_file)
conflicts for product entries of different types ("numeric"/"AD"): compile error C2039: 'ReturnType': is not a member of 'Eigen::ScalarBinaryOpTraits...'

Moreover, more support for depending projects can be provided now via transitive dependencies (cmake/cppadConfig.cmake.in)

CLAassistant commented 2 years ago

All committers have signed the CLA.

bradbell commented 2 years ago

It seems that three things are going on here:

Change the CppAD top level CmakeLists.txt so that FIND_PACKAGE( cppad ... ) and TARGET_LINK_LIBRARIES( cppad ... ) work ?
Change the CppAD top level CmakeLists.txt file to provide a make unistall command ?
Fix some problem related to using CppAD with Eigen ?

guestieng commented 2 years ago

First of all, to avoid confusion, I have only accidently(!) closed this pull request (then reopened it!) .

Then, as to the questions:

yes (commit 90aca75; ... supporting config. mode)
no, this feature has been implemented already before
yes (commit e10c38c)

Furthermore:

linking error (not finding definition of local::temp_file) : commit 90aca75 (CMakeLists.txt: line 566-569)
more support for depending projects can be provided now via transitive dependencies (cmake/cppadConfig.cmake.in): commit 6e4a74b

bradbell commented 2 years ago

Just for eigen changes (item 3 above): Would you please make a new pull request that includes an automated test that fails with the current master but passes with the eigen changes you are suggesting.

For this case, I think you can add your test by modifying the following file: https://github.com/coin-or/CppAD/blob/master/test_more/general/cppad_eigen.cpp

bradbell commented 2 years ago

I have merged pull request 155 (which corresponds to item 3 above). Would you please remove the corresponding parts of this pull request, so that it only implements item 1 above.

bradbell commented 2 years ago

The package config file installed by Eigen is egen3.pc so I do not understand why you are changing pkgconfig_info(eigen3 ${system_include} ${remove_coin_or}) to pkgconfig_info(Eigen3 ${system_include} ${remove_coin_or})

guestieng commented 2 years ago

The package config file installed by Eigen is egen3.pc so I do not understand why you are changing pkgconfig_info(eigen3 ${system_include} ${remove_coin_or}) to pkgconfig_info(Eigen3 ${system_include} ${remove_coin_or})

Yes, you are probably right here. In fact, now having "Eigen3" at this line is a little remainder of some very first tests for the compatibility in question. I just recovered it, i.e.: pkgconfig_info(eigen3 ...).

However, when using Eigen via pure Cmake (see find_package call), one certainly should use this "capital E" as the project target itself is labeled like this.

bradbell commented 2 years ago

I think we should also change SET(eigen_LIBRARIES "${Eigen3_LIBRARIES}") to SET(eigen_LIBRARIES "${eigen3_LIBRARIES}") Now that I think about it, this setting should probably be removed.
I do not know where USE_CMAKE_INTERFACE is set and what it means ?

guestieng commented 2 years ago

2. I do not know where `USE_CMAKE_INTERFACE` is set and what it means ?

It's simply a boolean cache variable, "off" by default. It switches between using "pkgconfig" / "cmake" for linking to external projects. This can probably be implemented a little bit more consistent, but I didn't dive that deep into the present cmake/pkgconfig machinery of Cppad.

bradbell commented 2 years ago

Are you trying to accomplish something that is not supported by the current CppAD cmake command ? https://coin-or.github.io/CppAD/doc/cmake.htm

If so, can you provide a workflow in https://github.com/coin-or/CppAD/tree/master/.github/workflows that fails with the current CppAD master but works with this pull request.

guestieng commented 2 years ago

Are you trying to accomplish something that is not supported by the current CppAD cmake command ? https://coin-or.github.io/CppAD/doc/cmake.htm

It seems this USE_CMAKE_INTERFACE switch can not be covered by the options available, indeed, can it?

If so, can you provide a workflow in https://github.com/coin-or/CppAD/tree/master/.github/workflows that fails with the current CppAD master but works with this pull request.

I am sorry, but at this point I have to step back a little for capacity reasons...

But as mentioned above the concrete failings without this pull request are the following (commit https://github.com/coin-or/CppAD/commit/90aca75c4d82251c9db98bdf53073374ac3dad8d):

If you do not or can not use pkgconfig, you can not use Eigen (forced to set include_eigen false)
If you do not or can not use pkgconfig, you can not use Cppad in a straightforward manner, i.e. Cmake targets seem not to be used (exported in particular) at all.

Besides, the transitive dependencies commit https://github.com/coin-or/CppAD/commit/6e4a74b9e2d7271650de45f9d9daa241e2792365 is certainly not fixing anything, but rather a feature.

bradbell commented 2 years ago

If you do not or can not use pkgconfig, you can not use Eigen (forced to set include_eigen false)

Yes, when an optional package has a pkgconfig file, CppAD expects to be able to use that file and instead of providing the install information for the package on the command line, it just uses a true/false flag on the command line and gets the necessary information from pkgconfig. Is there a system you have using that does not have access to pkgconfig ? If so, this is a problem for all the true/false optional package flags on the CppAD cmake command line.

guestieng commented 2 years ago

Yes, when an optional package has a pkgconfig file, CppAD expects to be able to use that file and instead of providing the install information for the package on the command line, it just uses a true/false flag on the command line and gets the necessary information from pkgconfig. Is there a system you have using that does not have access to pkgconfig ? If so, this is a problem for all the true/false optional package flags on the CppAD cmake command line.

Yes, indeed, this the case. In my case Eigen sufficient.

bradbell commented 2 years ago

What system are you using (that does not have pkgconfig) ?

guestieng commented 2 years ago

What system are you using (that does not have pkgconfig) ?

Windows.

bradbell commented 2 years ago

Are you using microsoft development tools, minsys, or cygwin ?

guestieng commented 2 years ago

If any, the first.

bradbell commented 2 years ago

I have been trying to get a msys system to build using github workflows (as a demonstration of how to do this). But have run into difficulities because the generated executables are not found; see the workflow branch https://github.com/coin-or/CppAD/tree/workflow

bradbell commented 2 years ago

@guestieng I have been able to link eigen on a msys2 machine. Perhaps I can walk you through the steps. Start by following the instructions on msys2 on https://coin-or.github.io/CppAD/doc/cmake.htm#CMake%20Command.msys2

and then just using

mkdir build
cd build
cmake ..
make check

Make sure this works for you before we go onto adding eigen.

guestieng commented 2 years ago

Make sure this works for you before we go onto adding eigen.

Thank you very much for your help and effort. However, this approach is not an option currently (the current approach is attempting a purely target-based pipline, insofar as even injections via CMAKE_PROJECT_INCLUDE and/or the like would be an acceptable alternative). I would understand though if you, in turn, decided to focus solely on the current philosophy of the Cppad (Cmake) interface.

bradbell commented 2 years ago

@guestieng do not understand. Do you have a google or other id that we could chat over. My google id is bradley.m.bell. If you send a chat id to that address (in next hour or so) I will accept it.

bradbell commented 1 year ago

I think I have solved the problem of building the tests, including the eigen tests, on windows using the Visual C++ compiler. See the following workflow file: https://github.com/coin-or/CppAD/blob/master/.github/workflows/conda-windows-eigen.yml

Here is an example workflow output: https://github.com/coin-or/CppAD/runs/7378019053?check_suite_focus=true

This required a change to the CppAD cmake API; see the heading 07-16 on https://coin-or.github.io/CppAD/doc/whats_new_22.htm

Does this work for you ?

bradbell commented 1 year ago

If you would like more support for installing without pkg-config, I suggest that we start with the use-case of eigen on windows. I think the best way to do this is, while include_eigen is true, and the following fails

    pkgconfig_info(eigen ${system_include})

to execute a different macro in cmake/findpackage_info.cmake that checks for eigen using https://cmake.org/cmake/help/latest/command/find_package.html

If both fail, then report the fatal error that the install failed.

coin-or / CppAD

Interface fixes and enhancements #154