Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

wentasah
Copy link
Contributor

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.

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 and others added 4 commits April 19, 2025 15:59
…t_vendor calls

This is needed at least for zenoh-cpp-vendor, which is being prepared
in lopsided98#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.
This is required since nix-eval-jobs version 2.26.0. Without this, we
don't find any package to update.
This makes not change for currently vendored packages, but it will
be needed for zenoh-cpp-vendor.
@wentasah wentasah force-pushed the extend-ament-vendor-autoupdate branch from 512b21b to 2913d26 Compare April 19, 2025 14:02
@wentasah
Copy link
Contributor Author

Fixed patching to build zenoh-cpp-vendor correctly.

@wentasah
Copy link
Contributor 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
…rences

The number of entries in vendored-source.json matches the actual
number of calls made during cmake execution. But the CMakeLists.txt
can contain multiple calls guarded with different if conditions, as is
the case with rviz-ogre-vendor. To be sure, that we patch the correct
call, we needed to patch all of them.

On the other hand, if the actual number of calls is greater than
one (as with zenoh-cpp-vendor), we patch them one-by-one, hoping that
there are no calls guarded by false if conditions.

This heuristic works for all packages that are currently patched with
patchAmentVendorGit.
@wentasah
Copy link
Contributor 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
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 .

''
sed -i '/VCS_TYPE \(git\|zip\|svn\|path\)/d' ${lib.escapeShellArg file}
'' + (
if length sourceInfos == 1 then
Copy link
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
Contributor 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
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
Contributor 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
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
Contributor 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.

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.

4 participants