Skip to content

Add stdc++fs as a target link library for clang compiler on linux #144

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.

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

Merged
merged 3 commits into from
Feb 12, 2019

Conversation

bhatsach
Copy link
Contributor

@bhatsach bhatsach commented Feb 8, 2019

This change is needed to compile pluginlib package and packages which
depend on it on linux clang compiler

This change is needed to compile pluginlib package and packages which
depend on it on linux clang compiler
@bhatsach
Copy link
Contributor Author

bhatsach commented Feb 8, 2019

Points to note

  • I am adding a nightly job on ROS2 CI to build on linux clang compiler in order to detect bugs early. Mac OS X builds can take very long to complete. Here's the PR link for the same: ros2/ci#244

Testing done

  • I have tested the above changes by doing a colcon build locally with all the ros2 packages pulled

@tfoote
Copy link
Member

tfoote commented Feb 8, 2019

This approach seems like it's trying to fix the test and not to identify the more general underlying requirement. What is the differentiating factor that requires this flag being added? Can you enumerate the cases, because at a glance at this looks like it should just be detecting linux an adding the additional compile flag? The previous flag was effectively selecting linux as osx runs clang by default and windows is vsbuild.

@bhatsach
Copy link
Contributor Author

bhatsach commented Feb 8, 2019

This approach seems like it's trying to fix the test and not to identify the more general underlying requirement. What is the differentiating factor that requires this flag being added? Can you enumerate the cases, because at a glance at this looks like it should just be detecting linux an adding the additional compile flag? The previous flag was effectively selecting linux as osx runs clang by default and windows is vsbuild.

Makes sense, I may be able to just update the check to verify that we are on linux. Will look into this now.

@bhatsach
Copy link
Contributor Author

bhatsach commented Feb 8, 2019

Updated the PR with just a check for linux for linking experimental filesystem libraries. Also updated the check to linux while exporting the libraries. Experimental Filesystem is a linux specific concept rather than compiler specific.

@nuclearsandwich nuclearsandwich self-assigned this Feb 9, 2019
@bhatsach
Copy link
Contributor Author

@nuclearsandwich and @tfoote, please let me know if you have any comments on the latest revision, thanks

Copy link
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

One question about the CMake logic for long term *nix support. Once that is settled we'll need to run CI.

This change is necessary for building with Clang on Linux. The use of
`UNIX AND NOT APPLE` is cribbed from a bunch of StackOverflow and CMake
mailing list examples to cover non-Apple *nix cases which for ROS 2
today means Linux, basically. But in the future it could also mean
NetBSD or FreeBSD, QNX, or NuttX as long as they provide libc++fs.
@nuclearsandwich
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Linux Clang Build Status

Copy link
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

Will merge as long as CI comes back green.

@bhatsach
Copy link
Contributor Author

One question about the CMake logic for long term *nix support. Once that is settled we'll need to run CI.

Thanks a lot for identifying and updating this. I locally tested it on ubuntu and the change looks good :)

@nuclearsandwich
Copy link
Member

@bhatsach do you have any insight into the warnings that are coming back on the Linux Clang job?

@nuclearsandwich
Copy link
Member

Because the warnings are in other packages and don't seem directly related to pluglinlib and the supported builds have no errors. I'm going to merge with the expectation that those interested in getting the clang on Linux builds fully green will do so.

@nuclearsandwich nuclearsandwich merged commit cc6af50 into ros:ros2 Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants