diff --git a/vinca/distro.py b/vinca/distro.py index c56010a..1b23bb0 100644 --- a/vinca/distro.py +++ b/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 /// 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/ 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//-/raw// diff --git a/vinca/pinning.py b/vinca/pinning.py index 19313f4..f42effd 100644 --- a/vinca/pinning.py +++ b/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: diff --git a/vinca/templates/build_ament_cmake.sh.in b/vinca/templates/build_ament_cmake.sh.in index cc25a25..e3197d7 100644 --- a/vinca/templates/build_ament_cmake.sh.in +++ b/vinca/templates/build_ament_cmake.sh.in @@ -69,20 +69,98 @@ 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 + # No real pthreads here (deliberately -- see history below). Instead, + # Asyncify lets the one genuinely blocking call in this stack (rmw_wait, + # which polls zenoh-pico's WS transport) cooperatively yield back to the + # browser's JS event loop instead of really blocking -- that's enough for + # wall-clock time (and incoming WebSocket messages) to advance while a + # "blocking" wait is outstanding, without needing an OS-level thread. + # + # Real pthreads (USE_PTHREADS=1) were tried instead of this and reverted. + # They do make a blocking std::condition_variable/rmw wait set actually + # wake up, but they require wasm --shared-memory, which is viral: every + # module dlopen'd into an eagerly-linked host must also be pthreads/ + # shared-memory or linking fails ("mismatch in shared state of memory"), + # and worse, a genuinely blocking wait on a thread that also needs to + # service a message loop (e.g. JupyterLite's xeus-python kernel) just + # deadlocks outright -- there's nothing left free to deliver the wakeup. + # Asyncify avoids needing real concurrency at all for this. + # + # Asyncify is also flatly incompatible with wasm's native + # exception-handling proposal: binaryen's Asyncify pass hard-crashes + # ("UNREACHABLE executed ... Asyncify.cpp") on any object file compiled + # with wasm EH instructions, rather than just producing a slower build. + # The emscripten-forge toolchain package's own activation script + # (etc/conda/activate.d/activate_emscripten_emscripten-wasm32.sh) sets + # EMCC_CFLAGS="... -sSUPPORT_LONGJMP=wasm -fwasm-exceptions" globally, so + # *every* em++/emcc invocation gets wasm EH by default regardless of what + # CMake is told -- confirmed by tracing emcc.py: -fwasm-exceptions is the + # only thing that sets settings.WASM_EXCEPTIONS=1, and there's no + # -fno-wasm-exceptions counter-flag it recognizes to un-set it again. + # Overriding EMCC_CFLAGS here (dropping just the exception-handling part, + # keeping the rest of the toolchain's own base flags) is the only place + # that actually works -- back to this toolchain's other documented + # default, the older JS-based mechanism, which Asyncify does handle. + export EMCC_CFLAGS="${EM_FORGE_CFLAGS_BASE:-}" + echo "set(CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s ALLOW_MEMORY_GROWTH=1 -sASYNCIFY -s ASYNCIFY_STACK_SIZE=24576 \")">> $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 ALLOW_MEMORY_GROWTH=1 -sASYNCIFY -s ASYNCIFY_STACK_SIZE=24576 \")">> $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 -- keep it consistent + # with the SHARED flags above. + echo "set(CMAKE_SHARED_MODULE_CREATE_C_FLAGS \"-s ASSERTIONS=1 -s SIDE_MODULE=1 -sWASM_BIGINT -s ALLOW_MEMORY_GROWTH=1 -sASYNCIFY -s ASYNCIFY_STACK_SIZE=24576 \")">> $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 ALLOW_MEMORY_GROWTH=1 -sASYNCIFY -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 -sALLOW_MEMORY_GROWTH=1 -sASYNCIFY -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 + + # 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 + # ::__rosidl_generator_c(pp)). That makes every downstream + # find_package() fail with "referenced, but are missing: + # ::__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 +170,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" diff --git a/vinca/test_github_raw_url.py b/vinca/test_github_raw_url.py new file mode 100644 index 0000000..b4474aa --- /dev/null +++ b/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 /// 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/ 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" + ) diff --git a/vinca/test_snapshot_metadata.py b/vinca/test_snapshot_metadata.py index ca7eb56..e616572 100644 --- a/vinca/test_snapshot_metadata.py +++ b/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 ), }