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.

Sign up for GitHub

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

Modify ctest so we can package the testfiles and install on the target. #435

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Modify ctest so we can package the testfiles and install on the target. #435

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions cmake/Modules/VolkAddTest.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,17 +139,18 @@ function(VOLK_ADD_TEST test_name executable_name)
#generate a shell script file that sets the environment and runs the test
set(sh_file ${CMAKE_CURRENT_BINARY_DIR}/${test_name}_test.sh)
file(WRITE ${sh_file} "#!${SHELL}\n")
if(SHELL_SUPPORTS_IFS)
file(APPEND ${sh_file} "export IFS=:\n")
else()
file(APPEND ${sh_file} "LL=\"$1\" && for tf in \"\$@\"; do LL=\"\${LL}:\${tf}\"; done\n")
endif()

#each line sets an environment variable
foreach(environ ${environs})
file(APPEND ${sh_file} "export ${environ}\n")
endforeach(environ)

if (NOT CMAKE_CROSSCOMPILING)
if(SHELL_SUPPORTS_IFS)
file(APPEND ${sh_file} "export IFS=:\n")
else()
file(APPEND ${sh_file} "LL=\"$1\" && for tf in \"\$@\"; do LL=\"\${LL}:\${tf}\"; done\n")
endif()

#each line sets an environment variable
foreach(environ ${environs})
file(APPEND ${sh_file} "export ${environ}\n")
endforeach(environ)
endif(CMAKE_CROSSCOMPILING)
Comment on lines +142 to +153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is one supposed to use it for cross-compiling?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that's some documentation that I'd read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I am doing is creating a package that has all the tests in it. So tests run on the target are not running in the build tree. Are we actually using the bit that runs qemu?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 2 CI tests that use QEMU. I'd argue that we can consider these tests our cross compile tests.

I'd really love to make cross compiling as simple as possible. Given the fact that I don't have experience with that yet I'd like to understand how to use your changes though. Otherwise I fear things will break again soon.

set(VOLK_TEST_ARGS "${test_name}")

#redo the test args to have a space between each
Expand All @@ -164,10 +165,15 @@ function(VOLK_ADD_TEST test_name executable_name)
#add the shell file as the test to execute;
#use the form that allows for $<FOO:BAR> substitutions,
#then combine the script arguments inside the script.
add_test(NAME qa_${test_name}
COMMAND ${SHELL} ${sh_file} ${TARGET_DIR_LIST}
)

if (NOT CMAKE_CROSSCOMPILING)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of lines 154-168 can be incorporated into both of these changes as one big change: either we're doing cross compiling or not; if so, then we use this indirect shell script to set various environment variables & such then internally call the test script; if not, then we just call the test directly. From a code-flow perspective, I prefer a full divide and conquer approach rather than this piecemeal.

add_test(NAME qa_${test_name}
COMMAND ${SHELL} ${sh_file} ${TARGET_DIR_LIST}
)
else()
add_test(NAME qa_${test_name}
COMMAND ${SHELL} ${test_name}_test.sh ${TARGET_DIR_LIST}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ${CMAKE_CURRENT_BINARY_DIR}/${test_name}_test.sh ... just to be explicit?

)
endif(CMAKE_CROSSCOMPILING)
Comment on lines +168 to +176
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the difference here is {sh_file} vs {test_name}_test.sh. Wouldn't it be sensible to write the correct filename into the variable {sh_file} directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sh_file contains the path to the shell file also. This path is based on the location of the build system, which is not where tests are installed on the target.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, wouldn't it be better to write the value you need to ${sh_file}? This would make this particular section simpler. The other section l142ff would benefit as well because comments on why things are done the way they are done could go there. I'd expect your short explanation why this is necessary there.

endif(UNIX)

if(WIN32)
Expand Down