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

Fixes #1484. #1944

Merged
merged 1 commit into from
Apr 30, 2021
Merged

Fixes #1484. #1944

merged 1 commit into from
Apr 30, 2021

Conversation

pablorcum
Copy link
Contributor

Asio submodule updated to tag 1.18.1.
Added define ASIO_DISABLE_VISIBILITY.

Signed-off-by: promero promero@mathworks.com

@pablorcum
Fixes #1484.
e5ca764 
Asio submodule updated to tag 1.18.1.
Added define ASIO_DISABLE_VISIBILITY.

Signed-off-by: promero <promero@mathworks.com>
MiguelCompany
MiguelCompany approved these changes Apr 30, 2021
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

LGTM. Let's see the CI results.

@MiguelCompany
Copy link
Member

@pablorcum Just one question. Will this work when Fast DDS is built on a system with a current installation of asio?
I see that the package version of libasio-dev on the Ubuntu apt repositories is 1.12.2 up to Groovy. On Hirsute it changes to 1.18.1

Will ASIO_DISABLE_VISIBILITY make any harm on 1.12.2, or will it just be ignored?

@pablorcum
Copy link
Contributor Author

@MiguelCompany Yes, it will work in the same way as before this commit. Updating the submodule has no impact in using an existing installation of asio, and the new define flag will be ignored.

In fact, I've just locally tested to build the project with an additional dummy define that does not exist, and did not have any impact. Therefore, old asio versions that do not support ASIO_DISABLE_VISIBILITY will keep working as today.

@MiguelCompany
Copy link
Member

@richiprosima Please test linux

@MiguelCompany
Copy link
Member

CI seems ok. Only known flakey failures.
Thank you @pablorcum for your contribution.

@MiguelCompany MiguelCompany merged commit 5579747 into eProsima:master Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelCompany MiguelCompany MiguelCompany 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.
2 participants
@pablorcum @MiguelCompany