-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
imgui: ship vcpkg' (cmake) distribution info #303682
Conversation
670d3fe
to
5021572
Compare
4ac25c6
to
0cbacb0
Compare
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/3795 |
cmakeRules = "${vcpkg.src}/ports/implot"; | ||
postPatch = '' | ||
cp "$cmakeRules"/CMakeLists.txt ./ |
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.
cmakeRules = "${vcpkg.src}/ports/implot"; | |
postPatch = '' | |
cp "$cmakeRules"/CMakeLists.txt ./ | |
postPatch = '' | |
cp "${vcpkg.src}/ports/implot"/CMakeLists.txt ./ |
or is this used as an env?
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.
This is so as to keep phases agnostic of the hashes. I see no good reason to render the hashes into the scripts
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.
I can't follow you.
Usually turning nix variables into bash variables is an anti pattern we try to avoid.
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.
Introducing the auxiliary variable like this gives the opportunity to implot.overrideAttrs (_: { cmakeRules = ./my-custom-cmake; })
which otherwise would require a careful surgery on the postPatch
. I've been following a similar pattern outside the tree and thought it wouldn't do harm to do the same here.
an anti pattern we try to avoid.
Yesssss setup-hooks are an anti-pattern, let's make nix evaluation faster and abolish setup hooks 😆
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.
You should be able to use finalAttrs to allow overrides like that, too.
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.
That's an option, but it's not really any better than eliminating the template rendering and treating postPatch
as a normal setup hook
EDIT: Note also, this would still require the auxiliary variable
0cbacb0
to
ea9d642
Compare
# CMake: link libGL from the "app" target | ||
(fetchpatch { | ||
url = "https://github.com/epezent/implot_demos/commit/6742e4202858eb85bd0d67ca5fa15a7a07e6b618.patch"; | ||
hash = "sha256-h4EJ9u1iHLYkGHgxSynskkuCGmY6mmvKdZSRwHJKerY="; | ||
}) |
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.
vcpkg' CMakeLists.txt for imgui doesn't target_link_libraries(... OpenGL::GL)
. I wasn't sure if it should, so for now I made the demo link libGL explicitly
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.
@SuperSandro2000 any concerns other than hooks-vs-finalAttrs? More generally, do you have any opinion on the decision to adopt 3rd party (vcpkg) distribution information?
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.
I'd like to merge tomorrow unless there are objections.
@SuperSandro2000 we haven't reached the agreement on the finalAttrs
matter but we can keep the question open and if we end up choosing against bash variables I'll just open a follow-up PR
48d0e4c
to
38a479a
Compare
3baa154
to
8b68642
Compare
Glfw may dlopen libwayland-client.so. This was observed when testing .#imgui.tests.demo
7298046
to
c74ce24
Compare
Well if there should be any objections they can be resolved in future PRs. RE: Ofborg
|
And I guess this PR does break imgui on darwin.... |
Well, "breaks" is maybe an overly strong way to put it, given that previously this derivation didn't actually "build" imgui. As I mentioned in the update, clients that prefer the "add_subdirectory" approach can still use I'll try to clean this up rn |
Opened #315917, it's just missing some frameworks. |
Description of changes
"Dear ImGui" is one instance of a project where upstream chooses to only distribute sources and headers without any metadata related to building or packaging (cmake "modules", pkg-config, ...). In practice, building and managing the dependencies of imgui, or including imgui in projects built by common build tools (cmake, meson, ...) are not trivial. One way to consume it, besides vendoring, is via the cmake rules maintained in a centralized repository like vcpkg (conan, ...). Presumably, it's even a quite common method.
A wider adoption of the vcpkg-like approach by upstream projects would be ideal for Nixpkgs purposes: it establishes a common interface and a dependency injection mechanism that help creating consistent package sets, fix points, modeling dependencies as independent derivations, etc. This common interface is in fact pure cmake and vcpkg-agnostic. Possibly worth a mention that vcpkg is licensed under MIT.
I suggest we try out the practice of distributing vcpkg' packaging info in situations where upstream doesn't provide any.
In this PR I adapt
imgui
and related packages to reusevcpkg
'CMakeLists.txt
instead of custom phases.Affected packages:
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.