-
-
Notifications
You must be signed in to change notification settings - Fork 17.5k
gz-utils: init at 3.1.1 #401219
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
gz-utils: init at 3.1.1 #401219
Conversation
|
I'm delighted as punch. I'm so close to building and running gazebosim with |
bengsparks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository contains tests; you should build these too.
They also contain a test/gtest_vendor, which you should remove from the tree, and prefer gtest from nixpkgs instead.
jopejoe1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository contains tests; you should build these too. They also contain a
test/gtest_vendor, which you should remove from the tree, and prefergtestfrom nixpkgs instead.
Just doing a doCheck = true; should be enough to run the tests, but I don't see that as something that would block this pr and can be done at a later time.
|
|
||
| nativeBuildInputs = [ | ||
| cmake | ||
| gz-cmake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in buildInputs or in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gz-cmake is a collection of cmake modules, so I imagine this is correct.
|
I'd frankly prefer if this package were merged with tests enabled; something like this tends to slip through the cracks 😄 |
I guess we can do it within this PR, can you give some directions on how to enable the tests? |
|
Sure, give me a few minutes. |
| nativeBuildInputs = [ | ||
| cmake | ||
| gz-cmake | ||
| spdlog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spdlog should be under buildInputs
| gz-cmake | ||
| spdlog | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I don't think we want to use the vendored cli11, so
buildInputs = [ cli11 spdlog ];
cmakeFlags = [
(lib.cmakeBool "GZ_UTILS_VENDOR_CLI11" false)
];|
Let's hold off on testing for a second; it's not entirely trivial, seeing as |
|
Thank you all ! |
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.