[add_launch_test] Correct default python executable for windows debug

[ivanpauno]

Just what the title says.

@hidmic I think that your original request was doing this in each of the places where add_launch_test is used, but I really think this should be the default behavior. What do you think?

And maybe, we should apply a similar change to ament_add_pytest_test ...

[hidmic]

Hmm, silently ignoring a user provided argument doesn't sound like a good idea. But I see your point, in which case we might as well remove the argument and remove use cases throughout the codebase.

I do wonder why this isn't the case for ament_add_pytest_test(...), as there may be a justification we're not seeing behind that design choice. Maybe @dirk-thomas can shed some light.

[ivanpauno]

Hmm, silently ignoring a user provided argument doesn't sound like a good idea. But I see your point, in which case we might as well remove the argument and remove use cases throughout the codebase.

I do wonder why this isn't the case for ament_add_pytest_test(...), as there may be a justification we're not seeing behind that design choice. Maybe @dirk-thomas can shed some light.

I'm not ignoring the user argument ... it's inside an 'if(NOT _add_launch_test_PYTHON_EXECUTABLE)'.
I'm only changing the default behavior when that argument is not passed.

[hidmic]

Oops, I misread the patch :) Anyways, the question as to why this isn't implemented in ament_add_pytest_test(...) stands.

[ivanpauno]

Oops, I misread the patch :) Anyways, the question as to why this isn't implemented in ament_add_pytest_test(...) stands.

Just to provide more context about ament_add_pytest_test, here is an existing workaround for windows-debug builds:
https://github.com/ros2/system_tests/blob/8484bbaae8f620196d97bae6c043af039ec98ba0/test_cli/CMakeLists.txt#L7-L41
And this is how it handles the PYTHON_EXECUTABLE argument:
https://github.com/ament/ament_cmake/blob/54eb418e4c262d4b657cc8eaecc1f88a8907bc8e/ament_cmake_pytest/cmake/ament_add_pytest_test.cmake#L66-L68

[jacobperron]

I've fixed conflicts with master. I've also fixed another bug 5ec8e9b.

This should resolve ros2/build_farmer#200

[jacobperron]

CI with build type Debug:

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

LGTM

[jacobperron]

I do wonder why this isn't the case for ament_add_pytest_test(...), as there may be a justification we're not seeing behind that design choice.

I believe something similar should be done in ament_add_pytest_test() as well. I believe that is the cause of the similar test failures for rqt_* packages. E.g. https://ci.ros2.org/job/ci_windows/6980/testReport/junit/rqt_py_common.test.test_rqt_common_unit/TestMessageTreeModel/test_path_names/

I'm looking into it.