[checkfornan] io.h for windows instead of unistd.h

[proyan]

Remaining fix from https://github.com/coin-or/CppAD/issues/112

[CLAassistant]

All committers have signed the CLA.

[bradbell]

I am not sure if you need !defined(__clang__) here ?

It seems <io.h> is required for MS to find mkstemp; see https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/mktemp?view=msvc-160 and https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/mktemp-wmktemp?view=msvc-160

Should we include this in the test for mkstemp in the cppad_has_mkstemp check in the file https://github.com/coin-or/CppAD/blob/master/include/cppad/CMakeLists.txt ?

# -----------------------------------------------------------------------------
# cppad_has_mkstemp
#
SET(source "
# include <stdlib.h>
# include <unistd.h>
int main(void)
{
    char pattern[] = \"/tmp/fileXXXXXX\";
    int fd = mkstemp(pattern);
    return 0;
}
" )
compile_source_test("${source}" cppad_has_mkstemp )

[proyan]

I have updated the remaining instances of unistd.h by io.h in 5808656. I'm running make check in the CI to see if there are other errors that I may have missed.

[proyan]

It seems io.h is the replacement for unistd in windows systems https://stackoverflow.com/questions/341817/is-there-a-replacement-for-unistd-h-for-windows-visual-c. So normally this should not be compiler dependent. I don't think we need to add !defined(clang) here

[bradbell]

I am not sure if you need !defined(__clang__) here ?

It seems <io.h> is required for MS to find mkstemp; see https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/mktemp?view=msvc-160 and https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/mktemp-wmktemp?view=msvc-160

Should we include this in the test for mkstemp in the cppad_has_mkstemp check in the file https://github.com/coin-or/CppAD/blob/master/include/cppad/CMakeLists.txt ?

# -----------------------------------------------------------------------------
# cppad_has_mkstemp
#
SET(source "
# include <stdlib.h>
# include <unistd.h>
int main(void)
{
    char pattern[] = \"/tmp/fileXXXXXX\";
    int fd = mkstemp(pattern);
    return 0;
}
" )
compile_source_test("${source}" cppad_has_mkstemp )

[bradbell]

I closed this bug by mistake and have re-opened it ?

I did some testing on windows and could not get it to find mkstemp. Then I realized that the link above for io.h was referring to mktemp which is not the same as mkstemp. It seems windows does not have a verision of he posix routine mkstemp ? https://pubs.opengroup.org/onlinepubs/9699919799/functions/mkdtemp.html

[bradbell]

Does an mkstemp function exist when using your Windows version of the clang compiler ?

[proyan]

@bradbell I've added the ci with make check for windows and ubuntu with github actions. I think somehow github actions might be disabled on the main repo, I can see the CI run on my fork at https://github.com/proyan/CppAD/actions.

[proyan]

Does an mkstemp function exist when using your Windows version of the clang compiler ?

I see the following errors in the CI https://github.com/Simple-Robotics/pycppad/pull/18/checks?check_run_id=3459595658 :

lld-link : error : undefined symbol: omp_set_dynamic [D:\a\pycppad\pycppad\CppAD\build\example\multi_thread\openmp\example_multi_thread_openmp.vcxproj]
  >>> referenced by example_multi_thread_openmp.dir\Release\a11c_openmp.obj:(bool __cdecl a11c(void))
  >>> referenced by example_multi_thread_openmp.dir\Release\simple_ad_openmp.obj:(bool __cdecl simple_ad(void))
  >>> referenced by example_multi_thread_openmp.dir\Release\team_openmp.obj:(bool __cdecl team_create(unsigned __int64))
  
lld-link : error : undefined symbol: omp_set_num_threads [D:\a\pycppad\pycppad\CppAD\build\example\multi_thread\openmp\example_multi_thread_openmp.vcxproj]
  >>> referenced by example_multi_thread_openmp.dir\Release\a11c_openmp.obj:(bool __cdecl a11c(void))
  >>> referenced by example_multi_thread_openmp.dir\Release\simple_ad_openmp.obj:(bool __cdecl simple_ad(void))
  >>> referenced by example_multi_thread_openmp.dir\Release\simple_ad_openmp.obj:(bool __cdecl simple_ad(void))
  >>> referenced 2 more times
  
lld-link : error : undefined symbol: __kmpc_fork_call [D:\a\pycppad\pycppad\CppAD\build\example\multi_thread\openmp\example_multi_thread_openmp.vcxproj]
  >>> referenced by example_multi_thread_openmp.dir\Release\a11c_openmp.obj:(bool __cdecl a11c(void))
  >>> referenced by example_multi_thread_openmp.dir\Release\simple_ad_openmp.obj:(bool __cdecl simple_ad(void))
  >>> referenced by example_multi_thread_openmp.dir\Release\team_openmp.obj:(bool __cdecl team_work(void (__cdecl *)(void)))
  
lld-link : error : undefined symbol: __kmpc_for_static_init_4 [D:\a\pycppad\pycppad\CppAD\build\example\multi_thread\openmp\example_multi_thread_openmp.vcxproj]
  >>> referenced by example_multi_thread_openmp.dir\Release\a11c_openmp.obj:(.omp_outlined.)
  >>> referenced by example_multi_thread_openmp.dir\Release\simple_ad_openmp.obj:(.omp_outlined.)
  >>> referenced by example_multi_thread_openmp.dir\Release\team_openmp.obj:(.omp_outlined.)
  
lld-link : error : undefined symbol: __kmpc_for_static_fini [D:\a\pycppad\pycppad\CppAD\build\example\multi_thread\openmp\example_multi_thread_openmp.vcxproj]
  >>> referenced by example_multi_thread_openmp.dir\Release\a11c_openmp.obj:(.omp_outlined.)
  >>> referenced by example_multi_thread_openmp.dir\Release\simple_ad_openmp.obj:(.omp_outlined.)
  >>> referenced by example_multi_thread_openmp.dir\Release\team_openmp.obj:(.omp_outlined.)
  
lld-link : error : undefined symbol: omp_get_thread_num [D:\a\pycppad\pycppad\CppAD\build\example\multi_thread\openmp\example_multi_thread_openmp.vcxproj]
  >>> referenced by example_multi_thread_openmp.dir\Release\simple_ad_openmp.obj:(unsigned __int64 __cdecl `anonymous namespace'::thread_number(void))
  >>> referenced by example_multi_thread_openmp.dir\Release\team_openmp.obj:(bool __cdecl team_create(unsigned __int64))
  >>> referenced by example_multi_thread_openmp.dir\Release\team_openmp.obj:(unsigned __int64 __cdecl `anonymous namespace'::thread_num(void))
  >>> referenced 2 more times
  
lld-link : error : undefined symbol: omp_in_parallel [D:\a\pycppad\pycppad\CppAD\build\example\multi_thread\openmp\example_multi_thread_openmp.vcxproj]
  >>> referenced by example_multi_thread_openmp.dir\Release\simple_ad_openmp.obj:(bool __cdecl `anonymous namespace'::in_parallel(void))
  >>> referenced by example_multi_thread_openmp.dir\Release\team_openmp.obj:(bool __cdecl team_create(unsigned __int64))
  >>> referenced by example_multi_thread_openmp.dir\Release\team_openmp.obj:(bool __cdecl `anonymous namespace'::in_parallel(void))
  >>> referenced 2 more times
  Building Custom Rule D:/a/pycppad/pycppad/CppAD/introduction/CMakeLists.txt
  introduction.vcxproj -> D:\a\pycppad\pycppad\CppAD\build\introduction\Release\introduction.exe
  Begin test group introduction

however, it still says that all tests are passing

[jcarpent]

You just need to link against openmp. Depending on the OS/compiler, this might not be needed.

[bradbell]

What do you get when you do the following: cmake your_command_line_arguments | grep multi_thread

[proyan]

What do you get when you do the following: cmake your_command_line_arguments | grep multi_thread

On my Ubuntu system, I get

-- make check_example_multi_thread_openmp: available
-- make check_example_multi_thread_pthread: available
-- boost_multi_thread_ok = 1
-- make check_example_multi_thread_bthread: available
-- make check_example_multi_thread: available

I will try on the windows CI. I think the suggestion of justin to link the examples with openmp should be sufficient

[bradbell]

@proyan Try the following:

make check_example_multi_thread_openmp

[proyan]

Regarding openmp linking, it seems that with visual studio and clang combination, this is a known issue https://stackoverflow.com/questions/62122247/openmp-linking-errors-in-visual-studio-2019-llvm

[proyan]

@proyan Try the following:

make check_example_multi_thread_openmp

In windows-clang, I get the undefined symbol error from above. On ubuntu it passes

[bradbell]

@proyan It seems to me that there are three different things going on in this pull request:

1. Automated testing to determine if mkstemp is available on a windows system

2. Setting up a standard file structure and adding to the yml files for driving automated tests. see the existing *.yml files in the CppAD master https://github.com/coin-or/CppAD/blob/master/.travis.yml https://github.com/coin-or/CppAD/blob/master/appveyor.yml

3. Fixing the linkage to omp library on window

Perhaps it would be better if we spit it up ?

[proyan]

I'm okay with reducing the scope of this PR. I think eventually this PR does two things:

1. Add CI with github actions for windows, ubuntu and MacOS.
2. Fix issues with make check on windows. (which for now is only with clang compiler linking with openmp)

Regarding mkstemp, I didn't see the error related to mkstemp in the CI run https://github.com/proyan/CppAD/actions

[bradbell]

@proyan Try the following:

make check_example_multi_thread_openmp

In windows-clang, I get the undefined symbol error from above. On ubuntu it passes

I did not realize that make check does not run the available muti_thread checks. I will need to look into why this is the case.

[bradbell]

What is the purpose of the file .github/workflows/ci-windows-clang.yml . It seems to be installing CppAD for use by pycppad ?

[bradbell]

I fixed make check so it includes the multi_thread examples; see https://github.com/coin-or/CppAD/commit/3667ead45b6085d4fd75bbd18d15b633eb5b75bb

[bradbell]

Looking at https://github.com/coin-or/CppAD/pull/117/commits/bc40d0ff4869b76e9d0fc1aec947d21523c72d6b

It seems the omp library may be system specific and that perhaps it would be better to use OpenMP_CXX_LIB_NAMES; see https://cmake.org/cmake/help/latest/module/FindOpenMP.html ?

[proyan]

What is the purpose of the file .github/workflows/ci-windows-clang.yml . It seems to be installing CppAD for use by pycppad ?

The github workflows files are only adding a CI (as seen here: https://github.com/proyan/CppAD/actions) that performs make check on pushes and pull requests. So this is independent from any other repository. I'm not sure why these are not being shown in this PR as well, I think the github actions might be disabled in the github repo.

[proyan]

It seems the omp library may be system specific and that perhaps it would be better to use OpenMP_CXX_LIB_NAMES; see https://cmake.org/cmake/help/latest/module/FindOpenMP.html

You're right, it wasn't correct to add the library linkage like I did. However, I think the CXX_FLAGS are already adding the linking specifications for openmp, so the issue isn't whether openmp can be found, but how to link for the windows+clang case. Could you please have a look at ddc1802?

[bradbell]

It seems to me that the variable

OpenMP_<lang>_FLAGS
    OpenMP compiler flags for <lang>, separated by spaces.

with = CXX should have all the necessary flags for the system ?

[proyan]

I don't think OpenMP CXX Flags are being correctly fixed based on the compiler/environment types. From the latest commit , I still get the error in the following CI. So the fix didn't work: https://github.com/proyan/CppAD/runs/3474261730?check_suite_focus=true

2021-08-31T15:15:22.6299194Z lld-link : warning : ignoring unknown argument '-Xclang' [D:\a\CppAD\CppAD\build\test_more\cppad_for_tmb\test_more_cppad_for_tmb.vcxproj]
2021-08-31T15:15:22.6312512Z lld-link : warning : ignoring unknown argument '-fopenmp' [D:\a\CppAD\CppAD\build\test_more\cppad_for_tmb\test_more_cppad_for_tmb.vcxproj]
2021-08-31T15:15:22.6516716Z lld-link : error : undefined symbol: omp_set_dynamic [D:\a\CppAD\CppAD\build\test_more\cppad_for_tmb\test_more_cppad_for_tmb.vcxproj]
2021-08-31T15:15:22.6519205Z   >>> referenced by test_more_cppad_for_tmb.dir\Release\multi_atomic_two.obj:(bool __cdecl multi_atomic_two(void))
2021-08-31T15:15:22.6528655Z   >>> referenced by test_more_cppad_for_tmb.dir\Release\multi_atomic_three.obj:(bool __cdecl multi_atomic_three(void))
2021-08-31T15:15:22.6530062Z   >>> referenced by test_more_cppad_for_tmb.dir\Release\multi_chkpoint_one.obj:(bool __cdecl multi_chkpoint_one(void))
2021-08-31T15:15:22.6533783Z   >>> referenced 1 more times

I'm out of ideas for how to proceed here.

[bradbell]

I have started a branch for testing ideas related to github actions; see https://github.com/coin-or/CppAD/tree/actions

[bradbell]

Using github actions on macos-latest,(on the actions branch) I get the following message

/Users/runner/work/CppAD/CppAD/include/cppad/core/check_for_nan.hpp:211:21: warning: 'tmpnam' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of tmpnam(3), it is highly recommended that you use mkstemp(3) instead. [-Wdeprecated-declarations]
        file_name = tmpnam( nullptr );

This indicates that macos is not finding mkstemp.

[bradbell]

I have done a lot of testing and cannot figure out why cmake on the macos-latest platform is not finding mkstemp.

Here is a a very similar test script that works in another repository: https://github.com/bradbell/tst_github/blob/main/mkstemp/CMakeLists.txt Here is the resulting test output https://github.com/bradbell/tst_github/runs/3525307701?check_suite_focus=true You will note that this script finds mkstemp because cmake has the following output

-- cxx_has_mkstemp_1 = 1
-- cxx_has_mkstemp_2 = 1

On the other hand, for the cppad version of the test (which I expect to be the same), has the following cmake output

 -- cppad_has_mkstemp = 0

TheCppAD actions branch has a version of the test that comments out the make check (so it runs much faster): https://github.com/coin-or/CppAD/blob/actions/.github/workflows/ci_test.yml The corresponding cmake output can be seen here https://github.com/coin-or/CppAD/runs/3525397477?check_suite_focus=true where you will see

 -- cppad_has_mkstemp = 0

[bradbell]

@proyan Sorry for the delay here. I was very busy with some things for work and forgot about this pull request. If you ware willing to maintain the files in the workflows directory, I will merge them into the coin-or CppAD repository.

[proyan]

Hi Brad, Sure. Maintaining the github workflow files should not be too taxing. You could go ahead and merge the PR from my side