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

More specific prefix in some cmake_parse_argument calls #523

Conversation

eggerk
Copy link
Contributor

@eggerk eggerk commented Apr 11, 2024

The macro ament_auto_package doesn't properly pass on the additional arguments to ament_package. The reason is that the call to the macro ament_execute_extensions again calls cmake_parse_arguments with the same variable prefix, effectively overwriting the original _ARG_UNPARSED_ARGUMENTS variable and leaving it empty.

I have found this issue to be present both in galactic (which I'm aware is already EOL) and rolling.

As a fix, I renamed changed the prefix argument for cmake_parse_arguments to have unique and non-conflicting variable names in both these macros.

Signed-off-by: Kevin Egger <kevin.egger@sevensense.ch>
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with green CI

@sloretz
Copy link
Contributor

sloretz commented Apr 18, 2024

CI (repos file build: --packages-up-to rcl ament_cmake_auto test: --packages-up-to rcl ament_cmake_auto)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor

sloretz commented Apr 18, 2024

I think the test failures on aarch64 are unrelated, but I'll have to wait to merge this PR until after the freeze: https://discourse.ros.org/t/freeze-of-ros-2-base-packages-upcoming-branch-and-tutorial-party-for-jazzy-jalisco/37191/2

@clalancette
Copy link
Contributor

I think the test failures on aarch64 are unrelated, but I'll have to wait to merge this PR until after the freeze: https://discourse.ros.org/t/freeze-of-ros-2-base-packages-upcoming-branch-and-tutorial-party-for-jazzy-jalisco/37191/2

Yes, they are unrelated. Going ahead and merging this one in.

@clalancette clalancette merged commit fdbf457 into ament:rolling May 13, 2024
3 checks passed
@sloretz
Copy link
Contributor

sloretz commented Jun 28, 2024

@Mergifyio backport iron humble jazzy

Copy link

mergify bot commented Jun 28, 2024

backport iron humble jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jun 28, 2024
Signed-off-by: Kevin Egger <kevin.egger@sevensense.ch>
(cherry picked from commit fdbf457)
mergify bot pushed a commit that referenced this pull request Jun 28, 2024
Signed-off-by: Kevin Egger <kevin.egger@sevensense.ch>
(cherry picked from commit fdbf457)
mergify bot pushed a commit that referenced this pull request Jun 28, 2024
Signed-off-by: Kevin Egger <kevin.egger@sevensense.ch>
(cherry picked from commit fdbf457)
sloretz pushed a commit that referenced this pull request Jun 28, 2024
Signed-off-by: Kevin Egger <kevin.egger@sevensense.ch>
(cherry picked from commit fdbf457)

Co-authored-by: Kevin Egger <eggerk@users.noreply.github.com>
sloretz pushed a commit that referenced this pull request Jun 28, 2024
Signed-off-by: Kevin Egger <kevin.egger@sevensense.ch>
(cherry picked from commit fdbf457)

Co-authored-by: Kevin Egger <eggerk@users.noreply.github.com>
sloretz pushed a commit that referenced this pull request Jun 28, 2024
Signed-off-by: Kevin Egger <kevin.egger@sevensense.ch>
(cherry picked from commit fdbf457)

Co-authored-by: Kevin Egger <eggerk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants