-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
losslesscut-bin: init at 3.33.1 #108512
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
losslesscut-bin: init at 3.33.1 #108512
Conversation
ee7308f
to
5e21add
Compare
5e21add
to
ac6d0a8
Compare
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.
Haven't looked too closely to the package definition, but building and running on Linux NixOS works. Cutting out a simple video also works.
I wonder; is there a reason as to not build the electron app from source?
Thanks for review and reply.
It would be wonderful to build from source, and that's what I have tried at first (#91683). Firstly, I personally have never succeeded in the source-build of any electron project. Secondly, the author of LosslessCut decided to bundle statically-build ffmpeg and ffprobe libreary instead of relying on the one provided by the package manager ar the environment, and the bundle will be downloaded during installation. We need to hack on the code to make it use the one downloaded by fetchurl or the one linked from the nixpkgs derivation. It would be great to get some advice on how to build them. |
a79fd6e
to
f756475
Compare
I understand. I've never built an electron package before, so I can't really give any guidance on that. IMO this can be merged (I don't have commit rights). Some MacOS / Windows (not sure if anyone's still on that) testing would be nice. |
f756475
to
99f1ae6
Compare
Just performed some modifications:
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
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.
Runs on macOS
a176c35
to
69cf681
Compare
Rebase onto recent Nixpkgs master and upgrade to 3.33.1 |
No idea if this is a bit overengineered. |
I think it could use some refactoring, but I would not say it's terribly complicated. |
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 would suggest to just merge this, but taking pkgs
as attribute is not very acceptable.
@@ -0,0 +1,31 @@ | |||
{ pkgs, stdenvNoCC, lib }: |
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.
{ pkgs, stdenvNoCC, lib }: | |
{ stdenv, callPackage, lib }: |
description = "The swiss army knife of lossless video/audio editing"; | ||
homepage = "https://mifi.no/losslesscut/"; | ||
license = licenses.mit; | ||
maintainers = with maintainers; [ ShamrockLee ]; |
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.
description = "The swiss army knife of lossless video/audio editing"; | |
homepage = "https://mifi.no/losslesscut/"; | |
license = licenses.mit; | |
maintainers = with maintainers; [ ShamrockLee ]; | |
description = "The swiss army knife of lossless video/audio editing"; | |
homepage = "https://mifi.no/losslesscut/"; | |
license = licenses.mit; | |
maintainers = with maintainers; [ ShamrockLee ]; |
Why not put these into the override below, get rid of callBin
maintainers = with maintainers; [ ShamrockLee ]; | ||
}; | ||
in ( | ||
if stdenvNoCC.isDarwin then passthru.dmg |
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.
if stdenvNoCC.isDarwin then passthru.dmg | |
if stdenvNoCC.isDarwin then callPackage ./dmg.nix { inherit version } |
and so on
Also, why do you call the package "lossless-cut" if its "losslesscut" |
It's just because the source repo is named "lossless-cut", and the manual suggests that the attribute be identical to the project name. (or do I misunderstand something?)
|
Add binary package for * Linux (AppImage) * Mac (dmg, x86_64 version) * Windows (zip) Use lib.meta as per NixOS#108938
69cf681
to
d7cd27d
Compare
Apply the suggestions and rename |
I'd suggest merging this, most or all comments are addressed and it's been open for a long time now. Subsequent updates can incorporate any minor improvements that are needed. |
Seems like calling the per-platform files in a let-expression makes them impossible to override (AFAIK). |
@ryneeverett Would it be better for the three of them be visible in all-packages.nix or should I put them in the |
@ShamrockLee I'm not the person to ask but I would think passthru would be acceptable. |
Add binary package for
Motivation for this change
If applied, LosslessCut -- a convenient video editor -- will be available in nixpkgs for Linux, Mac and Windows users.
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)