Use cmake --parallel instead of hardcoded -j4#621
Conversation
|
I get this. But, given that Bazel assumes one action == one core per default, this change will choke Bazel. WDYT? |
|
All these packages are now included in BCR. We can simply move to a Bazel native solution and dump rules_foreign_cc. |
Compilation is CPU intensive, and overbooking CPU for short period of time (8 - 16 seconds here) during bazel build is generally ok. We can potentially set something like Do you prefer to add |
yeah, that's another option |
|
Think about a basic github CI machine with two cores. If we unleash the number of cores, then this could potentially be very slow overall. Not sure what's the way forward here where "everyone will be happy". BCR approach: 👍 |
|
ok. I will send a PR to BCR for iceoryx 2.0.6 |
|
Using BCR iceoryx isn't super straightforward due to some cyclonedds couplings. Below is the patch we're currently using. I'm not sure how easy it would be to make this work for Bzlmod and Workspace at the same time... diff --git a/MODULE.bazel b/MODULE.bazel
index 597d4d9..d25164a 100644
--- a/MODULE.bazel
+++ b/MODULE.bazel
@@ -6,6 +6,7 @@ bazel_dep(name = "boringssl", version = "0.20251124.0")
bazel_dep(name = "curl", version = "8.11.0.bcr.4")
bazel_dep(name = "eigen", version = "3.4.1.bcr.1")
bazel_dep(name = "fmt", version = "12.1.0")
+bazel_dep(name = "iceoryx", version = "2.95.4")
bazel_dep(name = "libyaml", version = "0.2.5")
bazel_dep(name = "lz4", version = "1.10.0.bcr.1")
bazel_dep(name = "nlohmann_json", version = "3.12.0.bcr.1")
@@ -148,7 +149,6 @@ use_repo(
rules_ros2_non_module_deps,
"cyclonedds",
"foxglove_bridge",
- "iceoryx",
"osrf_pycommon",
"ros2",
"ros2_ament_index",
diff --git a/repositories/patches/cyclonedds_shm.patch b/repositories/patches/cyclonedds_shm.patch
new file mode 100644
index 0000000..bf9e27d
--- /dev/null
+++ b/repositories/patches/cyclonedds_shm.patch
@@ -0,0 +1,49 @@
+diff --git src/CMakeLists.txt src/CMakeLists.txt
+index a0edd71a..74bee0ba 100644
+--- src/CMakeLists.txt
++++ src/CMakeLists.txt
+@@ -14,6 +14,9 @@ if(NOT ${PROJECT_NAME} STREQUAL "CycloneDDS")
+ message(FATAL_ERROR "Top-level CMakeLists.txt was moved to the top-level directory. Please run cmake on ${dir} instead of ${CMAKE_CURRENT_LIST_DIR}")
+ endif()
+
++include_directories($ENV{EXT_BUILD_DEPS}/include)
++link_directories($ENV{EXT_BUILD_DEPS}/lib)
++
+ function(PREPEND var prefix)
+ set(listVar "")
+ foreach(f ${ARGN})
+@@ -48,21 +51,6 @@ else()
+ set(ENABLE_SHM "AUTO" CACHE STRING "Enable shared memory support")
+ endif()
+
+-set_property(CACHE ENABLE_SHM PROPERTY STRINGS ON OFF AUTO)
+-if(ENABLE_SHM)
+- if(NOT ENABLE_SHM STREQUAL "AUTO")
+- set(iceoryx_required REQUIRED)
+- else()
+- set(iceoryx_required QUIET)
+- endif()
+- find_package(iceoryx_binding_c ${iceoryx_required})
+- set(ENABLE_SHM ${iceoryx_binding_c_FOUND} CACHE STRING "" FORCE)
+- if(ENABLE_SHM AND APPLE)
+- get_filename_component(iceoryx_libdir "${ICEORYX_LIB}" DIRECTORY)
+- set(CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_RPATH};${iceoryx_libdir}")
+- endif()
+-endif()
+-
+ # ones that linger in the sources
+ # - DDS_HAS_BANDWIDTH_LIMITING
+ # - DDS_HAS_NETWORK_CHANNELS
+diff --git src/core/CMakeLists.txt src/core/CMakeLists.txt
+index 0f7a713c..f7e258f8 100644
+--- src/core/CMakeLists.txt
++++ src/core/CMakeLists.txt
+@@ -59,7 +59,7 @@ if(ENABLE_SECURITY)
+ endif()
+
+ if(ENABLE_SHM)
+- target_link_libraries(ddsc PRIVATE iceoryx_binding_c::iceoryx_binding_c)
++ target_link_libraries(ddsc PRIVATE iceoryx_binding_c)
+ endif()
+
+ include(ddsi/CMakeLists.txt)
diff --git a/repositories/patches/cyclonedds_tsan.patch b/repositories/patches/cyclonedds_tsan.patch
new file mode 100644
index 0000000..96fb172
--- /dev/null
+++ b/repositories/patches/cyclonedds_tsan.patch
@@ -0,0 +1,13 @@
+diff --git src/ddsrt/include/dds/ddsrt/attributes.h src/ddsrt/include/dds/ddsrt/attributes.h
+index a307d24..d43f678 100644
+--- src/ddsrt/include/dds/ddsrt/attributes.h
++++ src/ddsrt/include/dds/ddsrt/attributes.h
+@@ -130,7 +130,7 @@
+ #endif
+
+ #if defined(__has_feature)
+-# define ddsrt_has_feature_thread_sanitizer __has_feature(thread_sanitizer)
++# define ddsrt_has_feature_thread_sanitizer 0
+ #else
+ # define ddsrt_has_feature_thread_sanitizer 0
+ #endif
diff --git a/repositories/repositories.bzl b/repositories/repositories.bzl
index 60efc10..d5bba01 100644
--- a/repositories/repositories.bzl
+++ b/repositories/repositories.bzl
@@ -96,6 +96,15 @@ def ros2_workspace_repositories():
url = "https://github.com/google/googletest/archive/refs/tags/v1.17.0.tar.gz",
)
+ maybe(
+ http_archive,
+ name = "iceoryx",
+ strip_prefix = "iceoryx-2.0.5",
+ build_file = "@com_github_mvukov_rules_ros2//repositories:iceoryx.BUILD.bazel",
+ sha256 = "bf6de70e3edee71223f993a29bff5e61af95ce4871104929d8bd1729f544bafb",
+ url = "https://github.com/eclipse-iceoryx/iceoryx/archive/refs/tags/v2.0.5.tar.gz",
+ )
+
maybe(
http_archive,
name = "tinyxml2",
diff --git a/repositories/ros2_repositories_impl.bzl b/repositories/ros2_repositories_impl.bzl
index b6db132..3d19cb4 100644
--- a/repositories/ros2_repositories_impl.bzl
+++ b/repositories/ros2_repositories_impl.bzl
@@ -37,6 +37,10 @@ def ros2_repositories_impl():
sha256 = "ec3ec898c52b02f939a969cd1a276e219420e5e8419b21cea276db35b4821848",
strip_prefix = "cyclonedds-0.10.5",
url = "https://github.com/eclipse-cyclonedds/cyclonedds/archive/refs/tags/0.10.5.tar.gz",
+ patches = [
+ "@com_github_mvukov_rules_ros2//repositories/patches:cyclonedds_shm.patch",
+ "@com_github_mvukov_rules_ros2//repositories/patches:cyclonedds_tsan.patch",
+ ],
)
maybe(
@@ -50,15 +54,6 @@ def ros2_repositories_impl():
url = "https://github.com/ros2/geometry2/archive/refs/tags/0.25.15.tar.gz",
)
- maybe(
- http_archive,
- name = "iceoryx",
- strip_prefix = "iceoryx-2.0.5",
- build_file = "@com_github_mvukov_rules_ros2//repositories:iceoryx.BUILD.bazel",
- sha256 = "bf6de70e3edee71223f993a29bff5e61af95ce4871104929d8bd1729f544bafb",
- url = "https://github.com/eclipse-iceoryx/iceoryx/archive/refs/tags/v2.0.5.tar.gz",
- )
-
maybe(
http_archive,
name = "ros2_image_common", |
Several cmake builds pass
-- -j4to the underlying build tool (Ninja), capping parallelism at 4 jobs regardless of how many CPUs are available. On machines with many cores this is a significant bottleneck.This PR replaces them with cmake --parallel (introduced in CMake 3.12), which auto-detects available cores and passes the appropriate flag to the underlying build tool without hardcoding a limit.
Measured impact
Profiled with bazel build --profile on a 96-core Linux machine. Both iceoryx and cyclonedds were previously bottlenecked at 4/96 cores:
On machines with fewer cores the change is a no-op — cmake --parallel without a count uses a reasonable default that scales down
appropriately.
CMake version requirement
--parallel was added in CMake 3.12 (released June 2018). Both iceoryx and cyclonedds already require CMake versions well above that threshold.