Make ctests platform independent #461

mmuetzel · 2024-06-22T19:19:43Z

Pull request purpose

Change configuration of some ctests on Windows so they work "out of the box".

Detailed changes proposed in this pull request

Windows doesn't have a mechanism similar to rpaths. Instead, shared libraries are searched in the current working directory and in the directory with the executable followed by directories in the environment variable PATH.

Add the path to the shared libarpack library to PATH for ctests that run executables that are located in a different directory and that set the current working directory not to the directory with the libarpack.dll. Use a generator expression that should be working independent on the CMake generator.

Also remove the previously added work-around from the CI rules for MinGW that is no longer needed with that change.

fghoussen · 2024-06-22T19:34:38Z

CMakeLists.txt

+      if (WIN32 AND BUILD_SHARED_LIBS)
+        set_tests_properties(arpackmm_tst issue401_tst issue215_tst
+          PROPERTIES
+          ENVIRONMENT_MODIFICATION "PATH=path_list_prepend:$<TARGET_FILE_DIR:arpack>")


Shouldn't be this done for all tests? Why only theses 3 ones?

Good point. I forgot to mention the crucial point in the PR description: .dlls are also loaded from the current working directory on Windows (a security nightmare!).

Afaict, these three tests are the only ones that set the working directory to somewhere other than the directory with the libarpack.dll.

I'll update the commit message and the description of the PR accordingly.

Edit: done.

these three tests are the only ones that set the working directory to somewhere other than

These 3 tests seem to be the only ones to be ran from a sh script with modified WORKING_DIRECTORY:

arpack-ng/CMakeLists.txt

Lines 706 to 713 in 77ba50e

configure_file(EXAMPLES/MATRIX_MARKET/arpackmm.sh ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/arpackmm.sh)

add_test(NAME arpackmm_tst WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${BASH_PROGRAM} -eu arpackmm.sh)

configure_file(EXAMPLES/MATRIX_MARKET/issue401.mtx ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue401.mtx)

configure_file(EXAMPLES/MATRIX_MARKET/issue401.sh ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue401.sh)

add_test(NAME issue401_tst WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${BASH_PROGRAM} -eu issue401.sh)

configure_file(EXAMPLES/MATRIX_MARKET/issue215.mtx ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue215.mtx)

configure_file(EXAMPLES/MATRIX_MARKET/issue215.sh ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue215.sh)

add_test(NAME issue215_tst WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${BASH_PROGRAM} -eu issue215.sh)

May be changing add_test or/and configure_file could be enough?

Does autotools have this problem (we try as much as possible to make the 2 cmake/autotools work the same way)?

3 tests seem to be the only ones to be ran from a sh script with modified WORKING_DIRECTORY

WORKING_DIRECTORY could maybe simply be suppressed from add_test for these 3 tests: did you try? This way there would be nothing specific to any tests

A possible alternative could be to put everything for these tests in the CMAKE_BINARY_DIR (i.e., the directory with the built libarpack). That is: at least the generated .mtx files and the arpackmm binary. (The generated shell script could probably stay in a dedicated directory.)
The shell script tries to run the arpackmm binary in the current working directory. And iiuc, the arpackmm binary is looking for the .mtx files in the current working directory, too.

But that seems more messy than the current setup imho. Currently, the files for the Eigen tests are nicely separated in their own directory. Please, let me know if you prefer to change that anyway.

I haven't checked running the tests with the autotools build system on Windows yet. I'll try that after we know how this issue should be resolved.

I tested with the following commands (using autotools):

./bootstrap mkdir build-autotools cd build-autotools ../configure --enable-eigen make all -j11 make all check

The three tests involving arpackmm passed without issues. I guess libtool uses some magic to make this work on Windows. (It probably has to deal with that all the time having the binaries in the .libs folders...)
Fwiw, all other tests also passed without issues.

So afaics, no changes are necessary in the autotools build system regarding this issue.

A possible alternative could be to put everything for these tests in the CMAKE_BINARY_DIR

Not sure what to do here. Both solution look similar to me. @sylvestre: what do you say?

fghoussen · 2024-06-29T07:51:39Z

CMakeLists.txt

@@ -711,6 +711,11 @@ function(build_tests)
      configure_file(EXAMPLES/MATRIX_MARKET/issue215.mtx ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue215.mtx)
      configure_file(EXAMPLES/MATRIX_MARKET/issue215.sh ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue215.sh)
      add_test(NAME issue215_tst WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${BASH_PROGRAM} -eu issue215.sh)
+      if (WIN32 AND BUILD_SHARED_LIBS)
+        set_tests_properties(arpackmm_tst issue401_tst issue215_tst


Could you add a cmake function(add_test_with_working_dir test_name) to gather add_test and set_tests_properties for the 3 tests that need a working dir? Would be clearer I guess

I added that function in the last commit.

Is that what you had in mind?

Is that what you had in mind?

Yes!

fghoussen · 2024-06-30T12:43:48Z

CMakeLists.txt

@@ -704,13 +716,19 @@ function(build_tests)
      configure_file(EXAMPLES/MATRIX_MARKET/B.mtx  ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/B.mtx)
      configure_file(EXAMPLES/MATRIX_MARKET/Bz.mtx ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/Bz.mtx)
      configure_file(EXAMPLES/MATRIX_MARKET/arpackmm.sh ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/arpackmm.sh)
-      add_test(NAME arpackmm_tst WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${BASH_PROGRAM} -eu arpackmm.sh)
+      add_test_with_working_dir(arpackmm_tst
+        WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}


Can you move the WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} and the COMMAND inside the add_test_with_working_dir function

That would make that function less versatile.
Let's say a future test would require the working directory to be set to somewhere in the source tree. Then, the new function wouldn't work.

I also don't know what you mean by moving the COMMAND to the new function. It is different for each of the three tests.

fghoussen · 2024-06-30T17:50:47Z

CMakeLists.txt

@@ -626,7 +626,19 @@ endif()
 ############################
 # TEST
 ############################
-function(build_tests)  
+
+function(add_test_with_working_dir test_name)


As is the function name does not do what it says

I though at something like:

function(add_test_with_working_dir test_name test_dir test_cli) add_test(NAME ${test_name} WORKING_DIRECTORY ${test_dir} COMMAND {test_cli}) endfunction()

There is a reason why CMake recommends to use the function syntax with options instead of fixed arguments. See, e.g., at the bottom of the following page:
https://cmake.org/cmake/help/latest/command/add_test.html

The command syntax above is recommended over the older, less flexible form

Imho, the changes you are proposing would be a pessimization to the currently proposed implementation (mainly because it would be less flexible).

Please, let me know if this is a show-stopper for you. Otherwise, I'd prefer to not do the change that you are proposing.

As far as the function name is concerned: The new function should be used if tests are added that manually change the working directory to somewhere that doesn't contain the libarpack library. So, the current name isn't misleading imho.
Please, let me know if you prefer a different name for that function.

Sorry to bother.
Any feedback on the last comment?

As is the function name does not do what it says

Can you pick a function name what do what it says? add_test_with_custom_rpath or similar and explain/comment what you do there "on windows, add path to lib in PATH as rpath is not supported"

fghoussen · 2024-09-14T10:36:27Z

CMakeLists.txt

@@ -626,7 +626,19 @@ endif()
 ############################
 # TEST
 ############################
-function(build_tests)  
+
+function(add_test_with_working_dir test_name)


As is the function name does not do what it says

Can you pick a function name what do what it says? add_test_with_custom_rpath or similar and explain/comment what you do there "on windows, add path to lib in PATH as rpath is not supported"

fghoussen · 2024-09-14T10:36:56Z

CMakeLists.txt

      configure_file(EXAMPLES/MATRIX_MARKET/issue401.mtx ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue401.mtx)
      configure_file(EXAMPLES/MATRIX_MARKET/issue401.sh ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue401.sh)
-      add_test(NAME issue401_tst WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${BASH_PROGRAM} -eu issue401.sh)
+      add_test_with_working_dir(issue401_tst


What is the change here? Stick to minimal change please

fghoussen · 2024-09-14T10:37:17Z

CMakeLists.txt

@@ -795,7 +813,7 @@ endfunction(build_tests)

 if(TESTS)
    enable_testing()
-    set(CMAKE_CTEST_COMMAND ctest -V)   
+    set(CMAKE_CTEST_COMMAND ctest -V)


OK to remove extra spaces

Windows doesn't have a mechanism similar to rpaths. Instead, shared libraries are searched in the current working directory and in the directory with the executable followed by directories in the environment variable PATH. Add the path to the shared libarpack library to PATH for ctests that run executables that are located in a different directory and that set the current working directory not to the directory with the libarpack.dll. Use a generator expression that should be working independent on the CMake generator.

Add a new CMake function that helps to add tests that change their working directory to somewhere else than the directory with the libarpack library. In that case, the environment variable PATH is set to include the directory with the library on Windows while running the tests. Use that new CMake function for the three ctests involving the Eigen library.

mmuetzel · 2024-09-22T16:15:30Z

I hope the latest change addresses your comments.

fghoussen · 2024-09-22T16:17:57Z

I hope the latest change addresses your comments.

Yes, thanks!

fghoussen · 2024-09-22T16:18:40Z

Can you update the changelog?

mmuetzel · 2024-09-22T16:43:40Z

Is that Changelog entry ok?

mmuetzel · 2024-09-23T16:58:02Z

@sylvestre: Thank you for merging.

sylvestre · 2024-09-23T17:00:37Z

thank you for you contrib :)

fghoussen reviewed Jun 22, 2024

View reviewed changes

mmuetzel force-pushed the cmake branch from aaedccd to 64aedfb Compare June 24, 2024 07:26

fghoussen reviewed Jun 29, 2024

View reviewed changes

mmuetzel force-pushed the cmake branch from 64aedfb to 596c110 Compare June 30, 2024 10:25

fghoussen reviewed Jun 30, 2024

View reviewed changes

fghoussen reviewed Sep 14, 2024

View reviewed changes

mmuetzel added 3 commits September 22, 2024 17:59

CI: Remove work-around from rules for MinGW that is no longer needed

7671ceb

mmuetzel force-pushed the cmake branch from 596c110 to 3207015 Compare September 22, 2024 16:09

Update CHANGELOG.

2c1203e

sylvestre merged commit 9431fd5 into opencollab:master Sep 23, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ctests platform independent #461

Make ctests platform independent #461

mmuetzel commented Jun 22, 2024 •

edited

Loading

fghoussen Jun 22, 2024

mmuetzel Jun 24, 2024 •

edited

Loading

fghoussen Jun 24, 2024

fghoussen Jun 24, 2024

mmuetzel Jun 25, 2024 •

edited

Loading

mmuetzel Jun 25, 2024

fghoussen Jun 25, 2024

fghoussen Jun 29, 2024

mmuetzel Jun 30, 2024

fghoussen Jun 30, 2024

fghoussen Jun 30, 2024

mmuetzel Jun 30, 2024 •

edited

Loading

fghoussen Jun 30, 2024 •

edited

Loading

mmuetzel Jul 1, 2024

mmuetzel Jul 9, 2024

fghoussen Sep 14, 2024

fghoussen Sep 14, 2024

fghoussen Sep 14, 2024

fghoussen Sep 14, 2024

mmuetzel commented Sep 22, 2024

fghoussen commented Sep 22, 2024

fghoussen commented Sep 22, 2024

mmuetzel commented Sep 22, 2024

mmuetzel commented Sep 23, 2024

sylvestre commented Sep 23, 2024

	configure_file(EXAMPLES/MATRIX_MARKET/arpackmm.sh ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/arpackmm.sh)
	add_test(NAME arpackmm_tst WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${BASH_PROGRAM} -eu arpackmm.sh)
	configure_file(EXAMPLES/MATRIX_MARKET/issue401.mtx ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue401.mtx)
	configure_file(EXAMPLES/MATRIX_MARKET/issue401.sh ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue401.sh)
	add_test(NAME issue401_tst WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${BASH_PROGRAM} -eu issue401.sh)
	configure_file(EXAMPLES/MATRIX_MARKET/issue215.mtx ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue215.mtx)
	configure_file(EXAMPLES/MATRIX_MARKET/issue215.sh ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/issue215.sh)
	add_test(NAME issue215_tst WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${BASH_PROGRAM} -eu issue215.sh)

Make ctests platform independent #461

Make ctests platform independent #461

Conversation

mmuetzel commented Jun 22, 2024 • edited Loading

Pull request purpose

Detailed changes proposed in this pull request

Choose a reason for hiding this comment

mmuetzel Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmuetzel Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmuetzel Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

fghoussen Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmuetzel commented Sep 22, 2024

fghoussen commented Sep 22, 2024

fghoussen commented Sep 22, 2024

mmuetzel commented Sep 22, 2024

mmuetzel commented Sep 23, 2024

sylvestre commented Sep 23, 2024

mmuetzel commented Jun 22, 2024 •

edited

Loading

mmuetzel Jun 24, 2024 •

edited

Loading

mmuetzel Jun 25, 2024 •

edited

Loading

mmuetzel Jun 30, 2024 •

edited

Loading

fghoussen Jun 30, 2024 •

edited

Loading