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.

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

nordlicht: init at 0.4.5 #70944

Closed
wants to merge 1 commit into from
Closed

Conversation

@nixkoenner
Copy link

nixkoenner commented Oct 10, 2019

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 @

Copy link
Contributor

jonringer left a comment

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

@@ -0,0 +1,34 @@
{ stdenv, fetchFromGitHub, cmake, ffmpeg, popt, help2man }:
with cmake;

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor

shouldn't be necessary

Suggested change
with cmake;
@@ -0,0 +1,34 @@
{ stdenv, fetchFromGitHub, cmake, ffmpeg, popt, help2man }:
with cmake;
stdenv.mkDerivation rec {

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor

move indention of the entire file over to the beginning please

Suggested change
stdenv.mkDerivation rec {
stdenv.mkDerivation rec {
{ stdenv, fetchFromGitHub, cmake, ffmpeg, popt, help2man }:
with cmake;
stdenv.mkDerivation rec {
name = "nordlicht-${version}";

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor
Suggested change
name = "nordlicht-${version}";
pname = "nordlicht";
with cmake;
stdenv.mkDerivation rec {
name = "nordlicht-${version}";
version = "0.4.5-git.7ad3f008.4b1751bd";

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor

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

This comment has been minimized.

Copy link
@markuskowa

markuskowa Oct 11, 2019

Member

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

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor

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

This comment has been minimized.

Copy link
@markuskowa

markuskowa Oct 11, 2019

Member

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

stdenv.mkDerivation rec {
name = "nordlicht-${version}";
version = "0.4.5-git.7ad3f008.4b1751bd";
license = "GNU GPL v2 or later";

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor

goes into meta section

This comment has been minimized.

Copy link
@markuskowa

markuskowa Oct 11, 2019

Member

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

name = "nordlicht-${version}";
version = "0.4.5-git.7ad3f008.4b1751bd";
license = "GNU GPL v2 or later";
description = "creates colorful barcodes from video files";

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor

also meta section

This comment has been minimized.

Copy link
@markuskowa

markuskowa Oct 11, 2019

Member

also:

Suggested change
description = "creates colorful barcodes from video files";
description = "Tool for creating colorful barcodes from video files";
rev = "7ad3f008afe803037b332822028faed64b1751bd";
sha256 = "14657zfzvq0iki8sn6rq789bgjr7wvvid9y2clfy25bi0a1gvnix";
};
buildInputs = [ cmake ffmpeg popt help2man ];

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@c0bw3b

c0bw3b Nov 23, 2019

Contributor

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

sha256 = "14657zfzvq0iki8sn6rq789bgjr7wvvid9y2clfy25bi0a1gvnix";
};
buildInputs = [ cmake ffmpeg popt help2man ];
configurePhase = ''

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor

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

done
done
'';
installFlags = [ "DESTDIR=$(out)" "CMAKE_INSTAL_PREFIX=$(out)" ];

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 11, 2019

Contributor

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

src = fetchFromGitHub {
owner = "nordlicht";
repo = "nordlicht";
rev = "7ad3f008afe803037b332822028faed64b1751bd";

This comment has been minimized.

Copy link
@markuskowa

markuskowa Oct 11, 2019

Member
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

doronbehar commented May 23, 2020

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.