Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Remove connext workaround#275

Merged
mikaelarguedas merged 2 commits intomasterfrom
remove_connext_workaround
Dec 15, 2017
Merged

Remove connext workaround#275
mikaelarguedas merged 2 commits intomasterfrom
remove_connext_workaround

Conversation

@mikaelarguedas
Copy link
Copy Markdown
Member

@mikaelarguedas mikaelarguedas commented Dec 11, 2017 •
edited
Loading

From what I understand this was only necessary for Ubuntu connext builds with gcc4.8.2 (https://community.rti.com/static/documentation/connext-dds/5.3.0/doc/manuals/connext_dds/RTI_ConnextDDS_CoreLibraries_PlatformNotes.pdf).
As we now target connext 5.3.0 that is built with gcc5.4 this doesnt apply anymore.

I don't have the full context, relevant discussion: ros2/rcl#55 (comment)

If we decide to keep it it may make sense to export these flags (#178 (comment)) to avoid workarounds in downstream packages

  • Linux Debug Build Status

  • Linux Release Build Status

Edit: New ci:

  • Linux Debug Build Status

  • Linux Release Build Status

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Dec 11, 2017
This was referenced Dec 11, 2017
Merged
Closed
@mikaelarguedas mikaelarguedas self-assigned this Dec 11, 2017
@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Dec 12, 2017

CI is happy, ready for review

@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 12, 2017
@dhood
Copy link
Copy Markdown
Member

dhood commented Dec 12, 2017

this seems safe to remove now

I have some additional context since I was involved in the issue/PRs at the time, so I'll dump it here for the future (you were probably already aware of it all @mikaelarguedas it's just for general knowledge)

As we now target connext 5.3.0 that is built with gcc5.4 this doesnt apply anymore.

Assuming they fixed the issue moving forwards.. if CI passes without the workaround now then I suppose so 😄 At the time the issue (ros2/rcl#52) was only when using binaries off their website (not the debs we built ourselves), and now we use website binaries in our CI, so if it works for us it should work for everyone.

If we decide to keep it it may make sense to export these flags to avoid workarounds in downstream packages

Though it looks like it from ros2/rcl#55, workarounds aren't necessary in downstream packages since #188 (#173 was superseded by #188). The workaround in ros2/rcl#55 was just done before it, and probably could have been removed afterwards.

@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Dec 15, 2017

Thanks for the summary, it's good to have confirmation as I tried to follow these various connected issues but wasn't sure I got all the context.

@mikaelarguedas mikaelarguedas merged commit 4030f24 into master Dec 15, 2017
@mikaelarguedas mikaelarguedas deleted the remove_connext_workaround branch December 15, 2017 11:16
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Dec 15, 2017
@dhood dhood mentioned this pull request Feb 12, 2018
Merged
Karsten1987 added a commit that referenced this pull request Feb 12, 2018
@Karsten1987
Revert "Remove connext workaround (#275)"
49bdf2a 
This reverts commit 4030f24.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@dhood dhood dhood approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mikaelarguedas mikaelarguedas

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mikaelarguedas @dhood