Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Generate valid visibility macros by replacing hyphens in component name #135

Merged
merged 1 commit into from
Dec 30, 2020

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Dec 21, 2020

For cases where ign_add_component is being called using a name with hyphens, ign-cmake will generate a visibility macro using the hyphens, which is an invalid character. The PR changes two things:

  • Safety internal check to error on presence of hyphens
  • Patch _ign_add_library_or_component to replace the hyphens by underscores

If you want to see the problem in action, create a colcon workspace for ign-gazebo and use the win branch. This is the kind of error it generates:

In file included from /tmp/ign-gazebo/src/systems/air_pressure/AirPressure.hh:22,
                 from /tmp/ign-gazebo/src/systems/air_pressure/AirPressure.cc:18:
/var/lib/jenkins/workspace/ignition_gazebo-abichecker-any_to_any-ubuntu_auto-amd64/build/include/ignition/gazebo/air-pressure-system/Export.hh:25:28: warning: extra tokens at end of #ifndef directive
 #ifndef IGNITION_GAZEBO_AIR-PRESSURE-SYSTEM_EXPORT_HH_
                            ^
/var/lib/jenkins/workspace/ignition_gazebo-abichecker-any_to_any-ubuntu_auto-amd64/build/include/ignition/gazebo/air-pressure-system/Export.hh:26:28: warning: ISO C++11 requires whitespace after the macro name
 #define IGNITION_GAZEBO_AIR-PRESSURE-SYSTEM_EXPORT_HH_

@j-rivero j-rivero requested a review from mxgrey as a code owner December 21, 2020 15:06
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Dec 21, 2020
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM, is there some way we can run a full CI to make sure that it doesn't impact anything downstream?

@j-rivero
Copy link
Contributor Author

LGTM, is there some way we can run a full CI to make sure that it doesn't impact anything downstream?

umm, the out-of-the-box implementation I can think of is using the colcon windows builds with a custom gazebodistro using this branch. Here is a rendering one: Build Status

@mjcarroll
Copy link
Contributor

Okay, the current build failures here are because of #134, fixed by #138 . Let's wait and land that first so we can see the actions turn green here.

@mjcarroll
Copy link
Contributor

@j-rivero Sorry, it just looks like this needs a DCO signoff now.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina force-pushed the visibility_macro_hyphen branch from c347ad4 to df7ae7f Compare December 30, 2020 03:32
@chapulina
Copy link
Contributor

Rebased and fixed DCO. Merging 👍

@chapulina chapulina merged commit 07b793f into ign-cmake2 Dec 30, 2020
@chapulina chapulina deleted the visibility_macro_hyphen branch December 30, 2020 03:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants