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

Disable a Windows platform warning. #311

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

clalancette
Copy link
Contributor

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This should get rid of most of the warnings we are currently getting from the Windows CI jobs, as seen in https://ci.ros2.org/view/nightly/job/nightly_win_deb/1804/msbuild . See the diff for a better explanation of why we have to do this.

@clalancette
Copy link
Contributor Author

CI:

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

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette force-pushed the clalancette/disable-windows-warning branch from c8353c5 to 8458bba Compare November 11, 2020 18:24
@clalancette
Copy link
Contributor Author

Looks like I missed one in time_win32.c; that should now be fixed. Otherwise, initial CI looks pretty good. I'm going to run a full CI just to make sure that everything still works for all downstreams as well. I expect that to still have one warning on Windows, which needs a separate patch for RViz.

CI:

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

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

I'm surprised I couldn't find any issue saying this can happen without using /experimental:preprocessor, but maybe Windows introduced a bug in MSVC yesterday (?).

The other option is that we're enabling that compiler flag accidentally.
It would be good to double check that.

@clalancette
Copy link
Contributor Author

I'm surprised I couldn't find any issue saying this can happen without using /experimental:preprocessor, but maybe Windows introduced a bug in MSVC yesterday (?).

So we don't have /experimental:preprocessor on, as far as I can tell. We do turn on /Zc:preprocessor (the successor to /experimental:preprocessor) in one place: https://github.com/ros2/rosidl/blob/e22083c970f6235ef4266bab80947f9dbc316c37/rosidl_generator_cpp/CMakeLists.txt#L108 . But as far as I can tell, that should only be for that particular test, so it wouldn't be a global thing.

I'm going to go ahead and merge this as-is to reduce our warnings for now. If Windows/MSVC gets fixed, or we figure out that we are accidentally turning on flags, we can always revert this.

@clalancette
Copy link
Contributor Author

(oh, and all of the failures on Windows are there on the nightlies as well)

@clalancette clalancette merged commit d37bbc5 into master Nov 12, 2020
@clalancette clalancette deleted the clalancette/disable-windows-warning branch November 12, 2020 13:48
@ivanpauno
Copy link
Member

/Zc:preprocessor (the successor to /experimental:preprocessor) in one place: https://github.com/ros2/rosidl/blob/e22083c970f6235ef4266bab80947f9dbc316c37/rosidl_generator_cpp/CMakeLists.txt#L108 .

Yeah, I saw that one.

But as far as I can tell, that should only be for that particular test, so it wouldn't be a global thing.

Agreed, I don't think that will affect other places.

@clalancette clalancette added this to Proposed in Foxy Patch Release 4 via automation Nov 16, 2020
@clalancette clalancette moved this from Proposed to Needs backport in Foxy Patch Release 4 Nov 16, 2020
clalancette added a commit that referenced this pull request Dec 3, 2020
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette added a commit that referenced this pull request Dec 3, 2020
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
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

3 participants