-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
vcpkg: 2025.02.14 -> 2025.04.09 #393542
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
vcpkg: 2025.02.14 -> 2025.04.09 #393542
Conversation
e6018dd to
baee9ab
Compare
baee9ab to
f326bd6
Compare
|
cc @SomeoneSerge for the imgui regression |
f326bd6 to
43f81d4
Compare
|
Automatic update generated by nixpkgs-update tools. This update was made based on information from https://github.com/microsoft/vcpkg/releases. meta.description for vcpkg is: C++ Library Manager for Windows, Linux, and macOS meta.homepage for vcpkg is: https://vcpkg.io/ Updates performed
To inspect upstream changesImpactChecks done
Rebuild report (if merged into master) (click to expand)Instructions to test this update (click to expand)Either download from the cache: (The nixpkgs-update cache is only trusted for this store-path realization.) Or, build yourself: Or: After you've downloaded or built it, look at the files and if there are any, run the binaries: Pre-merge build resultsWe have automatically built all packages that will get rebuilt due to This gives evidence on whether the upgrade will break dependent packages.
|
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.
So far this isn't going too great, it's not good to block updates like this...
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 don't think applying patches to a vcpkg ports is a scalable solution to provide a CMake package for imgui. I looked more in depth and it seem we're borrowing vcpkg's CMake script to build imgui while upstream imgui does not offer a CMake package. Vcpkg cmake scripts are not meant to be used outside of vcpkg at all, and looking now upstream made the right choice to refuse the patch.
Honestly, I think we should not copy the vcpkg cmake script just to patch it, and instead we should provide our own CMake build script and update it as we need it. It would also remove an artificial dependency on the vcpkg package while keeping the feature you implemented. What do you think?
The following is probably a bit outside of the scope of this conversation, but I think the imgui nix package shouldn't try to expose a CMake package when imgui upstream does not support it and is not documented by upstream. If vcpkg's script makes it easier to use, then I would say the package wanting to use imgui with vcpkg's CMake script should use vcpkg as documented, then use the CMake package as documented.
On another note, the imgui community already wrote a single file CMake build script and they also have an active PR ocornut/imgui#3027 to add CMake right into imgui itself. Both solution don't need vcpkg and I think they are more suitable than the vcpkg specific build script.
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.
Now there's a get-merge-commit error, I'll rebase on master
5c19a03 to
b724a88
Compare
|
instead we should provide our own CMake build script and update it as
we need it
Yes. One downside to this (the reason I went for introducing the
explicit dependency) is this way we won't receive signal about our
CMakeLists missing out on new features (like SDL3 in this case), instead
we'll just silently build w/o those features.
looking now upstream made the right choice to refuse the patch
Yes, except for the part they kept their CMakeLists non-relocatable,
which is a bit sus from CMake point of view AFAIK. They're in their
right though
I think the imgui nix package shouldn't try to expose a CMake package
when imgui upstream does not support it
In this case we might as well delete everything that depends on imgui.
Upstream has had a semi-political stance about "imgui not requiring any
build systems", which is just not how this works. The majority of the
projects I came across, that consume imgui, consume it via Vcpkg's CMake
interface, which I interpret as evidence that (1) a standardized
interface is needed, (2) this interface is pretty much the only available.
an active PR ocornut/imgui#3027 to add CMake
I'm aware of the PR, so far it seemed more or less stalled, but if it
ever comes through we'll obviously upgrade to it
|
I haven't notice imgui had no proper build system, so accounting for this I think the approach of providing our own script makes the most sense. Thanks for pointing this out.
Right now our users are missing both on vcpkg and imgui features. I think a good solution would be to still use vcpkg source in imgui, but not use the vcpkg package from nix to get them. This would mean a de coupling between vcpkg and imgui which would completely prevent situation like today, and still provide the feature imgui users depends on. One would need to update imgui in a PR to provide new feature, but IMO that would be better for users in general. I can open a PR to discuss this.
Sounds good. |
b724a88 to
13e18cd
Compare
|
Had to force-push again because of the formatting check.
In this case I'd introduce an extra fixpoint at the top-level, something like
I'd be very grateful! It's obviously my responsibility for introducing the dependency in the first place, but I'm afraid I'm kind of slow... |
Automatic update generated by nixpkgs-update tools. This update was made based on information from https://github.com/microsoft/vcpkg/releases.
meta.description for vcpkg is: C++ Library Manager for Windows, Linux, and macOS
meta.homepage for vcpkg is: https://vcpkg.io/
Updates performed
To inspect upstream changes
Impact
Checks done
passthru.tests, if any, passedRebuild report (if merged into master) (click to expand)
Instructions to test this update (click to expand)
Either download from the cache:
(The nixpkgs-update cache is only trusted for this store-path realization.)
For the cached download to work, your user must be in the
trusted-userslist or you can usesudosince root is effectively trusted.Or, build yourself:
Or:
After you've downloaded or built it, look at the files and if there are any, run the binaries:
Pre-merge build results
We have automatically built all packages that will get rebuilt due to
this change.
This gives evidence on whether the upgrade will break dependent packages.
Note sometimes packages show up as failed to build independent of the
change, simply because they are already broken on the target branch.
nixpkgs-reviewresultGenerated using
nixpkgs-review.Command:
nixpkgs-reviewx86_64-linux❌ 8 packages failed to build:
✅ 1 package built:
Maintainer pings
cc @Guekka @gracicot @h7x4 for testing.
Tip
As a maintainer, if your package is located under
pkgs/by-name/*, you can comment@NixOS/nixpkgs-merge-bot mergeto automatically merge this update using thenixpkgs-merge-bot.Add a 👍 reaction to pull requests you find important.