Use visibility hidden by default #392

j-rivero · 2023-10-26T17:05:27Z

🎉 New feature

Part of #166

Note: all Ionic branches need to be checked and fixed before merging.

Summary

The commit changes the gz-cmake projects to use C/C++ visibility hidden by default. This is a potential breaking changed for projects using gz-cmake but the benefits in terms of creating portable code and time spend by the loader could be huge.

To avoid this change, the EXPOSE_SYMBOLS_BY_DEFAULT flag can be used in the gz_configure_project call.

The change deprecates the HIDDEN_SYMBOLS_BY_DEFAULT flag.

Test it

Create an Ionic colcon workspace and build it with this branch.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

The commit changes the gz-cmake projects to use C/C++ visibility hidden by default. This is a potential breaking changed for projects using gz-cmake but the benefits in terms of creating portable code and time spend by the loader could be huge. To avoid this change, the EXPOSE_SYMBOLS_BY_DEFAULT flag can be used in the gz_configure_project call. The change deprecates the HIDDEN_SYMBOLS_BY_DEFAULT flag. Signed-off-by: Jose Luis Rivero <[email protected]>

azeey · 2023-10-31T19:06:15Z

This is a good example of a PR that needs CI runs on all downstream libraries building this PR from source. Cross-ref gazebo-tooling/release-tools#851

scpeters · 2023-11-04T00:18:09Z

This is a good example of a PR that needs CI runs on all downstream libraries building this PR from source. Cross-ref gazebo-tooling/release-tools#851

example for testing with gz-utils in gazebosim/gz-utils#115

Part of testing gazebosim/gz-cmake#392 Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Steve Peters <[email protected]>

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

j-rivero · 2023-11-07T17:08:29Z

@osrf-jenkins run tests

j-rivero · 2023-11-07T17:42:51Z

@osrf-jenkins run tests

* Use single header for MovingWindowFilter * Use HIDE_SYMBOLS_BY_DEFAULT Part of testing gazebosim/gz-cmake#392 Signed-off-by: Jose Luis Rivero <[email protected]> Signed-off-by: Steve Peters <[email protected]> Co-authored-by: Steve Peters <[email protected]>

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

scpeters · 2023-11-08T03:20:08Z

wow, apparently we've already been using HIDE_SYMBOLS_BY_DEFAULT in sdformat since gazebosim/sdformat#780 (comment)

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Steve Peters <[email protected]>

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Steve Peters <[email protected]>

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Jose Luis Rivero <[email protected]> Co-authored-by: Jose Luis Rivero <[email protected]>

caguero · 2024-01-04T14:36:02Z

@j-rivero , any outstanding PR in any downstream libraries that we should merge before this one?

j-rivero · 2024-01-04T21:42:03Z

@j-rivero , any outstanding PR in any downstream libraries that we should merge before this one?

Nop! we are good to go.

Signed-off-by: Jose Luis Rivero <[email protected]>

j-rivero · 2024-01-10T15:41:54Z

Before merging, just found that we don't have a Migration.md entry for this, done in 75ae798

scpeters · 2024-01-10T18:44:48Z

@j-rivero , any outstanding PR in any downstream libraries that we should merge before this one?

Nop! we are good to go.

actually, do we want to remove the HIDE_SYMBOLS_BY_DEFAULT parameter from all the downstream libraries now that we've confirmed that it works? I think if we merge this now it will add cmake deprecation warnings everywhere

j-rivero · 2024-01-11T17:06:00Z

@j-rivero , any outstanding PR in any downstream libraries that we should merge before this one?

Nop! we are good to go.

actually, do we want to remove the HIDE_SYMBOLS_BY_DEFAULT parameter from all the downstream libraries now that we've confirmed that it works? I think if we merge this now it will add cmake deprecation warnings everywhere

Yep. See #166 (comment) and linked PRs after the comment.

j-rivero · 2024-01-22T10:58:56Z

Time has come. Ready to merge 🚀

azeey

LGTM! Just a few documentation related comments

Migration.md

azeey · 2024-01-22T16:57:16Z

Migration.md

@@ -9,6 +9,17 @@ release will remove the deprecated code.

 1. The minimum required cmake version is now 3.22.1.

+1. **Breaking**: C/C++ projects enable the visibility=hidden by default
+   gz-cmake4 changes the gz-cmake projects to use C/C++ visibility hidden
+   by default. This is a potential breaking changed for projects using


Suggested change

by default. This is a potential breaking changed for projects using

by default. This is a potential breaking change for projects using

azeey · 2024-01-22T16:57:31Z

Migration.md

+   gz-cmake4 changes the gz-cmake projects to use C/C++ visibility hidden
+   by default. This is a potential breaking changed for projects using
+   gz-cmake but the benefits in terms of creating portable code and
+   time spend by the loader could be relevant.


Suggested change

time spend by the loader could be relevant.

reducing the time spent by the loader could be significant.

azeey · 2024-01-22T16:58:30Z

Migration.md

+   by default. This is a potential breaking changed for projects using
+   gz-cmake but the benefits in terms of creating portable code and
+   time spend by the loader could be relevant.
+   To avoid this change, the EXPOSE_SYMBOLS_BY_DEFAULT flag can be used in


Suggested change

To avoid this change, the EXPOSE_SYMBOLS_BY_DEFAULT flag can be used in

To avoid this change, the `EXPOSE_SYMBOLS_BY_DEFAULT` flag can be used in

azeey · 2024-01-22T16:58:58Z

Migration.md

+   the gz_configure_project call.
+   The change deprecates the HIDDEN_SYMBOLS_BY_DEFAULT flag that can be


Suggested change

the gz_configure_project call.

The change deprecates the HIDDEN_SYMBOLS_BY_DEFAULT flag that can be

the `gz_configure_project` call.

The change deprecates the `HIDDEN_SYMBOLS_BY_DEFAULT` flag that can be

Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Jose Luis Rivero <[email protected]>

j-rivero

Thanks!

j-rivero requested review from mxgrey and scpeters as code owners October 26, 2023 17:05

j-rivero mentioned this pull request Oct 30, 2023

Add missing visibility declarations gazebosim/gz-common#548

Merged

8 tasks

mjcarroll assigned j-rivero Oct 30, 2023

j-rivero mentioned this pull request Oct 31, 2023

Fix visiblity for linux: move explicit instanciations to the header file gazebosim/gz-math#565

Closed

8 tasks

azeey mentioned this pull request Oct 31, 2023

Turn off c++ visibility on CI linux jobs gazebo-tooling/release-tools#380

Open

This was referenced Nov 3, 2023

Use HIDE_SYMBOLS_BY_DEFAULT, fix MovingWindowFilter gazebosim/gz-math#566

Merged

Use GZ_ visibilty macro instead of custom _EXPORTS_API gazebosim/gz-sensors#400

Merged

scpeters mentioned this pull request Nov 4, 2023

Use HIDE_SYMBOLS_BY_DEFAULT gazebosim/gz-utils#115

Merged

9 tasks

j-rivero mentioned this pull request Nov 6, 2023

Library symbols should be hidden by default #166

Closed

scpeters added a commit to gazebosim/gz-math that referenced this pull request Nov 6, 2023

Use HIDE_SYMBOLS_BY_DEFAULT

75a19c7

Part of testing gazebosim/gz-cmake#392 Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Nov 6, 2023

Use HIDE_SYMBOLS_BY_DEFAULT gazebosim/gz-math#567

Closed

9 tasks

scpeters added a commit to gazebosim/gz-plugin that referenced this pull request Nov 6, 2023

Use HIDE_SYMBOLS_BY_DEFAULT

d49c544

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Nov 6, 2023

Use HIDE_SYMBOLS_BY_DEFAULT gazebosim/gz-plugin#130

Merged

9 tasks

scpeters added a commit to gazebosim/gz-plugin that referenced this pull request Nov 7, 2023

Use HIDE_SYMBOLS_BY_DEFAULT (#130)

2d4793d

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

scpeters added a commit to gazebosim/gz-common that referenced this pull request Nov 8, 2023

Use HIDE_SYMBOLS_BY_DEFAULT

c4c6e48

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

scpeters added a commit to gazebosim/gz-msgs that referenced this pull request Nov 8, 2023

Use HIDE_SYMBOLS_BY_DEFAULT

832e89b

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

This was referenced Nov 8, 2023

Use HIDE_SYMBOLS_BY_DEFAULT gazebosim/gz-msgs#407

Merged

Use HIDE_SYMBOLS_BY_DEFAULT gazebosim/gz-common#551

Closed

scpeters added a commit to gazebosim/gz-msgs that referenced this pull request Nov 13, 2023

Use HIDE_SYMBOLS_BY_DEFAULT (#407)

875c054

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

scpeters added a commit to gazebosim/gz-transport that referenced this pull request Nov 13, 2023

Use HIDE_SYMBOLS_BY_DEFAULT

dd94bb8

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Nov 13, 2023

Use HIDE_SYMBOLS_BY_DEFAULT gazebosim/gz-transport#461

Merged

9 tasks

scpeters added a commit to gazebosim/gz-fuel-tools that referenced this pull request Nov 13, 2023

Use HIDE_SYMBOLS_BY_DEFAULT

68260f4

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Nov 13, 2023

Use HIDE_SYMBOLS_BY_DEFAULT gazebosim/gz-fuel-tools#389

Merged

9 tasks

scpeters added a commit to gazebosim/gz-rendering that referenced this pull request Nov 13, 2023

Use HIDE_SYMBOLS_BY_DEFAULT

63bd48c

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Nov 13, 2023

Use HIDE_SYMBOLS_BY_DEFAULT gazebosim/gz-rendering#941

Merged

9 tasks

iche033 pushed a commit to gazebosim/gz-rendering that referenced this pull request Nov 13, 2023

Use HIDE_SYMBOLS_BY_DEFAULT (#941)

122bea1

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

scpeters added a commit to gazebosim/gz-transport that referenced this pull request Nov 13, 2023

Use HIDE_SYMBOLS_BY_DEFAULT (#461)

c6d47cc

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

scpeters added a commit to gazebosim/gz-physics that referenced this pull request Nov 14, 2023

Use HIDE_SYMBOLS_BY_DEFAULT

27279af

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Nov 14, 2023

Use HIDE_SYMBOLS_BY_DEFAULT gazebosim/gz-physics#576

Merged

9 tasks

This was referenced Nov 16, 2023

Enable HIDE_SYMBOLS_BY_DEFAULT + patches gazebosim/gz-gui#600

Closed

Enable HIDE_SYMBOLS_BY_DEFAULT + patches gazebosim/gz-sensors#403

Closed

Enable HIDE_SYMBOLS_BY_DEFAULT + linux patches gazebosim/gz-sim#2248

Merged

scpeters added a commit to gazebosim/gz-fuel-tools that referenced this pull request Nov 16, 2023

Use HIDE_SYMBOLS_BY_DEFAULT (#389)

1e0a69d

Part of testing gazebosim/gz-cmake#392. Signed-off-by: Steve Peters <[email protected]>

Merge branch 'main' into jrivero/visibility_hidden_by_default

f0eabef

j-rivero mentioned this pull request Nov 29, 2023

Enable HIDE_SYMBOLS_BY_DEFAULT + patches (take II) gazebosim/gz-gui#601

Merged

9 tasks

Merge branch 'main' into jrivero/visibility_hidden_by_default

1fc2258

Include the change in Migration.md

75ae798

Signed-off-by: Jose Luis Rivero <[email protected]>

azeey approved these changes Jan 22, 2024

View reviewed changes

Update Migration.md

6f5927c

Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Jose Luis Rivero <[email protected]>

j-rivero commented Jan 26, 2024

View reviewed changes

j-rivero merged commit 646292a into main Jan 26, 2024
4 of 7 checks passed

j-rivero deleted the jrivero/visibility_hidden_by_default branch January 26, 2024 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use visibility hidden by default #392

Use visibility hidden by default #392

j-rivero commented Oct 26, 2023

azeey commented Oct 31, 2023

scpeters commented Nov 4, 2023

j-rivero commented Nov 7, 2023

j-rivero commented Nov 7, 2023

scpeters commented Nov 8, 2023

caguero commented Jan 4, 2024 •

edited

Loading

j-rivero commented Jan 4, 2024

j-rivero commented Jan 10, 2024

scpeters commented Jan 10, 2024

j-rivero commented Jan 11, 2024

j-rivero commented Jan 22, 2024 •

edited

Loading

azeey left a comment

azeey Jan 22, 2024

azeey Jan 22, 2024

azeey Jan 22, 2024

azeey Jan 22, 2024

j-rivero left a comment

	by default. This is a potential breaking changed for projects using
	by default. This is a potential breaking change for projects using

	time spend by the loader could be relevant.
	reducing the time spent by the loader could be significant.

	To avoid this change, the EXPOSE_SYMBOLS_BY_DEFAULT flag can be used in
	To avoid this change, the `EXPOSE_SYMBOLS_BY_DEFAULT` flag can be used in

		the gz_configure_project call.
		The change deprecates the HIDDEN_SYMBOLS_BY_DEFAULT flag that can be

Use visibility hidden by default #392

Use visibility hidden by default #392

Conversation

j-rivero commented Oct 26, 2023

🎉 New feature

Summary

Test it

Checklist

azeey commented Oct 31, 2023

scpeters commented Nov 4, 2023

j-rivero commented Nov 7, 2023

j-rivero commented Nov 7, 2023

scpeters commented Nov 8, 2023

caguero commented Jan 4, 2024 • edited Loading

j-rivero commented Jan 4, 2024

j-rivero commented Jan 10, 2024

scpeters commented Jan 10, 2024

j-rivero commented Jan 11, 2024

j-rivero commented Jan 22, 2024 • edited Loading

azeey left a comment

Choose a reason for hiding this comment

azeey Jan 22, 2024

Choose a reason for hiding this comment

azeey Jan 22, 2024

Choose a reason for hiding this comment

azeey Jan 22, 2024

Choose a reason for hiding this comment

azeey Jan 22, 2024

Choose a reason for hiding this comment

j-rivero left a comment

Choose a reason for hiding this comment

caguero commented Jan 4, 2024 •

edited

Loading

j-rivero commented Jan 22, 2024 •

edited

Loading