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
x265: fix build on aarch64 #100807
x265: fix build on aarch64 #100807
Conversation
Found an upstream issue: https://bitbucket.org/multicoreware/x265_git/issues/554 |
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.
Tested and working!
cc. @flokli for the big green button
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.
No green button, requested changes ;-)
@@ -34,9 +34,13 @@ let | |||
sha256 = "1jzgv2hxhcwmsdf6sbgyzm88a46dp09ll1fqj92g9vckvh9a7dsn"; | |||
}; | |||
|
|||
patches = stdenv.lib.optionals stdenv.hostPlatform.isAarch64 [ |
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.
patches shouldn't be conditional on a specific architecture - assuming the patch won't apply after a bump, this will lead to silent breakages.
If this patch doesn't break other arches, it should just be applied independent of architecture
@@ -34,9 +34,13 @@ let | |||
sha256 = "1jzgv2hxhcwmsdf6sbgyzm88a46dp09ll1fqj92g9vckvh9a7dsn"; | |||
}; | |||
|
|||
patches = stdenv.lib.optionals stdenv.hostPlatform.isAarch64 [ | |||
./0001-dynamicHDR10-update-CMakeLists.txt-for-aarch64-suppo.patch |
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.
Please don't bundle patches in nixpkgs, if they can be sent upstream, and fetchpatch
ed in here.
When doing so, please also add a comment pointing to the upstream PR.
Motivation for this change
Fixing aarch64 build of x265 after #98091.
I'll send the patch upstream when I figure out how to do so.
As for the high bit rate support, the header file for aarch64 assembly doesn't include the the prefixed versions (
x265_10bit
,x265_12bit
) from which I conclude it is not supported. Since it seems like it has to be explicitly implemented I made this a whitelist of justisx86_64
.Things done
sandbox
innix.conf
on non-NixOS linux)aarch64-linux
only)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)