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

Respect path order of DL_PATHS in catch_discover_tests #2878

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

cy20lin
Copy link
Contributor

@cy20lin cy20lin commented Jun 22, 2024

Description

In catch_discover_tests function, the DL_PATHS opition is used to specify the directories of the dependent shared libraries. The specified paths are used in the function to setup the environment for (1) retrieving the list of test cases from the text executable and for (2) executing the tests.

With current implementation, given DL_PATHS is specified, the prepared execution environments are inconsistent between (1) and (2), and the order of the paths specified by the DL_PATHS for searching libraries is not respected as well.

This PR tweak the catch_discover_tests_impl function, so that the execution environments are made consistent and the order of the search paths specified by DL_PATHS are respected.

Detail

Current behavior

The code for modifing the execution environment for (1) is shown as below, where the original value of environment variable is discarded and its value is overwrited by the specified DL_PATHS.

if(dl_paths)
    cmake_path(CONVERT "${dl_paths}" TO_NATIVE_PATH_LIST paths)
    set(ENV{${dl_paths_variable_name}} "${paths}")
endif()

The code for modifing the execution environment for (2) is shown as follows, the paths in the DL_PATHS are transformed to the perform the path_list_prepend command for each specified path in order, which resulting in the order of searching DL_PATHS being reversed, and follows the searching order of the original values from the environment.

  if(dl_paths)
    foreach(path ${dl_paths})
      cmake_path(NATIVE_PATH path native_path)
      list(APPEND environment_modifications "${dl_paths_variable_name}=path_list_prepend:${native_path}")
    endforeach()
  endif()

For example, when running on Windows, the environment variable for searching dependent shared libraries is PATH (i.e. dl_paths_variable_name is setted to PATH).

Given

  • the orignal environment PATH=/path/to/dir/C;/path/to/dir/D, and
  • the specified option DL_PATHS=/path/to/dir/A;/path/to/dir/B

We could see that under the current implementation,

  • the modified environment for (1) is: PATH=/path/to/dir/A;/path/to/dir/B
  • the modified environment for (2) is: PATH=/path/to/dir/B;/path/to/dir/A;/path/to/dir/C;/path/to/dir/D

Which are inconsistent, and the specified order is not respected.

Proposed behavior

The code for (1) is modified such that the DL_PATHS is prepend before the original paths, instead of overwriting the original values.

  if(dl_paths)
    cmake_path(CONVERT "$ENV{${dl_paths_variable_name}}" TO_NATIVE_PATH_LIST env_dl_paths)
    list(PREPEND env_dl_paths "${dl_paths}")
    cmake_path(CONVERT "${env_dl_paths}" TO_NATIVE_PATH_LIST paths)
    set(ENV{${dl_paths_variable_name}} "${paths}")
  endif()

The code for (2) has been modified so that the commands in environment_modifications follow a "first path, last prepend" order. This ensures that the finding procedure would first search the paths in DL_PATHS with the specified order, followed by the paths from the original environment.

  if(dl_paths)
    foreach(path ${dl_paths})
      cmake_path(NATIVE_PATH path native_path)
      list(PREPEND environment_modifications "${dl_paths_variable_name}=path_list_prepend:${native_path}")
    endforeach()
  endif()

For example, given that

  • the orignal environment PATH=/path/to/dir/C;/path/to/dir/D, and
  • the specified option DL_PATHS=/path/to/dir/A;/path/to/dir/B

With the proposed implementation, the modified environment for (1) and (2) becomes: PATH=/path/to/dir/A;/path/to/dir/B;/path/to/dir/C;/path/to/dir/D

The paths order are respected and the execution environments are made consistent.

@cy20lin cy20lin changed the title Respect path order of DL_PATHS in catch_discover_tests function Respect path order of DL_PATHS in catch_discover_tests Jun 22, 2024
Copy link

codecov bot commented Jun 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.11%. Comparing base (4e8d92b) to head (c642c4b).
Report is 16 commits behind head on devel.

Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #2878      +/-   ##
==========================================
+ Coverage   91.10%   91.11%   +0.01%     
==========================================
  Files         198      198              
  Lines        8493     8493              
==========================================
+ Hits         7737     7738       +1     
+ Misses        756      755       -1     

@horenmar horenmar added the Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration. label Aug 13, 2024
@horenmar horenmar force-pushed the feature/respect-dlpaths-order branch from 288a50d to 18b84a4 Compare August 13, 2024 20:09
@horenmar horenmar merged commit 1538be6 into catchorg:devel Aug 13, 2024
74 of 75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants