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
libplacebo: use upstreamed patch #108730
libplacebo: use upstreamed patch #108730
Conversation
cc @primeos |
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 see the advantage here tbh. Currently the commit is not yet merged into the upstream repository and that commit is likely GCed when/if you delete your branch (for the upstream MR).
In case upstream requests any changes (e.g. to make it work with older glslang versions as well) we could switch to the newer upstreamed patch but else this only documents it a bit better until the next release (but I guess we can also merge it for that reason if you want - should wait until the MR is merged though).
This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch) Result of 1 package failed to build and are new build failure:
18 packages built:
|
I understood the policy was to upstream patches and not have them live in nixpkgs tree as much as possible. But I could be wrong :) |
5b836da
to
f72e150
Compare
(feel free to close if I'm incorrect about the policy. No hard feeling) |
Yes, we try to upstream the patches to drop them after the next release / try to keep our downstream patches to a minimum. But if they're already in Nixpkgs I don't think there's any reason/recommendation to switch to fetchurl (that only makes sense if the patch was upstreamed in advance to link to the source and make the patch easier to verify; and for large patches to avoid bloating Nixpkgs). But tbh this is just how I view/perceive it so I could be wrong (but I hope it makes sense anyway). But since it seems like upstream might indeed want the patch to be backwards compatible we could switch to it once the MR is merged (e.g. for the unlikely case the the gslang update gets reverted in Nixpkgs). Feel free to update this PR or ping me once the MR is merged or to close this PR. |
sounds good |
It is the preferred way because we do not need to maintain the patch here.
That is not an issue. The patch is cached when it is merged and if it is merged we can always update the commit and the hash which is way easier than maintaining the patch in tree.
Yes, it is. |
Co-authored-by: Sandro <sandro.jaeckel@gmail.com> Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
f72e150
to
95b754c
Compare
@SuperSandro2000 please explain... We don't need to maintain our downstream patch, we can/must drop if after the next release anyway.
No. It might be cached once it is first build on Hydra but not immediately when merging. And I'm not sure if tarballs.nixos.org even caches patches as well (would make sense but I didn't check it).
We're not maintaining the patch here, it is already upstreamed (that is the important thing, not where we source the patch from).
Agreed. Maybe your first statements came of wrong then but in any case: This PR is basically a no-op. But since the MR is merged now and we already have this PR (I usually try not to reject PRs :o) we can just merge it and be done with it. @GrahamcOfBorg build libplacebo @baloo Anyway, sorry that this discussion went a bit in other directions, and thanks again for the quick fix and upstreaming the patch! :) (Edit: With the force-push I switched to the commit that is now in the master branch.) |
Motivation for this change
Fetch patch from upstream repository.
See #108727
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)