Skip to content

Allow generating vendored-source.json for packages with multiple ament_vendor calls#620

Merged
wentasah merged 7 commits intolopsided98:developfrom
wentasah:extend-ament-vendor-autoupdate
Sep 8, 2025
Merged

Allow generating vendored-source.json for packages with multiple ament_vendor calls#620
wentasah merged 7 commits intolopsided98:developfrom
wentasah:extend-ament-vendor-autoupdate

Conversation

@wentasah
Copy link
Copy Markdown
Collaborator

This is needed at least for zenoh-cpp-vendor, which is being prepared in #558.

The format of vendored-source.json is changed from a single object to an array of objects. The order of objects corresponds to the order of ament_vendor calls in the patched CMakeLists.txt.

ament_cmake_vendor_packageConfig.cmake was updated to generate such a format automatically and patchAmentVendorGit was updated to understand that format and patch all call sites in the correct order.

The vendored-source.json files are regenerated automatically and there are no changes except addition of extra brackets.

@wentasah wentasah mentioned this pull request Apr 16, 2025
muellerbernd added a commit to muellerbernd/nix-ros-overlay that referenced this pull request Apr 16, 2025
@wentasah wentasah force-pushed the extend-ament-vendor-autoupdate branch from 2e602c0 to 512b21b Compare April 17, 2025 14:24
@wentasah wentasah marked this pull request as ready for review April 17, 2025 14:46
@wentasah wentasah mentioned this pull request Apr 17, 2025
@wentasah wentasah force-pushed the extend-ament-vendor-autoupdate branch from 512b21b to 2913d26 Compare April 19, 2025 14:02
@wentasah
Copy link
Copy Markdown
Collaborator Author

Fixed patching to build zenoh-cpp-vendor correctly.

@wentasah
Copy link
Copy Markdown
Collaborator Author

Even this is not good. This version fails with rviz-ogre-vendor, which contains several calls to ament_vendor, guarded with different if conditions. This version patches only the first occurrence, which is for WIN32. So we'll have to update the patching code to sometimes patch all occurrences (the old behavior) and sometimes one by one (this PR up to now).

@wentasah wentasah marked this pull request as draft April 19, 2025 18:52
@wentasah
Copy link
Copy Markdown
Collaborator Author

Now it works for all packages in the overlay as well as for zenoh-cpp-vendor. See https://hydra.iid.ciirc.cvut.cz/eval/3871?compare=lopsided98-develop&full=0.

@wentasah wentasah marked this pull request as ready for review April 19, 2025 20:33
@muellerbernd
Copy link
Copy Markdown
Contributor

muellerbernd commented Apr 20, 2025

Now it works for all packages in the overlay as well as for zenoh-cpp-vendor. See https://hydra.iid.ciirc.cvut.cz/eval/3871?compare=lopsided98-develop&full=0.

Thanks for your work. Now everything works fine. As soon as this gets merged I will update #558 .

Comment thread lib/default.nix Outdated
''
sed -i '/VCS_TYPE \(git\|zip\|svn\|path\)/d' ${lib.escapeShellArg file}
'' + (
if length sourceInfos == 1 then
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit brittle. Not that big a deal since we only have to handle a limited number of examples, but as an alternative, would it be feasible to match against the original VCS_URL value when patching?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not. CMakeFile authors are very creative and use variables in URLs like:

VCS_URL https://github.com/gazebosim/${GITHUB_NAME}.git

I was also looking at whether cmake can tell us the line number from where a macro (of a function) was called so that we can match based on that, but nothing like this seems to exist.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it could be done if there were some way to wrap the ament_vendor() call with our own macro that replaces the VCS_URL parameter.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem to solve here would be how to call the original ament_vendor implementation from the wrapper with the same name. I don't see any means for macro namespacing in cmake. Maybe, we can use Nix to patch the original implementation to have a different name and call that from the wrapper.

@muellerbernd
Copy link
Copy Markdown
Contributor

muellerbernd commented May 13, 2025

@wentasah I have updated my test branch for this PR to be up to date with the develop branch.
But now compiling zenoh-cpp-vendor does not work anymore.

error: builder for '/nix/store/3qd6hzm9f9lj028bqcvhi2xyrc0kndgc-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1.drv' failed with exit code 2;
       last 25 log lines:
       >     CMAKE_INSTALL_LOCALEDIR
       >     CMAKE_INSTALL_MANDIR
       >     CMAKE_INSTALL_NAME_DIR
       >     CMAKE_INSTALL_SBINDIR
       >     CMAKE_POLICY_DEFAULT_CMP0025
       >
       >
       > -- Build files have been written to: /build/rmw_zenoh-release-release-jazzy-zenoh_cpp_vendor-0.2.4-1/build
       > cmake: enabled parallel building
       > cmake: enabled parallel installing
       > Running phase: buildPhase
       > build flags: -j64 SHELL=/nix/store/58br4vk3q5akf4g8lx0pqzfhn47k3j8d-bash-5.2p37/bin/bash
       > [  6%] Creating directories for 'zenoh_c_vendor'
       > [ 12%] Performing download step for 'zenoh_c_vendor'
       > === ./zenoh_c_vendor (tar) ===
       > Downloaded tarball from 'file:///nix/store/5jnxif96jdmdqg0fnqjgjy023k2bcl7p-e6a1971139f405f7887bf5bb54f0efe402123032.tar' and unpacked it
       > [ 18%] No update_disconnected step for 'zenoh_c_vendor'
       > [ 25%] No patch_disconnected step for 'zenoh_c_vendor'
       > [ 31%] Performing configure step for 'zenoh_c_vendor'
       > loading initial cache file /build/rmw_zenoh-release-release-jazzy-zenoh_cpp_vendor-0.2.4-1/build/zenoh_c_vendor-config.cmake
       > CMake Error: The source directory "/build/rmw_zenoh-release-release-jazzy-zenoh_cpp_vendor-0.2.4-1/build/zenoh_c_vendor-prefix/src/zenoh_c_vendor" does not appear to contain CMakeLists.txt.
       > Specify --help for usage, or press the help button on the CMake GUI.
       > make[2]: *** [CMakeFiles/zenoh_c_vendor.dir/build.make:93: zenoh_c_vendor-prefix/src/zenoh_c_vendor-stamp/zenoh_c_vendor-configure] Error 1
       > make[1]: *** [CMakeFiles/Makefile2:156: CMakeFiles/zenoh_c_vendor.dir/all] Error 2
       > make: *** [Makefile:146: all] Error 2
       For full logs, run 'nix log /nix/store/3qd6hzm9f9lj028bqcvhi2xyrc0kndgc-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1.drv'.

Also updating of the vendor.json is not working:

============== Updating ros-jazzy-zenoh-cpp-vendor ==============
Running phase: unpackPhase
unpacking source archive /nix/store/0wl79ahilcmmvr45gfgcpsa9lhhn9pcq-0.2.4-1.tar.gz
source root is rmw_zenoh-release-release-jazzy-zenoh_cpp_vendor-0.2.4-1
setting SOURCE_DATE_EPOCH to timestamp 1745217032 of file "rmw_zenoh-release-release-jazzy-zenoh_cpp_vendor-0.2.4-1/package.xml"
Running phase: patchPhase
Running phase: updateAutotoolsGnuConfigScriptsPhase
Running phase: configurePhase
fixing cmake files...
cmake flags: -DCMAKE_FIND_USE_SYSTEM_PACKAGE_REGISTRY=OFF -DCMAKE_FIND_USE_PACKAGE_REGISTRY=OFF -DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=OFF -DCMAKE_INSTALL_LOCALEDIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/share/locale -DCMAKE_INSTALL_LIBEXECDIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/libexec -DCMAKE_INSTALL_LIBDIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/lib -DCMAKE_INSTALL_DOCDIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/share/doc/zenoh_cpp_vendor -DCMAKE_INSTALL_INFODIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/share/info -DCMAKE_INSTALL_MANDIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/share/man -DCMAKE_INSTALL_INCLUDEDIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/include -DCMAKE_INSTALL_SBINDIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/sbin -DCMAKE_INSTALL_BINDIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/bin -DCMAKE_INSTALL_NAME_DIR=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source/lib -DCMAKE_POLICY_DEFAULT_CMP0025=NEW -DCMAKE_FIND_FRAMEWORK=LAST -DCMAKE_STRIP=/nix/store/8v6k283dpbc0qkdq81nb6mrxrgcb10i1-gcc-wrapper-14-20241116/bin/strip -DCMAKE_RANLIB=/nix/store/8v6k283dpbc0qkdq81nb6mrxrgcb10i1-gcc-wrapper-14-20241116/bin/ranlib -DCMAKE_AR=/nix/store/8v6k283dpbc0qkdq81nb6mrxrgcb10i1-gcc-wrapper-14-20241116/bin/ar -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_INSTALL_PREFIX=/nix/store/mp61izang6dii67jlbfhjrifgbqg7y2c-ros-jazzy-zenoh-cpp-vendor-0.2.4-r1-source -DAMENT_CMAKE_ENVIRONMENT_PARENT_PREFIX_PATH_GENERATION:BOOL=OFF -DPython3_EXECUTABLE:FILEPATH=/nix/store/f2krmq3iv5nibcvn4rw7nrnrciqprdkh-python3-3.12.9/bin/python3.12 -DAMENT_CMAKE_ENVIRONMENT_PARENT_PREFIX_PATH_GENERATION:BOOL=OFF -DPython3_EXECUTABLE:FILEPATH=/nix/store/f2krmq3iv5nibcvn4rw7nrnrciqprdkh-python3-3.12.9/bin/python3.12
-- The C compiler identification is GNU 14.2.1
-- The CXX compiler identification is GNU 14.2.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /nix/store/8v6k283dpbc0qkdq81nb6mrxrgcb10i1-gcc-wrapper-14-20241116/bin/gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /nix/store/8v6k283dpbc0qkdq81nb6mrxrgcb10i1-gcc-wrapper-14-20241116/bin/g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ament_cmake: 2.5.4 (/nix/store/1bcaqlmsj8h66v9bqri12iwaha1b7n2s-ros-jazzy-ament-cmake-2.5.4-r1/share/ament_cmake/cmake)
-- Found Python3: /nix/store/f2krmq3iv5nibcvn4rw7nrnrciqprdkh-python3-3.12.9/bin/python3.12 (found version "3.12.9") found components: Interpreter
-- Found ament_cmake_vendor_package: Dummy version for Nix patching (/nix/store/dyclrz7njjz961z11sdp73x92bqhlqvf-ament_cmake_vendor_package_nix)
'/nix/store/gg1b5asslgwzmh0cznwxvpclwpg2706f-nix-prefetch-git/bin/nix-prefetch-git' '--url' 'https://github.com/eclipse-zenoh/zenoh-c.git' '--rev' 'ffa4bddc947f7ed6c0e3b4546205dd1b73e7df81' '--fetch-submodules'
'/nix/store/gpxsdrrd4x93fs75395vr2dfys1ki9mq-jq-1.7.1-bin/bin/jq' '[] + [{url: "https://github.com/eclipse-zenoh/zenoh-c.git", rev: "ffa4bddc947f7ed6c0e3b4546205dd1b73e7df81", hash: .hash}]'
Initialized empty Git repository in /tmp/nix-shell-260601-0/git-checkout-tmp-oaAQjBEX/zenoh-c-ffa4bdd/.git/
From https://github.com/eclipse-zenoh/zenoh-c
 * branch            ffa4bddc947f7ed6c0e3b4546205dd1b73e7df81 -> FETCH_HEAD
Switched to a new branch 'fetchgit'
removing `.git'...
/nix/store/gg1b5asslgwzmh0cznwxvpclwpg2706f-nix-prefetch-git/bin/.nix-prefetch-git-wrapped: line 468: nix-hash: command not found
CMake Error at /nix/store/dyclrz7njjz961z11sdp73x92bqhlqvf-ament_cmake_vendor_package_nix/ament_cmake_vendor_packageConfig.cmake:34 (execute_process):
  execute_process failed command indexes:

    1: "Child return code: 127"

Call Stack (most recent call first):
  CMakeLists.txt:37 (ament_vendor)


-- Configuring incomplete, errors occurred!

Can you try to reproduce that?

Edit:
If I use version 0.2.3 for zenoh-cpp-vendor compiling works fine but updating returns the same error.

@wentasah
Copy link
Copy Markdown
Collaborator Author

I'm reworking this based on above discussion with @lopsided98, but it takes me longer than expected due to other duties. I'd like to have it ready this week. I'll then look at zenoh-cpp-vendor because without it, we won't beady for Kilted release.

@wentasah wentasah marked this pull request as draft June 4, 2025 22:35
@wentasah wentasah force-pushed the extend-ament-vendor-autoupdate branch from 5dcb017 to 8643394 Compare June 4, 2025 22:39
@muellerbernd
Copy link
Copy Markdown
Contributor

@wentasah thanks for your work. I have updated my test branch to be up to date with this PR. for humble and jazzy zenoh-cpp-vendor do compile.

wentasah added 6 commits June 5, 2025 12:16
This is required since nix-eval-jobs version 2.26.0. Without this, we
don't find any package to update.
This makes no change for currently vendored packages, but it will be
needed for zenoh-cpp-vendor.
This is just for testing generation of vendored-source.json and
related code. To compile the package, more patching is needed.
…t_vendor calls

This is needed at least for zenoh-cpp-vendor, which is being prepared
in lopsided98#558, but it also improves the robustness of the whole patching
machanism.

The format of vendored-source.json is changed from a single object to
object of objects, one for each call to ament_vendor. The key in the
top-level object is the target name specified in the ament_vendor call
in CMakeLists.txt.

The principle of operation is slightly different from the previous
version. Instead of introducing "fake" ament_cmake_vendor_package just
for creation of vendored-source.json, we introduce a "wrapped" version
of ament_cmake_vendor_package, which is used both for creation of
vendored-source.json as well as for compiling the package. When
compiling the package, the wrapped version captures ament_vendor calls
and modifies the arguments to use the prefetched data from the Nix
store instead of downloading them directly.

patchAmentVendorGit was updated to understand the new format and to
invoke CMake with the wrapped package and information from
vendored-source.json.

The vendored-source.json files will be changed to the new format in
the next commit.
The commits and hashes do not change, only the structure and
indentation.
@wentasah wentasah force-pushed the extend-ament-vendor-autoupdate branch from 8643394 to 2c2c34d Compare June 5, 2025 11:36
@wentasah wentasah marked this pull request as ready for review June 5, 2025 11:37
@wentasah
Copy link
Copy Markdown
Collaborator Author

wentasah commented Jun 5, 2025

@lopsided98 This is an updated version, which wraps calls to ament_vendor and should be more reliable than previously used patching of CMakeLists.txt files with sed. For details see individual commit messages and comments in the code. It doesn't seem to cause any breakage compared to the develop branch.

@muellerbernd Thanks for quick testing.

This can be useful for troubleshooting potential problems.
@wentasah wentasah merged commit 6c906e8 into lopsided98:develop Sep 8, 2025
@wentasah wentasah deleted the extend-ament-vendor-autoupdate branch September 8, 2025 18:09
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.

3 participants