Compiling ros/time.h in x64 Visual Studio project results in incorrect selection between sys/time.h and sys/timeb.h

[marcbertola]

Hello,

This is my first time compiling ROS into a C++ application, so please bear with me if I'm missing something.

I'm putting together a small application to publish messages using ROS's infrastructure. I have the following include at the top of my file: #include "ros/ros.h"

Which results in: Cannot open include file: 'sys/time.h': No such file or directory testprogram c:\opt\ros\melodic\x64\include\ros\time.h 68

Looking in time.h, I see that it expects WIN32 to be defined to include sys/timeb.h (which exists in Windows) as opposed to sys/time.h (which does not). There seems to be an assumption that WIN32 will be defined in Windows applications, but I have stumbled across a situation where it isn't.

I am using a fairly recent version of Visual Studio 2017, and my program is a console x64 build, and as such, does not have WIN32 defined by default. I'm not sure it is a reliable #define to select between timeb.h and time.h.

Perhaps another built-in preprocessor directive could be used instead? https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=vs-2019

All this being said, the following workaround does the trick: If I include before including ros/time.h, it no longer complains about the issue.

[dirk-thomas]

The code in question:

https://github.com/ros/roscpp_core/blob/a3f26d8970727068ded255f10025dc0e8ac7247c/rostime/include/ros/time.h#L65-L69

The code base uses WIN32 in many places to check for Windows. That is afaik the recommended approach. I am not sure why that is not defined in your case.

Maybe @kejxu can comment on this?

[kejxu]

Thanks for sharing your experience @marcbertola, and redirection @dirk-thomas!

This is a good catch, VS compiler only defines _WIN32; however, we have never noticed build breaks resulting from this. Quick question for @marcbertola, how are you building the application? Are you building with catkin/cmake?

I dug in a bit into the code base, and exactly as you mentioned, WIN32 is defined in <windows.h>:

// minwindef.h
#ifndef WIN32
#define WIN32
#endif

whereas the VS compiler only specifies _WIN32. This is also the reason why our recent port have only been using _WIN32 (for example, https://github.com/ros/roscpp_core/commit/11db0ec91841b7f06062740f6be43e25771b9a10)

[marcbertola]

Hi @kejxu, thanks for looking into this.

I was writing a standalone program to characterize the throughput for a large PointCloud2 through ROS topics. It was a simple console program using standard, portable C++. It created a PointCloud2 message, populated it, and then sent it out repeatedly. I then used one of the standard ROS utilities (can't remember which one) to receive the messages and display the rate at which they were being received. To answer your question: I wasn't building ROS itself or using catkin.

Since I didn't actually need any of the Windows API to do this (just standard C++), I didn't include windows.h, and so WIN32 was not defined. My own research corroborates what you are saying about WIN32 vs _WIN32. When I did include it before including the ROS headers, WIN32 was defined and things started working.

Also, I haven't tried it, it's probably worth checking to see if if either variable is defined if you try compiling with GCC under Cygwin or MinGW. But then again, time.h might actually exist in those scenarios, so it might already be ok.

[dirk-thomas]

Assuming that #110 checking for _WIN32 resolved this I am going ahead and close the ticket. Please feel free to continue commenting and the ticket can be reopened if necessary.

[kejxu]

I wasn't building ROS itself or using catkin.

Thanks! I wouldn't be able to make any assertive conclusions without further investigation, but I had very similar questions when I just started working with catkin. A few of these macros, WIN32 and _WINDOWS for example, I think are injected by catkin's cmake pipeline.

If time allows, I think it would be worthwhile to try building the application with catkin.

Since I didn't actually need any of the Windows API to do this (just standard C++), I didn't include windows.h, and so WIN32 was not defined. My own research corroborates what you are saying about WIN32 vs _WIN32. When I did include it before including the ROS headers, WIN32 was defined and things started working.

You're definitely doing the right thing not including windows.h =)

Also, I haven't tried it, it's probably worth checking to see if if either variable is defined if you try compiling with GCC under Cygwin or MinGW. But then again, time.h might actually exist in those scenarios, so it might already be ok.

This is a very interesting point. I have not had too much experience working with GCC on Windows, but based on what I have read about, _WIN32 might need to be injected from the command line during build time. However, at this point I think we are only targeting MSVC for the win32 platform. But still, this is still a very valid point, thanks for pointing it out!