Fix for incorrect delimiters in relative_input_file on Windows

[MichaelOrlov]

• Addressing CI failure described in [Foxy] rosbag2_storage_mcap: merge into rosbag2 repo (backport #1163) rosbag2#1198 (comment)
• Fix for ""// with input from rosbag2_storage_mcap_testdata/msg\ComplexMsgDependsOnIdl.msg"
• On Windows platform message generator messing up with the input file name ""/msg\ComplexMsgDependsOnIdl.msg" by inserting double backslashes instead of one forward slash.
• Partial backport from Expose .msg/.srv/.action to .idl conversion via rosidl translate CLI #576

[emersonknapp]

This looks good to me... but I wonder if it should be considered a breaking change for any existing code depending on the contents of these IDL files within the release

[MichaelOrlov]

@emersonknapp This is a good question. I've tried to search in regards changed relative_input_file parameter across repos and files where it is used.
And apparently, it turns out that this parameter only participates in forming a comment message in the generated type description

rosidl/rosidl_adapter/rosidl_adapter/resource/msg.idl.em

Line 2 in b62e8c4

// with input from @(pkg_name)/@(relative_input_file)

i.e.

// with input from @(pkg_name)/@(relative_input_file)

It will not break any API or ABI compatibility.
I am very doubt, that anything else depends on the format of the comment // with input from @(pkg_name)/@(relative_input_file) in the type description.

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/c5c8c547760e183e8f3fb587ac70472f/raw/afd26accee89750545fe9e56bc8256e161a4412b/ros2.repos
BUILD args: --packages-above-and-dependencies rosidl_adapter rosbag2_storage_mcap
TEST args: --packages-above rosidl_adapter rosbag2_storage_mcap
ROS Distro: foxy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12114

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

Re-start CI with focal target
Gist: https://gist.githubusercontent.com/MichaelOrlov/c5c8c547760e183e8f3fb587ac70472f/raw/afd26accee89750545fe9e56bc8256e161a4412b/ros2.repos
BUILD args: --packages-above-and-dependencies rosidl_adapter rosbag2_storage_mcap
TEST args: --packages-above rosidl_adapter rosbag2_storage_mcap
ROS Distro: foxy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12115

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

Windows build failed due to the issue with the Connext_DDS which is exists in baseline and failing every other Windows job with error messages:

Copyright (C) Microsoft Corporation. All rights reserved.



  Checking Build System

  Building Custom Rule C:/ci/ws/src/ros2/rosidl_typesupport_connext/rosidl_typesupport_connext_cpp/CMakeLists.txt

  identifier.cpp

  wstring_conversion.cpp

     Creating library C:/ci/ws/build/rosidl_typesupport_connext_cpp/Release/rosidl_typesupport_connext_cpp.lib and object C:/ci/ws/build/rosidl_typesupport_connext_cpp/Release/rosidl_typesupport_connext_cpp.exp

CVTRES : fatal error CVT1107: 'C:\Program Files\rti_connext_dds-5.3.1\lib\x64Win64VS2017\nddscpp.lib' is corrupt [C:\ci\ws\build\rosidl_typesupport_connext_cpp\rosidl_typesupport_connext_cpp.vcxproj]

LINK : fatal error LNK1123: failure during conversion to COFF: file invalid or corrupt [C:\ci\ws\build\rosidl_typesupport_connext_cpp\rosidl_typesupport_connext_cpp.vcxproj]

---
--- stderr: rosidl_typesupport_connext_cpp
CMake Warning (dev) at C:/ci/ws/install/share/connext_cmake_module/cmake/Modules/FindConnext.cmake:163 (list):
  Policy CMP0121 is not set: The list() command now validates parsing of
  index arguments.  Run "cmake --help-policy CMP0121" for policy details.
  Use the cmake_policy command to set the policy and suppress this warning.
  Invalid list index "nddsc.lib".
Call Stack (most recent call first):
  C:/ci/ws/install/share/connext_cmake_module/cmake/Modules/FindConnext.cmake:198 (_count_found_libraries)
  CMakeLists.txt:25 (find_package)

Re-running Windows job one more time without CI_USE_CONNEXTDDS flag.

• Windows

[MichaelOrlov]

Windows CI job fails in rqt_py_common with error messages

Traceback (most recent call last):
  File "C:\ci\ws\install\Lib\site-packages\ament_index_python\packages.py", line 50, in get_package_prefix
    content, package_prefix = get_resource('packages', package_name)
  File "C:\ci\ws\install\Lib\site-packages\ament_index_python\resources.py", line 48, in get_resource
    raise LookupError(
LookupError: Could not find the resource 'launch' of type 'packages'

I've made some investigation and was able to reproduce this failure on my local machine with and without fixes from this PR. It means that this failure exists on the baseline even without my changes.
One interesting moment is that those tests from rqt_py_common fail only if sourcing local_setup.bat.
If make a clean build or reopen the console and run

colcon test --packages-select rqt_py_common --event-handlers console_direct+

The tests will pass without any errors with and without changes from this PR.

[MichaelOrlov]

@emersonknapp @clalancette Kindly ping for review and merge.
I don't have permission to merge into the rosidl repo.

[emersonknapp]

I don't have that access either - and I would also be hesitant to merge something so low in the stack until CI is all sorted out. I'm running this Windows Foxy build with no changes as a test just to make sure of the baseline

[quarkytale]

Hey @MichaelOrlov @emersonknapp I can help with this, I'm in for getting things merged but I agree with Emerson, let's have CI sorted out atleast before merging this one.

[emersonknapp]

I don't think we should move forward with this change. There will only be one last release, and we don't want to change anything about code generation without having any future chance to fix unexpected consequences. Since this change was intended to fix a test case in a downstream package, I think we should just patch that test to go green for the current behavior, which while maybe odd should be considered correct and final for Foxy

[quarkytale]

CI without mcap

[MichaelOrlov]

@quarkytale CI still magically fails in rqt_py_common We can't merge this PR if it causes regression.

• I've prepared a workaround for the originally failing test from rosbag2_storage_mcap in [foxy] Workaround for failing can_resolve_msg_with_idl_deps test on Windows rosbag2#1355 will see how it will pass CI.

[MichaelOrlov]

Ok folks.

• Windows CI job with [foxy] Workaround for failing can_resolve_msg_with_idl_deps test on Windows rosbag2#1355 failed the same rqt_py_common test with exactly the same error messages:

The test did not generate a result file:

Traceback (most recent call last):
  File "C:\ci\ws\install\Lib\site-packages\ament_index_python\packages.py", line 50, in get_package_prefix
    content, package_prefix = get_resource('packages', package_name)
  File "C:\ci\ws\install\Lib\site-packages\ament_index_python\resources.py", line 48, in get_resource
    raise LookupError(
LookupError: Could not find the resource 'launch' of type 'packages'

During handling of the above exception, another exception occurred:

• It means that failure in the rqt_py_common tests exists on the baseline and that there is no regression in the current Fix for incorrect delimiters in relative_input_file on Windows #745 PR. It just turns out that failure in the rqt_py_common reproduces when rosbag2_storage_mcap also participates in build and test.
  This explains why the test was passing when @emersonknapp was running CI for pure master branch with --packages-above rosidl_adapter only without rosbag2_storage_mcap.

• Given the fact that with [foxy] Workaround for failing can_resolve_msg_with_idl_deps test on Windows rosbag2#1355 test failure in the rqt_py_common tests will still persist when compiling and running with rosbag2_storage_mcap I would strongly recommend to proceed in either of two options.

1. Proceed and merge this Fix for incorrect delimiters in relative_input_file on Windows #745 as an adequate solution for the failing test in rosbag2_storage_mcap test. Please note that it was proved that there is no regression in Fix for incorrect delimiters in relative_input_file on Windows #745
2. Go ahead with [Foxy] revert #1198 "rosbag2_storage_mcap: merge into rosbag2" rosbag2#1347 by reverting mcap bring up entirely and dismiss Fix for incorrect delimiters in relative_input_file on Windows #745 and [foxy] Workaround for failing can_resolve_msg_with_idl_deps test on Windows rosbag2#1355

[MichaelOrlov]

@emersonknapp @quarkytale @clalancette Please see my comment ^^^^ (above).

[emersonknapp]

I don't understand why not option 3) Just merge ros2/rosbag2#1355

[MichaelOrlov]

I don't understand why not option 3) Just merge ros2/rosbag2#1355

Because failure in rqt_py_common tests will still persist after merging ros2/rosbag2#1355. And this is not really a fix for the root cause of the issue, but rather an accommodation of the test to the wrong behavior.

[emersonknapp]

I still don't get it - this change #745 doesn't fix rqt_py_common either, does it? Are you invested in fixing that rqt_py_common test for some reason? As far as I can tell, it's totally unrelated to the rosbag2 stuff

[MichaelOrlov]

@emersonknapp Yes. failure in the rqt_py_common is totally unrelated to the rosbag2 stuff.

The reason why I prefer #745 versus ros2/rosbag2#1355 is that #745 is really fixing the root cause of the failure in the rosbag2_storage_mcap tests and ros2/rosbag2#1355 only making adoptaions in the test for the buggy behaviour.

[emersonknapp]

I'm trying to argue that Foxy's codegen should be considered finalized, and since it doesn't affect behavior we can have a little kludge workaround downstream so that the test is not "overfit" to exact text output but instead just tests what matters, which is that things work correctly.

[MichaelOrlov]

As regards to the

Are you invested in fixing that rqt_py_common test for some reason?

I've spent some time trying to figure out why it fails and found that it fails only if sourcing /install/local_setup.bash
I don't know the details of why.

[MichaelOrlov]

Why consider Foxy's codegen as finalized if we know that there is obviously a bug in it and we have an adequate fix for it?

[quarkytale]

... we have an adequate fix for it?

If that fix actually passes CI and makes the final snapshot stable then that's all I care about.

[MichaelOrlov]

... we have an adequate fix for it?

If that fix actually passes CI and makes the final snapshot stable then that's all I care about.

Though, it can't magically fix what was already broken before in the rqt_py_common.
The #745 intends to fix failures in rosbag2_storage_mcap tests and actually fixes them.

[MichaelOrlov]

• Closing as will not be merged in favor of the [Foxy] revert #1198 "rosbag2_storage_mcap: merge into rosbag2" rosbag2#1347
• We decided to revert mcap bring up completely to avoid any risks and concerns.