opencore-amr init at 0.1.5 #28034
opencore-amr init at 0.1.5 #28034
Conversation
| {stdenv, fetchurl}: | ||
|
|
||
| stdenv.mkDerivation { | ||
| name = "opencore-amr"; |
There was a problem hiding this comment.
The name should contain the version in it. Usually something like
let
version = "0.1.5";
in
stdenv.mkDerivation {
name = "opencore-amr-${version}";
would be done, also adding the version to the url component in the same way.
| @@ -0,0 +1,16 @@ | |||
| {stdenv, fetchurl}: | |||
There was a problem hiding this comment.
A nitpick, but spaces are the preferred style: { stdenv, fetchurl }
| sha256 = "0hfk9khz3by0119h3jdwgdfd7jgkdbzxnmh1wssvylgnsnwnq01c"; | ||
| }; | ||
| meta = { | ||
| homepage = https://opencore-amr.sourceforge.io/; |
There was a problem hiding this comment.
Nitpick: Newlines are appreciated between }; and meta.
|
@infinisil I implemented the required changes. Feel free to check and request any other changes. |
pkgs/top-level/all-packages.nix
Outdated
| @@ -4682,6 +4682,8 @@ with pkgs; | |||
| openssl = null; | |||
| }; | |||
|
|
|||
| opencore-amr = callPackage ../development/libraries/libopencore { }; | |||
|
|
|||
There was a problem hiding this comment.
You should move this to the other libraries in all-packages.nix and sort in it alphabetically. Search for ### to find the Development / Libraries section.
Also you should call it either libopencore or opencore-amr. So either change the package path or change the attribute name. Probably changing the path makes more sense.
|
And when that's done this LGTM :D. Only thing left to do then is squash the commits into a single one |
|
@infinisil implemented your changes. Feel free to merge 🙂 |
|
Thanks! |
|
@kiloreux Now it's too late, but just so you know for your next PR: Infinisil was talking about squashing the commits to match the commit-policy here: https://nixos.org/nixpkgs/manual/#idm140737318169792 which keeps the history cleaner. (A single "opencore-amr: init at 0.1.5") |
Motivation for this change
Enable support for libopencore for use inside ffmpeg
https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/ffmpeg-full/default.nix#L158
Things done
Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers.
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)