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

KDB_EXEC_PATH should not be set in installed test scripts #3246

Open
kodebach opened this issue Nov 18, 2019 · 7 comments
Assignees
Labels
bug

Comments

@kodebach
Copy link
Contributor

@kodebach kodebach commented Nov 18, 2019

No, KDB_EXEC_PATH is not set for an installed kdb (unless the user sets it)

I just took the master version of Elektra and installed it. The file /usr/local/lib/elektra/tool_exec/check_meta contains the line:

export KDB_EXEC_PATH="/home/klemens/libelektra/build/bin:$KDB_EXEC_PATH"

Of course this doesn't affect testmod_* tests, but it is wrong nonetheless. I will create a new issue.

Originally posted by @kodebach in #2856 (comment)


Setting KDB_EXEC_PATH in installed test scripts not necessarily wrong, but it should not contain the build directory. It wouldn't matter, if the build directory no longer exists. But tests could break, if the build directory contains different versions of the test scripts.

We should use a solution similar to the one that is used for the $KDB variable. It is only set to ${CMAKE_BINARY_DIR}/bin/kdb in the testscr_* variants of the tests, which are not installed.

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 18, 2019

Thank you for reporting this problem! I think its unrelated to check_meta in particular but it is in all tests (as it is done in tests/shell/include_common.sh.in).

I agree with @kodebach that we need to avoid this line completely for the installed scripts using a similar logic as we have for KDB.

@petermax2 do you maybe have time to fix this?

@kodebach

This comment has been minimized.

Copy link
Contributor Author

@kodebach kodebach commented Nov 18, 2019

I think its unrelated to check_meta in particular but it is in all tests (as it is done in tests/shell/include_common.sh.in).

check_meta was just an example.

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 18, 2019

Maybe the line can be removed from there altogether and we simply set KDB_EXEC_PATH from within the environment of cmake/ctest as it is already done in tests/shell/shell_recorder/tutorial_wrapper/CMakeLists.txt

@kodebach

This comment has been minimized.

Copy link
Contributor Author

@kodebach kodebach commented Nov 18, 2019

AFAIK CMake's add_test doesn't allow setting environment variables, we would have to wrap the test in a script or in an env call or something like that.

@petermax2

This comment has been minimized.

Copy link
Member

@petermax2 petermax2 commented Nov 18, 2019

@kodebach is right! Setting the environment via add_msr_test does not work.

@petermax2

This comment has been minimized.

Copy link
Member

@petermax2 petermax2 commented Nov 18, 2019

@petermax2 do you maybe have time to fix this?

Not so much. I will try to find a fix but I don't know when I'll be able to investigate further.

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 20, 2019

I will try to find a fix

@petermax2 thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.