gz-tools: init at 2.0.3#432725
Conversation
yzhou216
left a comment
There was a problem hiding this comment.
$ ./result/bin/gz
I cannot find any available 'gz' command:
* Did you install any Gazebo library?
* Did you set the GZ_CONFIG_PATH environment variable?
E.g.: export GZ_CONFIG_PATH=$HOME/local/share/gzI guess GZ_CONFIG_PATH needs to be set?
gc does have a cli tool? I only build them as librarys |
Yes. That's why I used |
Currently all gc packages are built as librarys. They aren't usable as cli tool. |
Then you should probably remove the executable in |
Then my next PR will be to activate the |
This comment was marked as outdated.
This comment was marked as outdated.
yzhou216
left a comment
There was a problem hiding this comment.
You might want to mark x86_64-darwin as badPlatform. Other than that, it looks good. Please squash the last commit when you get a chance.
|
I've just tried rebasing the gz-tools branch over the gz-make branch using the activate CLI tool, and it works too. |
|
What did has failed, in particular? I can't reproduce your failure and would like to solve the bug. |
https://github.com/yzhou216/nixpkgs-review-gha/actions/runs/16884488306/job/47828535150#step:6:611 |
|
I think, I see, what's the error is. The current |
Maybe, gz-cmake doesn't provide a mainprogram? error: builder for '/nix/store/kmzbxb1y1vympkhg6hqaxzbm7py3nr7y-gz-cmake-4.2.0.drv' failed with exit code 2;
last 25 log lines:
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/GzSanitizers.cmake
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/GzSetCompilerFlags.cmake
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/GzStringAppend.cmake
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/GzUtils.cmake
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/JoinPaths.cmake
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/Export.hh.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/cmake_uninstall.cmake.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/cpack_options.cmake.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/gz-all-config.cmake.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/gz-component-config.cmake.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/gz-config.cmake.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/gz_auto_headers.hh.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/upload_doc.sh.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/version_info.json.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/pkgconfig/gz-component.pc.in
> -- Installing: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/cmake/gz-cmake4/cmake4/pkgconfig/gz.pc.in
> Running phase: fixupPhase
> shrinking RPATHs of ELF executables and libraries in /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0
> checking for references to /build/ in /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0...
> patching script interpreter paths in /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0
> /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/share/gz/gz-cmake4/tools/doc_check.sh: interpreter directive changed from "#!/bin/sh" to "/nix/store/gkwbw9nzbkbz298njbn3577zmrnglbbi-bash-5.3p0/bin/sh"
> stripping (with command strip and flags -S -p) in /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/lib
> Running phase: installCheckPhase
> Executing versionCheckPhase
> versionCheckHook: /nix/store/w635qr3zqsmkjrg5lbnq3maw87zfn04y-gz-cmake-4.2.0/bin/gz was not found, or is not an executable
For full logs, run:
nix log /nix/store/kmzbxb1y1vympkhg6hqaxzbm7py3nr7y-gz-cmake-4.2.0.drv |
I don't think it does. https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/gz/gz-cmake/package.nix#L53 |
|
|
short reminder: Currently this pr is based on #432783, so you can't merge this pr, because this would automatically merge the commits from the other pr! |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5803 |
yzhou216
left a comment
There was a problem hiding this comment.
Could you please use present tense verbs in the commit messages?
done! |
|
I think, it's now mergable, because this pr don't depends on other pr's anymore |
| coreutils # For env | ||
| ]; | ||
|
|
||
| buildInputs = [ gz-cmake ]; |
There was a problem hiding this comment.
ruby should be added to buildInputs and nativeCheckInputs as gz-tools ships a Ruby dispatcher (src/cmd/cmdtools.rb.in) that the gz binary invokes to route subcommands into each Gazebo library's CLI plugin. Without ruby in buildInputs, the binary is installed but every subcommand (gz topic, gz log, gz sim, ...) fails at runtime once downstream libraries are added.
You can see my buildInputs/nativeCheckInputs.
| repo = "gz-tools"; | ||
| tag = "gz-tools${lib.versions.major finalAttrs.version}_${finalAttrs.version}"; | ||
| hash = "sha256-xMFJylj7OnDc7zVWiI4a/mvNpu9scz83F3bGopCt8l8="; | ||
| }; |
There was a problem hiding this comment.
Worth adding patches/pr-174.patch that I have in my implementation at these lines. I've opened a PR upstream in gazebosim/gz-tools#174.
gz-tools encodes a non-relative lookup for libbackward that fails inside the Nix store once LD_LIBRARY_PATH/DYLD_LIBRARY_PATH aren't globally populated.
I found that without it, any error path in gz sim segfaults inside backward-cpp instead of printing a stack trace. I found this happened with gz-sensors plugin-load failures.
| buildInputs = [ gz-cmake ]; | ||
|
|
||
| # Disable automatic shebang patching since we'll do it manually | ||
| dontPatchShebangs = true; |
There was a problem hiding this comment.
Is there a reason you are patching shebangs manually? I found it wasn't required.
| installCheckPhase = '' | ||
| # Verify the binary exists and can be executed | ||
| if [ -x "$out/bin/gz" ]; then | ||
| "$out/bin/gz" --help >/dev/null || true |
There was a problem hiding this comment.
If you add || true failures will go unnoticed.
It could be worth replacing installCheckPhase with passthru.tests.version = testers.testVersion which runs as a separate derivation and is the nixpkgs standard pattern. You can see what I have done.
|
|
||
| passthru = { | ||
| updateScript = nix-update-script { | ||
| attrPath = "gz-tools"; |
There was a problem hiding this comment.
Upstream tags look like gz-tools2_2.0.3 so you will need to use a regex so nix-update doesn't pick up unrelated refs. Suggest:
updateScript = nix-update-script {
extraArgs = [ "--version-regex=gz-tools${lib.versions.major version}_([\\d\\.]+)" ];
};| meta = { | ||
| description = "Gazebo Sim tools collection"; | ||
| homepage = "https://gazebosim.org/"; | ||
| downloadPage = "https://github.com/gazebosim/gz-tools"; |
There was a problem hiding this comment.
Could be worth adding changelog = "https://github.com/gazebosim/gz-tools/blob/${finalAttrs.src.tag}/Changelog.md";
| downloadPage = "https://github.com/gazebosim/gz-tools"; | ||
| license = lib.licenses.asl20; | ||
| maintainers = with lib.maintainers; [ guelakais ]; | ||
| platforms = lib.platforms.all; |
There was a problem hiding this comment.
It could be worth narrowing this asplatforms = lib.platforms.all includes mingw, wasi, and other platforms that have never been built. Perhaps use lib.platforms.linux ++ lib.platforms.darwin.
| hash = "sha256-xMFJylj7OnDc7zVWiI4a/mvNpu9scz83F3bGopCt8l8="; | ||
| }; | ||
|
|
||
| strictDeps = true; |
There was a problem hiding this comment.
Since the nixpkgs-vet was last run against this PR, the requirement for new packet to include __structuredAttrs = true has been added. Suggest adding it to your implementation. It helps catch dependency leaks.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.