Add testcase and fix for include order of generated code with stdint.h

[mathias-luedtke]

Description

This PR add a test case that shows that cpplint checks fail for generated code for services that have buffers in their response, but not in the request.
The same issue should happen for actions.

The tests will fail with:

- rosidl_generator_py.cpplint_rosidl_generated_py build/include_order [4] (/ws/build/rosidl_generator_py/rosidl_generator_py/rosidl_generator_py/srv/_get_buffer_s.c:83)
  <<< failure message
    Found C system header after other header. Should be: _get_buffer_s.h, c system, c++ system, other.
  >>>

Is this user-facing behavior change?

No

Did you use Generative AI?

No

Additional Information

This issue was introduced by #250

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[skyegalaxy]

@adbidwai to take a look

[adbidwai]

Hi, I can take a look at this. Thanks!

[fujitatomoya]

I do not think we can take this without fixes, because this generates intentionally red test.

[mathias-luedtke]

I do not think we can take this without fixes, because this generates intentionally red test.

I can add the fix as well, but I need to know which option you do prefer (see #264)

[mathias-luedtke]

@fujitatomoya: Which fix would you prefer?

• silence build/include_order in the cpplint checks
• always include <stdint.h>
• more complicated fix: add stdint.h in the request/goal part, if the parts use buffers

[fujitatomoya]

I'd say that the best option we got here is to hoist the include to the top-of-file system-header block, conditionally at the interface level with checking that interface requires the header. but i guess that perfectly acceptable fallback: always include <stdint.h> in the top block if that makes the complicatoin. I was thinking about the compile-time concern but stdint.h is one of the smallest headers in the standard library. what do you think?

[mathias-luedtke]

I'd say that the best option we got here is to hoist the include to the top-of-file system-header block, conditionally at the interface level with checking that interface requires the header.

I came up with mathias-luedtke@620a026
However, I am not convinced..

but i guess that perfectly acceptable fallback: always include <stdint.h>

It turns out that Python.h already includes it, so it should not affect compile time.
-> We could even drop the stdint.h include and it still passes.

Furthermore - depending on the message type - the generated code already uses the types from stdint.h without including them explicitly.

IMHO always including stdint.h is the better fix: it is simple and maintainable.
And it allows us to drop the has_buffer_fields handling, which should make up for any (negligible) include overhead.

[MatthijsBurgh]

For whatever it is worth for the re-review: I checked 9dbae59 against a real downstream failure and it resolves it.

ed_gui_server_interfaces/srv/GetEntityInfo.srv has exactly the shape from #264 — no buffer in the request, two in the response — and is red on rolling:

- ed_gui_server_interfaces.cpplint_rosidl_generated_py build/include_order [4] (.../srv/_get_entity_info_s.c:137)
  <<< failure message
    Found C system header after other header. Should be: _get_entity_info_s.h, c system, c++ system, other.
  >>>

(https://github.com/tue-robotics/ed_gui_server/actions/runs/33599833100)

I have no rolling environment at hand, so I checked it two ways:

• The include-emission region of _msg_support.c.em on this branch — from include_parts = down to // end nested array functions include — is identical to jazzy's apart from the 'stdint.h' entry and an empy comment. Jazzy generates a cpplint-clean _get_entity_info_s.c for this service, so this branch does too.
• ament_cpplint --filters=-readability/fn_size --linelength 999 over the generated file for that service: clean as jazzy generates it, reproduces the error above with <stdint.h> live in the response block as rolling emits it today, clean again with <stdint.h> in the top block as this branch emits it.

The second is on a reconstruction of the rolling output rather than the real thing, so please read it as corroboration of the first rather than an independent result. The middle case is the control — without it a clean run would not mean much.

The same argument covers the GetBuffer.srv added here: with stdint.h unconditional it is always in the first block, so the testcase should be red before 9dbae59 and green after, which I think addresses the "generates intentionally red test" point from the review.