Skip to content

Explicitly link against dl for dladdr()#48

Merged
clalancette merged 1 commit intorollingfrom
christophebedard/rhel-dladdr-fix
Mar 2, 2023
Merged

Explicitly link against dl for dladdr()#48
clalancette merged 1 commit intorollingfrom
christophebedard/rhel-dladdr-fix

Conversation

@christophebedard
Copy link
Copy Markdown
Member

@christophebedard christophebedard commented Mar 1, 2023

Fix regression introduced by #43 on RHEL.

It looks like we need to explicitly link against dl on RHEL. Note that, if TRACETOOLS_LTTNG_ENABLED, LTTng's link flags already include -ldl.

Signed-off-by: Christophe Bedard bedard.christophe@gmail.com

@christophebedard christophebedard self-assigned this Mar 1, 2023
@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Mar 1, 2023

CI job showing tracetools build failure on RHEL: https://ci.ros2.org/job/ci_linux-rhel/322/consoleFull#console-section-217

A different but functionally identical commit makes tracetools build fine on RHEL: https://ci.ros2.org/job/ci_linux-rhel/323/console#console-section-217

@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Mar 1, 2023 •
edited
Loading

Testing --packages-above tracetools:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

and:

  • RHEL Build Status (same as the one I linked to above)

The Rpr job is expected to fail due to the previous ABI break (some packages in this repo depend on rclcpp which depends on tracetools). The local GitHub actions are expected to fail for unrelated reasons.

@christophebedard christophebedard mentioned this pull request Mar 1, 2023
Merged
@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Mar 1, 2023

@cottsay would appreciate a RHEL-specific set of eyes on this one, thanks!

cottsay
cottsay approved these changes Mar 1, 2023
View reviewed changes
@christophebedard christophebedard force-pushed the christophebedard/rhel-dladdr-fix branch 2 times, most recently from 4229e6e to 7076b64 Compare March 1, 2023 16:56
cottsay
cottsay approved these changes Mar 1, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on a quick fix for this.

@cottsay
Copy link
Copy Markdown
Member

cottsay commented Mar 1, 2023

Fresh CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

cottsay
cottsay reviewed Mar 1, 2023
View reviewed changes
@christophebedard christophebedard force-pushed the christophebedard/rhel-dladdr-fix branch from 4c87434 to f5fa292 Compare March 1, 2023 18:08
@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Mar 1, 2023 •
edited
Loading

  • Linux-rhel Build Status

FYI that job is testing a different branch (christophebedard/rhel-dladdr-fix-test for my initial fix PoC), so it's not testing the new changes. I've deleted that branch now.

@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Mar 1, 2023 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows canceled

This was referenced Mar 1, 2023
Closed
Closed
christophebedard
christophebedard commented Mar 1, 2023
View reviewed changes
@christophebedard @cottsay
Explicitly link against dl for dladdr()
6f588bf 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Co-authored-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
@christophebedard christophebedard force-pushed the christophebedard/rhel-dladdr-fix branch from f5fa292 to 6f588bf Compare March 1, 2023 22:43
@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Mar 1, 2023

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Mar 2, 2023

The test failures on aarch64 and RHEL are known, and match the nightlies. The Windows test failures are new, but I have a hard time seeing how this PR would cause them given its contents. I'm going to go ahead and merge this (and then do a new release here), and we'll see what happens.

@clalancette clalancette merged commit 32b1377 into rolling Mar 2, 2023
@clalancette clalancette deleted the christophebedard/rhel-dladdr-fix branch March 2, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cottsay cottsay cottsay approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

Assignees

@christophebedard christophebedard

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@christophebedard @mjcarroll @cottsay @clalancette