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

Disable Ogre deprecation warning on Windows #242

Merged
merged 2 commits into from
Apr 12, 2018
Merged

Disable Ogre deprecation warning on Windows #242

merged 2 commits into from
Apr 12, 2018

Conversation

greimela-si
Copy link
Contributor

The Windows build currently shows a deprecation warning in Ogre when building with Visual Studio 2017.

@dirk-thomas
Copy link
Member

Is this related to #184? Does it address any of these / all of these / or different warnings?

@greimela-si
Copy link
Contributor Author

No, this PR only fixes one specific warning that occurs since #186 has been merged (yesterday).

Nevertheless, we are currently able to build rviz using VS2017 without any warnings.
I would have triggered a new CI run but it looks like the ci_windows job seems to have some problems...

@dirk-thomas
Copy link
Member

dirk-thomas commented Apr 11, 2018 •
edited
Loading

we are currently able to build rviz using VS2017 without any warnings.

Can you create a PR with these changes to get rid of all the remaining warning. We soon want to switch to VS 2017 and that is the last code change we are waiting for.

I would have triggered a new CI run but it looks like the ci_windows job seems to have some problems...

One of the nodes (icecube) failed recent jobs. It has been taken offline since then. Please feel free to reschedule your build anytime.

@greimela-si
Copy link
Contributor Author

Can you create a PR with these changes to get rid of all the remaining warning.

I think my comment was a bit misunderstanding.
I wanted to say that we are able to build the latest ros2 branch without warnings using VS2017.

@dirk-thomas dirk-thomas mentioned this pull request Apr 11, 2018
Closed
wjwwood
wjwwood reviewed Apr 11, 2018
View reviewed changes
rviz_default_plugins/test/rviz_default_plugins/displays/image/ros_image_texture_test.cpp Outdated
# include <Ogre.h> // NOLINT
# pragma warning(pop)
#else
# include <Ogre.h> // NOLINT
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer that we do not repeat the include statement in this case since it's not different across OS's.

Copy link
Member

Choose a reason for hiding this comment

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

Here's what I would prefer:

#ifdef _WIN32
# pragma warning(push)
# pragma warning(disable : 4996)
#endif

#include <Ogre.h>  // NOLINT

#ifdef _WIN32
# pragma warning(pop)
#endif

I tried to make pr through the webui, but it gave me a 404 for some reason.

Is this change ok with you @greimela-si?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that looks good to me!

@wjwwood
Copy link
Member

wjwwood commented Apr 11, 2018 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status (looks like a bug due to RTI admin console being open)
  • Windows Build Status (same here)

@wjwwood wjwwood merged commit 923c51c into ros2:ros2 Apr 12, 2018
@greimela-si greimela-si deleted the bugfix/ogre_deprecation_warning branch April 12, 2018 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@greimela-si @dirk-thomas @wjwwood