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.

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

Only export public symbols on non-windows platforms [11754] #1998

Merged
merged 6 commits into from
Dec 20, 2021
Merged

Only export public symbols on non-windows platforms [11754] #1998

merged 6 commits into from
Dec 20, 2021

Conversation

MiguelCompany
Copy link
Member

This should fix #1994 by using LD version scripts to control the list of exported symbols.

@MiguelCompany MiguelCompany added this to the v2.4.0 milestone Jun 4, 2021
@MiguelCompany MiguelCompany mentioned this pull request Jun 4, 2021
Closed
@MiguelCompany MiguelCompany changed the title Only export public symbols on non-windows platforms Only export public symbols on non-windows platforms [11754] Jun 4, 2021
@MiguelCompany MiguelCompany marked this pull request as ready for review June 7, 2021 10:30
@MiguelCompany MiguelCompany modified the milestones: v2.4.0, v2.5.0 Sep 9, 2021
@MiguelCompany MiguelCompany mentioned this pull request Oct 6, 2021
Closed
@MiguelCompany
Refs 11751. Using --version-script to only export symbols containing …
b21be9f 
…eprosima.

Signed-off-by: Miguel Company <miguelcompany@eProsima.com>
@MiguelCompany
Refs 11751. Also exporting the MD5 class.
9134d82 
Signed-off-by: Miguel Company <miguelcompany@eProsima.com>
@MiguelCompany
Refs 11751. Add more exported symbols.
fff2a84 
Signed-off-by: Miguel Company <miguelcompany@eProsima.com>
@MiguelCompany
Refs 11751. Check support for linker option.
1087e79 
Signed-off-by: Miguel Company <miguelcompany@eProsima.com>
@MiguelCompany MiguelCompany force-pushed the bugfix/export-symbols branch from f71df25 to 1087e79 Compare October 6, 2021 12:45
JLBuenoLopez
JLBuenoLopez previously approved these changes Dec 15, 2021
View reviewed changes
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

LGTM, though @MiguelBarro knows more about CMake and might see some issue.

Refs 11751. Pass the linker flags using a well-known cmake variable
8ec1fec 
Signed-off-by: Barro <miguelbarro@eprosima.com>
MiguelBarro
MiguelBarro previously approved these changes Dec 17, 2021
View reviewed changes
Copy link
Contributor

@MiguelBarro MiguelBarro left a comment

Choose a reason for hiding this comment

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

LGTM

@MiguelCompany
Copy link
Member Author

@MiguelBarro CMake documentation on CheckCXXSourceCompiles specifically mentions CMAKE_REQUIRED_FLAGS, so that is what I used.

Was I mistaken?

Revert "Refs 11751. Pass the linker flags using a well-known cmake va…
bbe4de0 
…riable"

This reverts commit 8ec1fec.

Signed-off-by: Barro <miguelbarro@eprosima.com>
MiguelBarro
MiguelBarro previously approved these changes Dec 17, 2021
View reviewed changes
Copy link
Contributor

@MiguelBarro MiguelBarro left a comment

Choose a reason for hiding this comment

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

Using CMAKE_REQUESTED_FLAGS is acceptable because its documented within the check module.

JLBuenoLopez
JLBuenoLopez approved these changes Dec 17, 2021
View reviewed changes
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

👍 😅

@MiguelCompany MiguelCompany added the ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. label Dec 17, 2021
@EduPonz EduPonz merged commit 2a655a2 into master Dec 20, 2021
@EduPonz EduPonz deleted the bugfix/export-symbols branch December 20, 2021 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JLBuenoLopez JLBuenoLopez JLBuenoLopez approved these changes

@MiguelBarro MiguelBarro MiguelBarro left review comments

Assignees
No one assigned
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.

SQLite conflict [11751]
4 participants
@MiguelCompany @MiguelBarro @JLBuenoLopez @EduPonz