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

Interface fixes and enhancements #154

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

guestieng
Copy link

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)

Konrad Schatz added 3 commits June 30, 2022 13:43
…rror C2039: 'ReturnType': is not a member of 'Eigen::ScalarBinaryOpTraits...'
- now simplified usage of cppad via find_package and target_link_libraries in depending projects is possible
- fixed: problems if pkgconfig can not be used
- fixed: linking error (not finding definition of local::temp_file)
- using more generic header install directory
@CLAassistant
Copy link

CLAassistant commented Jun 30, 2022

CLA assistant check
All committers have signed the CLA.

@bradbell
Copy link
Contributor

bradbell commented Jul 1, 2022

It seems that three things are going on here:

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

@guestieng
Copy link
Author

guestieng commented Jul 1, 2022

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

Then, as to the questions:

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

Furthermore:

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

@guestieng guestieng closed this Jul 1, 2022
@guestieng guestieng reopened this Jul 1, 2022
@bradbell
Copy link
Contributor

bradbell commented Jul 2, 2022

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
Copy link
Contributor

bradbell commented Jul 5, 2022

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.

# Conflicts (fixed):
#	test_more/general/cppad_eigen.cpp
@bradbell
Copy link
Contributor

bradbell commented Jul 6, 2022

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
Copy link
Author

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
Copy link
Contributor

bradbell commented Jul 6, 2022

  1. 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.

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

@guestieng
Copy link
Author

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
Copy link
Contributor

bradbell commented Jul 7, 2022

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
Copy link
Author

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 90aca75):

  • 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 6e4a74b is certainly not fixing anything, but rather a feature.

@bradbell
Copy link
Contributor

bradbell commented Jul 7, 2022

  • 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
Copy link
Author

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
Copy link
Contributor

bradbell commented Jul 7, 2022

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

@guestieng
Copy link
Author

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

Windows.

@bradbell
Copy link
Contributor

bradbell commented Jul 7, 2022

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

@guestieng
Copy link
Author

If any, the first.

@bradbell
Copy link
Contributor

bradbell commented Jul 8, 2022

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
Copy link
Contributor

bradbell commented Jul 8, 2022

@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
Copy link
Author

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
Copy link
Contributor

bradbell commented Jul 8, 2022

@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
Copy link
Contributor

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
Copy link
Contributor

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.

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.

3 participants