Skip to content
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.

Sign up for GitHub

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

Jump to bottom

nordlicht: init at 0.4.5 #70944

Closed
wants to merge 1 commit into from
Closed

nordlicht: init at 0.4.5 #70944

wants to merge 1 commit into from
+34 −0

Conversation

nixkoenner
Copy link

Motivation for this change

Because... why not? :-)
Build and manual test seems to work for me.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

Sorry, something went wrong.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 10, 2019
jonringer
jonringer reviewed Oct 11, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll also want to make an entry in pkgs/top-level/all-packages.nix so that it's able to be built like other packages

pkgs/tools/video/nordlicht/default.nix
@@ -0,0 +1,34 @@
{ stdenv, fetchFromGitHub, cmake, ffmpeg, popt, help2man }:
with cmake;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be necessary

Suggested change
with cmake;

pkgs/tools/video/nordlicht/default.nix
@@ -0,0 +1,34 @@
{ stdenv, fetchFromGitHub, cmake, ffmpeg, popt, help2man }:
with cmake;
stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move indention of the entire file over to the beginning please

Suggested change
stdenv.mkDerivation rec {
stdenv.mkDerivation rec {

pkgs/tools/video/nordlicht/default.nix
{ stdenv, fetchFromGitHub, cmake, ffmpeg, popt, help2man }:
with cmake;
stdenv.mkDerivation rec {
name = "nordlicht-${version}";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name = "nordlicht-${version}";
pname = "nordlicht";

pkgs/tools/video/nordlicht/default.nix
with cmake;
stdenv.mkDerivation rec {
name = "nordlicht-${version}";
version = "0.4.5-git.7ad3f008.4b1751bd";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use an official release, if you can't use it for some reason (necessary patches), then please reference the date of the commit:
unstable-YYYY-MM-DD

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at Github the revision you give is the 0.4.5 release.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if that's the case, then it sould be rev = version in the src attr set

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tag starts with v: rev = "v${version}

pkgs/tools/video/nordlicht/default.nix
stdenv.mkDerivation rec {
name = "nordlicht-${version}";
version = "0.4.5-git.7ad3f008.4b1751bd";
license = "GNU GPL v2 or later";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goes into meta section

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and should be license = stdenv.lib.licenses.gpl2Plus

pkgs/tools/video/nordlicht/default.nix
name = "nordlicht-${version}";
version = "0.4.5-git.7ad3f008.4b1751bd";
license = "GNU GPL v2 or later";
description = "creates colorful barcodes from video files";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also meta section

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also:

Suggested change
description = "creates colorful barcodes from video files";
description = "Tool for creating colorful barcodes from video files";

pkgs/tools/video/nordlicht/default.nix
rev = "7ad3f008afe803037b332822028faed64b1751bd";
sha256 = "14657zfzvq0iki8sn6rq789bgjr7wvvid9y2clfy25bi0a1gvnix";
};
buildInputs = [ cmake ffmpeg popt help2man ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmake should be in nativeBuildInputs (isn't needed at runtime)

Suggested change
buildInputs = [ cmake ffmpeg popt help2man ];
nativeBuildInputs = [ cmake ];
buildInputs = [ ffmpeg popt help2man ];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure help2man belongs to nativeBuildInputs too (only needed to generate man pages at build time)

pkgs/tools/video/nordlicht/default.nix
sha256 = "14657zfzvq0iki8sn6rq789bgjr7wvvid9y2clfy25bi0a1gvnix";
};
buildInputs = [ cmake ffmpeg popt help2man ];
configurePhase = ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of this should be unnecesary if cmake is listed as a nativeBuildInput, try removing it first

pkgs/tools/video/nordlicht/default.nix
done
done
'';
installFlags = [ "DESTDIR=$(out)" "CMAKE_INSTAL_PREFIX=$(out)" ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should be unnecessary to specify as well once cmake is a nativeBuildInput

markuskowa
markuskowa reviewed Oct 11, 2019
View reviewed changes
pkgs/tools/video/nordlicht/default.nix
src = fetchFromGitHub {
owner = "nordlicht";
repo = "nordlicht";
rev = "7ad3f008afe803037b332822028faed64b1751bd";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rev = "7ad3f008afe803037b332822028faed64b1751bd";
rev = "v${version}";

@markuskowa markuskowa changed the title added package nordlicht nordlicht: init at 0.4.5 Oct 11, 2019
@doronbehar
Copy link
Contributor

I'm closing as OP seems not intent to respond to review requests.

@doronbehar doronbehar closed this May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer

@c0bw3b c0bw3b

@markuskowa markuskowa

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nixkoenner @doronbehar @jonringer @c0bw3b @markuskowa