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.

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

[ros2] Adding visibility control headers for Windows build #159

Merged
merged 1 commit into from
Apr 30, 2020

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented Apr 8, 2020

  • Adding visibility control headers for Windows build
  • Defined _SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING to ack the <experimental/filesystem> usage on MSVC. Otherwise, it manifests as a compile error.

@seanyen
Copy link
Contributor Author

seanyen commented Apr 8, 2020

@jacobperron @mjcarroll This is ready for review and merge. Thanks.

@mjcarroll
Copy link
Contributor

Defined _SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING to ack the <experimental/filesystem> usage on MSVC. Otherwise, it manifests as a compile error.

Since this is coming from the rcpputils package, does it make sense to add that definition there rather than here?

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Everything else looks good once we determine where that compiler definition should go.

@mjcarroll
Copy link
Contributor

Oh, I apologize, I thought that I had already landed #157 which removes the filesystem_helper.

Let me get that tested and in, then the define would probably make more sense in the rcpputils package.

@seanyen
Copy link
Contributor Author

seanyen commented Apr 29, 2020

@mjcarroll Friendly ping.

@mjcarroll mjcarroll merged commit bd2a7f9 into ros-perception:ros2 Apr 30, 2020
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.

None yet

2 participants