Skip to content

Implement ament_add_default_options #390

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 8, 2022
Merged

Implement ament_add_default_options #390

merged 5 commits into from
Jun 8, 2022

Conversation

methylDragon
Copy link
Contributor

This PR adds the ament_add_default_options() macro to allow for explicit invocation for common options.

Currently only one option is contained in the macro (BUILD_SHARED_LIBS), but the macro can be extended.

This will allow ros2/rmw_dds_common#62 to be merged (with the macro call).

@methylDragon
Copy link
Contributor Author

Should I also add a comment on ament_index https://github.com/ament/ament_index/blob/5ce2eba781801154d93c6fd7d6f66b23e4a7182d/ament_index_cpp/CMakeLists.txt#L13-L18 for deprecation?

Something like:

# TODO(CH3): This will be left in, but is by-right deprecated with the addition of
#  ament_add_default_options(). It's left in to prevent needing to send mass PRs
#  to add the ament_add_default_options() to all the libraries.

@methylDragon methylDragon force-pushed the ch3/ament_add_default_options branch from 02e9295 to 86a2dc5 Compare May 26, 2022 19:11
sloretz
sloretz reviewed May 26, 2022
View reviewed changes
ament_cmake_core/cmake/core/ament_add_default_options.cmake Outdated
# TODO(CH3): Would be good to parse args to skip options next time.
set(options EXCLUDE_BUILD_SHARED_LIBS)
set(oneValueArgs)
set(multiValueArgs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since CMake macro()s don't have a separate scope for variables, I'd recommend prefixing these variables to avoid collisions. Maybe options -> _aado_options?

Copy link
Contributor Author

@methylDragon methylDragon May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah good idea, I've been spoilt with how it's been used in the ignition gz stack 😅

cc27182

@methylDragon
Copy link
Contributor Author

I don't have write access, so if this is ok, feel free to merge it for me 👀

@clalancette
Copy link
Contributor

I don't have write access, so if this is ok, feel free to merge it for me eyes

Note that this will need approval and CI run on it (via https://ci.ros2.org) before merging.

@methylDragon
Copy link
Contributor Author

Ah!

Windows: Build Status
Linux: Build Status
Linux-aarch64: Build Status

wjwwood
wjwwood reviewed Jun 2, 2022
View reviewed changes
@methylDragon
Unset macro variables at end
ce7703a 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Update comment and string
d3a2818 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Copy link
Contributor Author

methylDragon commented Jun 2, 2022 •
edited
Loading

Rerunning CI

Windows: Build Status
Linux: Build Status
Linux-aarch64: Build Status

wjwwood
wjwwood approved these changes Jun 8, 2022
View reviewed changes
ament_cmake_core/cmake/core/ament_add_default_options.cmake Outdated
ON)
endif()

unset(aado_options EXCLUDE_BUILD_SHARED_LIBS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a copy-paste error, but I think the EXCLUDE_BUILD_SHARED_LIBS is not needed.

@methylDragon
Refine macro unsets
2d0751e 
Signed-off-by: methylDragon <[email protected]>
@methylDragon methylDragon merged commit 1aebbce into ament:master Jun 8, 2022
@methylDragon methylDragon deleted the ch3/ament_add_default_options branch June 8, 2022 21:48
This was referenced Jun 9, 2022
Merged
Merged
Merged
hannes09 pushed a commit to hannes09/ament_cmake that referenced this pull request Dec 17, 2022
@methylDragon
Implement ament_add_default_options (ament#390)
104467f 
* Implement ament_add_default_options

Signed-off-by: methylDragon <[email protected]>

* Mangle cmake_parse_arguments vars

Signed-off-by: methylDragon <[email protected]>

* Unset macro variables at end

Signed-off-by: methylDragon <[email protected]>

* Update comment and string

Signed-off-by: methylDragon <[email protected]>

* Refine macro unsets

Signed-off-by: methylDragon <[email protected]>
@TannerGilbert TannerGilbert mentioned this pull request Mar 24, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz left review comments

@wjwwood wjwwood wjwwood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@methylDragon @clalancette @wjwwood @sloretz