Enable real pthreads for emscripten-wasm32 builds

vinca/distro.py

@@ -543,12 +543,26 @@ def _construct_raw_url_github(self, pkg_info):
         # Extract owner/repo
         owner_repo = raw_url_base.split("github.com/")[-1]
         # Use rev if available, otherwise fallback to tag
-        ref = pkg_info.get("rev") or pkg_info.get("tag")
+        rev = pkg_info.get("rev")
+        tag = pkg_info.get("tag")
         xml_name = pkg_info.get("package_xml_name", "package.xml")
         additional_folder = pkg_info.get("additional_folder", "")
         if additional_folder != "":
             additional_folder = additional_folder + "/"
-        raw_url = f"https://raw.githubusercontent.com/{owner_repo}/{ref}/{additional_folder}{xml_name}"
+        if rev:
+            # A commit hash is unambiguous as-is.
+            ref_path = rev
+        else:
+            # ros2-gbp release tags look like "release/jazzy/foo_pkg/1.2.3-1" --
+            # raw.githubusercontent.com's short <owner>/<repo>/<ref>/<path> form
+            # has to guess where a slash-containing ref ends and the path
+            # begins, and that guess is inconsistently cached across CDN edges:
+            # the same URL can 404 from some vantage points (including GitHub
+            # Actions runners) while resolving fine from others. The explicit
+            # refs/tags/<name> form removes the ambiguity and resolves
+            # reliably everywhere.
+            ref_path = f"refs/tags/{tag}"
+        raw_url = f"https://raw.githubusercontent.com/{owner_repo}/{ref_path}/{additional_folder}{xml_name}"
         return raw_url
 
     # format (checked against GitLab 19.x): https://gitlab.com/<NAMESPACE>/-/raw/<REV>/<PATH>

vinca/pinning.py

@@ -219,11 +219,69 @@ def _migration_name(name: str) -> str:
     return name
 
 
+def _existing_eol_comment_text(source: Any, index: int) -> Optional[str]:
+    """Return the plain text of a CommentedSeq item's trailing EOL comment, if any."""
+    ca = getattr(source, "ca", None)
+    if ca is None:
+        return None
+    entry = ca.items.get(index)
+    if not entry:
+        return None
+    token = entry[0]
+    if token is None:
+        return None
+    return str(token.value).lstrip("#").strip()
+
+
+def _flatten_v1_selectors(value: Any) -> Any:
+    """Convert v1-style `- if: COND then: [...]` list entries into the legacy
+    `- VALUE  # [COND]` comment-annotated form that rattler-build's variant
+    config loader actually evaluates lazily per target_platform (unlike the
+    v1 if/then/else mapping form, which it treats as an opaque literal value
+    rather than a selector -- confirmed via `Could not parse version spec
+    for variant key ...: invalid channel` / `multiple bracket sections not
+    allowed` errors when left unconverted).
+
+    Passthrough items (plain scalars, possibly already carrying their own
+    `# [selector]` EOL comment) must have that existing comment re-attached
+    at their new index -- ruamel stores comments keyed by list position on
+    the *source* CommentedSeq, not on the item itself, so a naive
+    `result.append(item)` into a freshly created CommentedSeq silently
+    drops it, turning a platform-scoped entry into an unconditional one.
+    """
+    if not isinstance(value, list):
+        return value
+    import ruamel.yaml.comments as _rc
+
+    result = _rc.CommentedSeq()
+    for old_index, item in enumerate(value):
+        if isinstance(item, Mapping) and "if" in item and "then" in item:
+            cond = str(item["if"])
+            for entry in item["then"]:
+                idx = len(result)
+                result.append(entry)
+                result.yaml_add_eol_comment(f"[{cond}]", idx)
+            else_branch = item.get("else")
+            if else_branch is not None:
+                not_cond = f"not ({cond})"
+                for entry in else_branch:
+                    idx = len(result)
+                    result.append(entry)
+                    result.yaml_add_eol_comment(f"[{not_cond}]", idx)
+        else:
+            idx = len(result)
+            result.append(item)
+            comment_text = _existing_eol_comment_text(value, old_index)
+            if comment_text:
+                result.yaml_add_eol_comment(comment_text, idx)
+    return result
+
+
 def _overlay(target: Any, source: Any) -> None:
     for key, value in source.items():
         if key == "migrator_ts" or str(key).startswith("__"):
             continue
-        target[key] = value
+        target[key] = _flatten_v1_selectors(value)
 
 
 def _migration_timestamp(payload: bytes) -> float:

vinca/templates/build_ament_cmake.sh.in

@@ -69,20 +69,96 @@ if [[ $target_platform =~ emscripten.* ]]; then
   echo "set(CMAKE_STRIP FALSE)  # used by default in pybind11 on .so modules">> $SRC_DIR/__vinca_shared_lib_patch.cmake
   echo "set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE BOTH)  # fixes an error where numpy header files are not found correctly">> $SRC_DIR/__vinca_shared_lib_patch.cmake
 
-  # if [ "${PKG_NAME}" == "ros-humble-examples-rclcpp-minimal-publisher" ] || [ "${PKG_NAME}" == "ros-humble-examples-rclcpp-minimal-subscriber" ] || [ "${PKG_NAME}" == "ros-humble-rclcpp-components" ]; then
-  #   echo "set(CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s USE_PTHREADS=0 -s DEMANGLE_SUPPORT=1 -s ALLOW_MEMORY_GROWTH=1 \")">> $SRC_DIR/__vinca_shared_lib_patch.cmake
-  #   echo "set(CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s USE_PTHREADS=0 -s DEMANGLE_SUPPORT=1 -s ALLOW_MEMORY_GROWTH=1 -sASYNCIFY -O3 -s ASYNCIFY_STACK_SIZE=24576 \")">> $SRC_DIR/__vinca_shared_lib_patch.cmake
-  #   echo "set(CMAKE_EXE_LINKER_FLAGS \"-sMAIN_MODULE=1 -sASSERTIONS=1 -fexceptions -lembind -sWASM_BIGINT -s USE_PTHREADS=0 -s DEMANGLE_SUPPORT=1 -sALLOW_MEMORY_GROWTH=1 -sASYNCIFY -O3 -s ASYNCIFY_STACK_SIZE=24576 -L$SRC_DIR/build -L$PREFIX/lib\")  # remove SIDE_MODULE from exe linker flags">> $SRC_DIR/__vinca_shared_lib_patch.cmake
-  # else
-    echo "set(CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s USE_PTHREADS=0 -s ALLOW_MEMORY_GROWTH=1 -s DEMANGLE_SUPPORT=1 \")">> $SRC_DIR/__vinca_shared_lib_patch.cmake
-    echo "set(CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s USE_PTHREADS=0 -s ALLOW_MEMORY_GROWTH=1 -s DEMANGLE_SUPPORT=1 \")">> $SRC_DIR/__vinca_shared_lib_patch.cmake
-    echo "set(CMAKE_EXE_LINKER_FLAGS \"-sMAIN_MODULE=1 -sASSERTIONS=1 -fexceptions -lembind -sWASM_BIGINT -s USE_PTHREADS=0 -sALLOW_MEMORY_GROWTH=1 -s DEMANGLE_SUPPORT=1 -L$SRC_DIR/build -L$PREFIX/lib\")  # remove SIDE_MODULE from exe linker flags">> $SRC_DIR/__vinca_shared_lib_patch.cmake
-  # fi
+  # Real pthreads (USE_PTHREADS=1) are required so that blocking waits
+  # (std::condition_variable / rmw wait sets) actually work: without a real
+  # OS-level thread, libc++'s condition_variable timed-wait never wakes up
+  # on its own, and code that spins/blocks the main thread (e.g. rclcpp's
+  # executor) hangs forever with no way for the browser's JS event loop
+  # (and thus wall-clock time) to ever advance underneath it. This must be
+  # consistent across every emscripten-wasm32 package: mixing a
+  # pthread-enabled module with a non-pthread one is a hard ABI-level
+  # mismatch ("memory import shared state mismatch") since a wasm module's
+  # shared-vs-non-shared linear memory is fixed at compile+link time.
+  #
+  # This flag has to be set at COMPILE time too (not just link time) for
+  # every translation unit -- it bakes in the wasm 'atomics'/'bulk-memory'
+  # features that the linker later requires when producing shared memory
+  # ("wasm-ld: error: --shared-memory is disallowed ... because it was not
+  # compiled with 'atomics' or 'bulk-memory' features"). add_compile_options
+  # here (via CMAKE_PROJECT_INCLUDE, included right after every project()
+  # call) applies it to every target compiled in every package.
+  echo "add_compile_options(\"SHELL: -s USE_PTHREADS=1\")">> $SRC_DIR/__vinca_shared_lib_patch.cmake
+
+  echo "set(CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s USE_PTHREADS=1 -s ALLOW_MEMORY_GROWTH=1 \")">> $SRC_DIR/__vinca_shared_lib_patch.cmake
+  echo "set(CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s USE_PTHREADS=1 -s ALLOW_MEMORY_GROWTH=1 \")">> $SRC_DIR/__vinca_shared_lib_patch.cmake
+  # CMake's MODULE library type (add_library(... MODULE), what
+  # pybind11_add_module() uses for Python C extensions e.g. rclpy's
+  # _rclpy_pybind11) is a distinct target type from SHARED and reads its
+  # own CMAKE_SHARED_MODULE_CREATE_*_FLAGS variables -- setting only the
+  # SHARED ones above left every MODULE-type .so linked without
+  # USE_PTHREADS=1, producing a non-shared-memory module that fails to
+  # load ("mismatch in shared state of memory") next to the rest of a
+  # pthreads build, even though its own object files were compiled with
+  # atomics support and looked fine individually.
+  echo "set(CMAKE_SHARED_MODULE_CREATE_C_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s USE_PTHREADS=1 -s ALLOW_MEMORY_GROWTH=1 \")">> $SRC_DIR/__vinca_shared_lib_patch.cmake
+  echo "set(CMAKE_SHARED_MODULE_CREATE_CXX_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s USE_PTHREADS=1 -s ALLOW_MEMORY_GROWTH=1 \")">> $SRC_DIR/__vinca_shared_lib_patch.cmake
+  # PTHREAD_POOL_SIZE and MAXIMUM_MEMORY are only meaningful on the final
+  # MAIN_MODULE executable link (they configure the Worker pool and the
+  # shared SharedArrayBuffer's reserved size respectively -- side modules
+  # don't have memory of their own, they use the main module's). Emscripten
+  # requires MAXIMUM_MEMORY to be set explicitly whenever
+  # ALLOW_MEMORY_GROWTH is combined with USE_PTHREADS, since a shared
+  # wasm memory's maximum size can't be left unbounded.
+  echo "set(CMAKE_EXE_LINKER_FLAGS \"-sMAIN_MODULE=1 -sASSERTIONS=1 -fexceptions -lembind -sWASM_BIGINT -s USE_PTHREADS=1 -s PTHREAD_POOL_SIZE=4 -sALLOW_MEMORY_GROWTH=1 -s MAXIMUM_MEMORY=1024MB -L$SRC_DIR/build -L$PREFIX/lib\")  # remove SIDE_MODULE from exe linker flags">> $SRC_DIR/__vinca_shared_lib_patch.cmake
+
+  # A message package's *Config.cmake only exports find_dependency() calls
+  # for what its own package.xml/CMakeLists.txt actually declares -- it has
+  # no idea VINCA_EMSCRIPTEN_STATIC_TYPESUPPORT_C/_CPP named an extra
+  # typesupport backend, so it never re-exports *that* dependency to ITS
+  # OWN consumers. A package that only uses one message package at a time
+  # never notices (it already found the backend itself while configuring
+  # its own rosidl_generate_interfaces() call), but one that find_package()s
+  # several message packages together hits "the target was not found ...
+  # A find_package call is missing for an IMPORTED target" the first time a
+  # downstream *Export.cmake references
+  # rosidl_typesupport_microxrcedds_c(pp)::rosidl_typesupport_microxrcedds_c(pp)
+  # without anyone upstream having found it first. Pre-finding it here (via
+  # CMAKE_PROJECT_INCLUDE, so it's already in every target's CMake
+  # namespace before that project's own find_package() calls run) covers
+  # every consumer uniformly instead of patching each one individually.
+  # A package that calls rosidl_generate_interfaces() itself (i.e. defines
+  # its own messages/services/actions) must NOT get this pre-find: that
+  # macro discovers available typesupport implementations itself and
+  # registers each one's ament_export_targets() call in a fixed relative
+  # order (each backend's generator target before its own typesupport
+  # target). Pre-finding the override backend here makes it "already a
+  # target" before that macro runs, which -- empirically confirmed by
+  # inspecting the resulting package's own ament_cmake_export_targets-extras.cmake
+  # -- causes THIS package's typesupport entry to jump to the front of its
+  # own _exported_targets list, ahead of the generator target its own
+  # Export.cmake requires (INTERFACE_LINK_LIBRARIES references
+  # <pkg>::<pkg>__rosidl_generator_c(pp)). That makes every downstream
+  # find_package(<this package>) fail with "referenced, but are missing:
+  # <pkg>::<pkg>__rosidl_generator_c(pp)" -- reproducible regardless of
+  # whether the C or C++ (or both) override is set. Without any pre-find,
+  # rosidl_generate_interfaces() discovers the same override backend on its
+  # own (via STATIC_ROSIDL_TYPESUPPORT_C/_CPP below) in the correct order,
+  # so skipping it here loses nothing for this package's own typesupport
+  # selection -- it only loses the (here unneeded) benefit described above
+  # of pre-registering the backend for *consumers* of this package.
+  if ! grep -q "rosidl_generate_interfaces(" "$SRC_DIR/$PKG_NAME"/src/work/CMakeLists.txt 2>/dev/null; then
+    if [ -n "${VINCA_EMSCRIPTEN_STATIC_TYPESUPPORT_C:-}" ]; then
+      echo "find_package(${VINCA_EMSCRIPTEN_STATIC_TYPESUPPORT_C} QUIET)">> $SRC_DIR/__vinca_shared_lib_patch.cmake
+    fi
+    if [ -n "${VINCA_EMSCRIPTEN_STATIC_TYPESUPPORT_CPP:-}" ]; then
+      echo "find_package(${VINCA_EMSCRIPTEN_STATIC_TYPESUPPORT_CPP} QUIET)">> $SRC_DIR/__vinca_shared_lib_patch.cmake
+    fi
+  fi
 
   export BUILD_TYPE="Debug"
   export EXTRA_CMAKE_ARGS=" \
       -DPYTHON_SOABI="cpython-${ROS_PYTHON_VERSION//./}-wasm32-emscripten" \
-      -DRMW_IMPLEMENTATION=rmw_wasm_cpp \
+      -DRMW_IMPLEMENTATION=${VINCA_EMSCRIPTEN_RMW_IMPLEMENTATION:-rmw_wasm_cpp} \
       -DCMAKE_FIND_ROOT_PATH=$PREFIX \
       -DCMAKE_POSITION_INDEPENDENT_CODE=TRUE \
       -DCMAKE_PROJECT_INCLUDE=$SRC_DIR/__vinca_shared_lib_patch.cmake \
@@ -92,8 +168,8 @@ if [[ $target_platform =~ emscripten.* ]]; then
   export CMAKE_GEN="emcmake cmake"
   export CMAKE_BLD="cmake"
 
-  export STATIC_ROSIDL_TYPESUPPORT_C=rosidl_typesupport_introspection_c
-  export STATIC_ROSIDL_TYPESUPPORT_CPP=rosidl_typesupport_introspection_cpp
+  export STATIC_ROSIDL_TYPESUPPORT_C=${VINCA_EMSCRIPTEN_STATIC_TYPESUPPORT_C:-rosidl_typesupport_introspection_c}
+  export STATIC_ROSIDL_TYPESUPPORT_CPP=${VINCA_EMSCRIPTEN_STATIC_TYPESUPPORT_CPP:-rosidl_typesupport_introspection_cpp}
 else
   export BUILD_TYPE="Release"
   export CMAKE_GEN="cmake"

vinca/test_github_raw_url.py

@@ -0,0 +1,59 @@
+from typing import Any
+
+from vinca.distro import Distro
+
+
+def _distro() -> Any:
+    return Distro.__new__(Distro)
+
+
+def test_tag_ref_uses_explicit_refs_tags_prefix():
+    # ros2-gbp release tags look like "release/jazzy/foo_pkg/1.2.3-1" -- the
+    # short <owner>/<repo>/<ref>/<path> raw.githubusercontent.com form has to
+    # guess where a slash-containing ref ends and the path begins, and that
+    # guess is inconsistently cached across CDN edges (the same URL 404s from
+    # some vantage points, including GitHub Actions runners, while resolving
+    # fine from others). The explicit refs/tags/<name> form is unambiguous.
+    pkg_info = {
+        "url": "https://github.com/ros2-gbp/ros2_control-release.git",
+        "tag": "release/jazzy/controller_interface/4.47.0-1",
+    }
+
+    url = _distro()._construct_raw_url_github(pkg_info)
+
+    assert url == (
+        "https://raw.githubusercontent.com/ros2-gbp/ros2_control-release/"
+        "refs/tags/release/jazzy/controller_interface/4.47.0-1/package.xml"
+    )
+
+
+def test_rev_ref_is_used_as_is():
+    # A commit hash is already unambiguous -- it must not get the refs/tags/
+    # prefix, since it isn't a tag name.
+    pkg_info = {
+        "url": "https://github.com/ros2-gbp/ros2_control-release.git",
+        "rev": "abc123def456",
+    }
+
+    url = _distro()._construct_raw_url_github(pkg_info)
+
+    assert url == (
+        "https://raw.githubusercontent.com/ros2-gbp/ros2_control-release/"
+        "abc123def456/package.xml"
+    )
+
+
+def test_tag_ref_with_additional_folder_and_custom_xml_name():
+    pkg_info = {
+        "url": "https://github.com/example/some-release.git",
+        "tag": "release/rolling/some_pkg/1.0.0-1",
+        "additional_folder": "some_pkg",
+        "package_xml_name": "package.xml",
+    }
+
+    url = _distro()._construct_raw_url_github(pkg_info)
+
+    assert url == (
+        "https://raw.githubusercontent.com/example/some-release/"
+        "refs/tags/release/rolling/some_pkg/1.0.0-1/some_pkg/package.xml"
+    )

vinca/test_snapshot_metadata.py

@@ -61,9 +61,11 @@ def make_snapshot_distro(monkeypatch):
     distro._distro = Mock()
     snapshot_xml_by_url = {
         "https://raw.githubusercontent.com/example/snapshot-package-release/"
-        "release/rolling/snapshot_package/1.0.0-1/package.xml": (SNAPSHOT_PACKAGE_XML),
+        "refs/tags/release/rolling/snapshot_package/1.0.0-1/package.xml": (
+            SNAPSHOT_PACKAGE_XML
+        ),
         "https://raw.githubusercontent.com/example/snapshot-dependency-release/"
-        "release/rolling/snapshot_dependency/1.0.0-1/package.xml": (
+        "refs/tags/release/rolling/snapshot_dependency/1.0.0-1/package.xml": (
             SNAPSHOT_DEPENDENCY_XML
         ),
     }