coin-or / CppAD

A C++ Algorithmic Differentiation Package: Home Page
https://cppad.readthedocs.io
Other
446 stars 94 forks source link

Fix compilation on Windows with V142 compiler #144

Closed jcarpent closed 2 years ago

jcarpent commented 2 years ago

This PR is a split of #143. It will then be followed by a second PR related to the CI using conda.

Thanks in advanced @bradbell for your review.

Best, Justin

bradbell commented 2 years ago

It appears to me that the changing {debug_which} -> {cppad_debug_which} in test_more/debug_rel/CMakeLists.txt is a bug fix. Do you agree ?

If you agree, I will make this change as a separate commit on the master branch and also on the stable/20220000 branch.

bradbell commented 2 years ago

I changed the master version oftest_more/debug_rel/CMakeLists.txt to the one in this pull request.

jcarpent commented 2 years ago

It appears to me that the changing {debug_which} -> {cppad_debug_which} in test_more/debug_rel/CMakeLists.txt is a bug fix. Do you agree ?

If you agree, I will make this change as a separate commit on the master branch and also on the stable/20220000 branch.

I do agree.

jcarpent commented 2 years ago

@bradbell Is the PR fine for you?

bradbell commented 2 years ago

@jcarpent I think it would be best to just drop the change to config.guess ? see the discussion below

In the top level CMakeLists.txt I changed the new code to indent 4 spaces instead of 2 (as all the rest of the CppAD code does).

I did a local merge to a local copy of the master on my machine and ran bin/check_all.sh mixed which does more extensive testing.

I got a failure in the test bin/check_automake.sh which did not like the fact that config.guess is different from the system version.

This file gets automatically replaced as a link by the command automake --add-missing and then converted to a real file by bin/autotools.sh automake

The autotools version of the install have been deprecated for about 10 years https://coin-or.github.io/CppAD/doc/autotools.htm I would like to drop it but coin-or is still using it (I think).

jcarpent commented 2 years ago

@jcarpent I think it would be best to just drop the change to config.guess ?

Done

bradbell commented 2 years ago

I have one other question: in speed/example/CMakeLists.txt you removed the line set_compile_flags( speed_example "${cppad_debug_which}" "${source_list}" ) I do not know why you removed this ?

The `bin/check_all.sh mixed`` script randomly selects if the odd or even indexed files are compiled with or without NDEBUG defined. This makes sure that the code works both ways. In addition, more error checking is done when NDEBUG is not defined.

jcarpent commented 2 years ago

It should then be deactivated on WIndows, as you cannot compile with two different compilations options (Release == 03 and Debug).

bradbell commented 2 years ago

I think what we really need is a check, in the top level CMakeLists.txt, that cppad_debug_which is not debug_odd or debug_even on windows systems (in which case an error message is generated and the configuration is aborted). See the section of CMakeLists.txt below # cppad_debug_which. This should fix the problem for all the sub-directories.

For an example of system dependent code in the top level CMakeLists.txt; see IF( ${CMAKE_SYSTEM_NAME} STREQUAL "Darwin" )

I can make the change if you like ?

jcarpent commented 2 years ago

What to do then if it is not the correct value?

jcarpent commented 2 years ago

I can make the change if you like ?

Please move forward if you are a clear picture of the change.

bradbell commented 2 years ago

What to do then if it is not the correct value?

When building on windows the cmake command will fail if cppad_debug_which is debug_odd or debug_even.

jcarpent commented 2 years ago

Looks fine. Thanks.