Modify ctest so we can package the testfiles and install on the target.#435
Modify ctest so we can package the testfiles and install on the target.#435balister wants to merge 1 commit intognuradio:mainfrom
Conversation
This lets us run the volk tests on the embedded target easily. Major changes are not including full paths to files, since these were based on the build system paths and not setting paths to find libraries in the build directory. Signed-off-by: Philip Balister <[email protected]>
jdemel
left a comment
jdemel
left a comment
There was a problem hiding this comment.
I have some comments regarding this PR.
Mostly, can you add info on how does this change make life easier for cross compiled libraries. How does one use it in that case? An example added to the documentation would be great. e.g. in our README file in the cross-compile section.
Unfortunately, our TravisCI is still down. Thus, we can't see the impact of these particular changes in our cross compile tests with QEMU.
| if (NOT CMAKE_CROSSCOMPILING) | ||
| 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} | ||
| ) | ||
| endif(CMAKE_CROSSCOMPILING) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
How is one supposed to use it for cross-compiling?
There was a problem hiding this comment.
Now that's some documentation that I'd read.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
michaelld
left a comment
michaelld
left a comment
There was a problem hiding this comment.
I'm not sure your solution here is the right solution ... it's really not clear to me how it helps allow testing function when cross-building. I'd need to do some cross-building & testing ... which is unlikely any time soon given my current workload. So ... it's on you to help me/us understand how your tweaks here -do- help.
| ) | ||
| else() | ||
| add_test(NAME qa_${test_name} | ||
| COMMAND ${SHELL} ${test_name}_test.sh ${TARGET_DIR_LIST} |
There was a problem hiding this comment.
Shouldn't this be ${CMAKE_CURRENT_BINARY_DIR}/${test_name}_test.sh ... just to be explicit?
| COMMAND ${SHELL} ${sh_file} ${TARGET_DIR_LIST} | ||
| ) | ||
|
|
||
| if (NOT CMAKE_CROSSCOMPILING) |
There was a problem hiding this comment.
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.
|
@balister I'd like to see these improvements in a release. Could you address the comments and maybe rebase this PR? A rebase should trigger more CI as well. |
4 participants
This lets us run the volk tests on the embedded target easily. Major changes
are not including full paths to files, since these were based on the build
system paths and not setting paths to find libraries in the build directory.
Signed-off-by: Philip Balister [email protected]