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

update windows.cmake to fix common build issues on Windows #984

Merged
merged 9 commits into from
Jan 28, 2019
Merged

update windows.cmake to fix common build issues on Windows #984

merged 9 commits into from
Jan 28, 2019

Conversation

kejxu
Copy link
Contributor

@kejxu kejxu commented Jan 10, 2019

while building on Windows, it's not uncommon to encounter various platform-specific issues and compiler-specific issues (if using visual c++ compiler, MSVC).

For the debug flags and compiler options, a lot of these issues are already known to the community, and most of them could be resolved with existing good practices:

  • -DNOMINMAX helps avoid collision with std::min/std::max if there's windows.h/windef.h
  • -DNO_STRICT (and -DQ_NOWINSTRICT for QT) disables STRICT type checking
  • -D_USE_MATH_DEFINES allows using Math Constants since they are not defined in Standard C/C++
  • -DWIN32_LEAN_AND_MEAN excludes various APIs from windows.h to improve build time since basic inclusion would be enough

johnsonshih and others added 8 commits January 10, 2019 13:14
* Avoid using set_target_properties on ALIAS targets

This change is to fix the error like below:
2018-11-30T10:40:39.7610068Z CMake Error at C:/opt/ros/melodic/x64/share/catkin/cmake/platform/windows.cmake:47 (set_target_properties):
2018-11-30T10:40:39.7610963Z   set_target_properties can not be used on an ALIAS target.
2018-11-30T10:40:39.7611263Z Call Stack (most recent call first):
2018-11-30T10:40:39.7611903Z   abseil_cpp/CMake/AbseilHelpers.cmake:67 (add_library)
2018-11-30T10:40:39.7612185Z   abseil_cpp/absl/base/CMakeLists.txt:74 (absl_library)

* nit
)

Walking through the projects we touched for ROS, many of them simply are just missing the dllimport/dllexport declarative, so no imported library is generated and breaks the build for downstream projects. Some are fixed by defining dllexport/dllimport, by defining WINDOWS_EXPORT_ALL_SYMBOLS, or by switching them to STATIC library (STATIC lib sometimes even creates more problems since the author assumes it is a shared lib at run-time).

In order to quickly enabling more untouched ROS packages, I am suggesting to enable CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS globally to quickly enabling them, but we should still encourage the package owners to explicitly use dllexport in our cookbook.
@dirk-thomas
Copy link
Member

While I am not a big fan of working around the various definitions globally within catkin I have to acknowledge that for a large existing code base like ROS 1 it is probably the more realistic approach.

Thank you for the patch.

@dirk-thomas dirk-thomas merged commit 3c62f99 into ros:kinetic-devel Jan 28, 2019
@kejxu kejxu deleted the fix_windows.cmake branch January 28, 2019 21:05
@kejxu
Copy link
Contributor Author

kejxu commented Jan 28, 2019

While I am not a big fan of working around the various definitions globally within catkin I have to acknowledge that for a large existing code base like ROS 1 it is probably the more realistic approach.

Thanks a lot for your understanding! Hopefully this kind of change could be eventually removed from the global build tool (catkin) when all the packages could provide native support Windows (some day); before that, however, this might be an easier way to enable everything working on Windows =)

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