Enable catkin build use_nmake on Windows #949
Conversation
python/catkin/builder.py
Outdated
| @@ -427,7 +427,7 @@ def build_catkin_package( | |||
| make_executable = 'make' | |||
|
|
|||
| make_cmd = [make_executable] | |||
| make_cmd.extend(handle_make_arguments(make_args)) | |||
| make_cmd.extend(handle_make_arguments(make_args, not use_nmake)) | |||
There was a problem hiding this comment.
Please pass keyword arguments by explicitly using the keyword: handle_make_arguments(make_args, append_default_jobs_flags=not use_nmake).
Same below.
There was a problem hiding this comment.
use keyword argument
python/catkin/builder.py
Outdated
| @@ -702,12 +704,13 @@ def build_package( | |||
|
|
|||
|
|
|||
| def get_new_env(package, develspace, installspace, install, last_env, destdir=None): | |||
| env_script = 'env.bat' if platform.system() is 'Windows' else 'env.sh' | |||
There was a problem hiding this comment.
Should the condition use sys.platform == 'win32' instead to not use .bat when using cygwin?
There was a problem hiding this comment.
change to use sys.platform
| set CATKIN_ENV_HOOK_WORKSPACE= | ||
|
|
||
| set /a _HOOK_COUNT=%_HOOK_COUNT%+1 | ||
| goto :hook_loop |
There was a problem hiding this comment.
Can you please elaborate why this was changed.
There was a problem hiding this comment.
This is to match what the find_env_hooks() in _setup_util.py template (cmake/templates/_setup_util.py.in) does. find_env_hooks() will put env hooks to environment variable CATKIN_ENVIRONMENT_HOOKS%d and CATKIN_ENVIRONMENT_HOOKS%d_WORKSPACE. This batch file will look for those variables and invoke them
| @@ -165,6 +169,9 @@ message(STATUS "catkin ${catkin_VERSION}") | |||
| # ensure that no current package name is set | |||
| unset(_CATKIN_CURRENT_PACKAGE) | |||
|
|
|||
| # tools/libraries.cmake | |||
| configure_shared_library_build_settings() | |||
There was a problem hiding this comment.
Doesn't adding this change the current behavior? Before the option was not set which make add_library default to STATIC. With this line being added which basically runs this:
catkin/cmake/tools/libraries.cmake
Lines 11 to 13 in 964f1bf
the default behavior changes to SHARED.
There was a problem hiding this comment.
the BUILD_SHARED_LIBS is turned on in catkin/cmake/platform/windows.cmake, so it's ON by default (even on *nix). In addition, configure_shared_library_build_settings() is called by catkin/cmake/catkin_workspace.cmake, too. Which means BUILD_SHARED_LIBS is turned on by default in toplevel.cmake. I'm not sure if add_library is default to STATIC, from my observation, it's default to SHARED.
Do we have a consensus about the default value of this switch? We need to nail down the default value and then change the code to make it consistent.
There was a problem hiding this comment.
The toplevel.cmake / catkin_workspace is only being used by catkin_make. All other build tools (catkin_make_isolated, catkin-tools, colcon) don't use a workspace-level CMake file but invoke CMake for each package separately.
But I think you are right that BUILD_SHARED_LIBS is turned on by default. The reason is that both (one would already be sufficient) of the following files are included in the all.cmake file (platform/windows.cmake, tools/libraries.cmake). Both declare the option and ensure that it is initialized with on if not already set.
Can you clarify why explicitly calling configure_shared_library_build_settings() here is necessary for making nmake work on Windows?
There was a problem hiding this comment.
For Windows, all shared libraries are built into Dll files and need to specify the storage-class attributes to indicate if a function/data is exported (the dll provides the function/data) or imported (the dll consumes the function/data). We use macro to decide what attribute to use when compile a component (see https://github.com/ros/ros_comm/blob/melodic-devel/tools/rosbag_storage/include/rosbag/macros.h for example). The macro checks the flag ROS_BUILD_SHARED_LIBS to decide the storage-class attribute. configure_shared_library_build_settings() is where to decide whether to set the ROS_BUILD_SHARED_LIBS.
There was a problem hiding this comment.
Thanks for clarifying - I forgot that this is what we use ROS_BUILD_SHARED_LIBS for.
|
Thank you for the improvement! |
add missing part to handle use_nmake
update setup.bat template
skip replacing ':' with ';'