kore: init at 2.0.0 #29989

JohnMH · 2017-10-01T16:53:32Z

Things done

Include kore web framework

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
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 nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

orivej · 2017-10-01T16:57:59Z

pkgs/development/web/kore/default.nix

+    homepage = https://kore.io;
+    license = licenses.isc;
+    platforms = platforms.all;
+    maintainers = [ "John M. Harris, Jr. <johnmh@openblox.org>" ];


Could you add yourself to lib/maintainers.nix?

orivej · 2017-10-01T17:00:49Z

pkgs/development/web/kore/default.nix

+{ stdenv, callPackage, openssl }:
+
+let
+  source = callPackage ./source.nix { };


Why did you put the source in a separate file? Generally this is only accepted when it must be updated by tools that cannot update default.nix.

orivej · 2017-10-01T17:03:04Z

pkgs/development/web/kore/default.nix

+
+  preConfigure = ''
+    makeFlags="$makeFlags PREFIX=$out"
+  '';


Try makeFlags = [ "PREFIX=$(out)" ].

vyp · 2017-10-01T17:02:15Z

pkgs/development/web/kore/default.nix

+stdenv.mkDerivation {
+  name = "kore-${source.version}";
+
+  src = source.src;


Why did you put src in a separate file?

vyp · 2017-10-01T17:03:30Z

pkgs/development/web/kore/default.nix

+    homepage = https://kore.io;
+    license = licenses.isc;
+    platforms = platforms.all;
+    maintainers = [ "John M. Harris, Jr. <johnmh@openblox.org>" ];


Add yourself to https://github.com/NixOS/nixpkgs/blob/master/lib/maintainers.nix instead so that you can just do maintainers = with maintainers; [ JohnMHarrisJr ];

JohnMH · 2017-10-01T17:23:44Z

Thanks for all the feedback, noticed some issues myself as soon as I submitted the PR.

vyp · 2017-10-01T17:34:57Z

lib/maintainers.nix

@@ -287,6 +287,7 @@
  joelmo = "Joel Moberg <joel.moberg@gmail.com>";
  joelteon = "Joel Taylor <me@joelt.io>";
  johbo = "Johannes Bornhold <johannes@bornhold.name>";
+  johnmh = "John M. Harris, Jr. <johnmh@openblox.org>";


The attribute name here should be your GitHub handle as it says at the top of the file.

@vyp That's absolutely horrible. I shouldn't even have to have a GitHub handle.

It has to be unique like this otherwise what if https://github.com/johnmh ever wants to maintain anything. Also it makes it easier to @ ping people.

I agree with the sentiment that needing a GitHub account is not great, but seeing as though to add a package to the official nixpkgs repository, you have to make a GitHub "pull request" adding it, which requires one to sign up to and use GitHub, I don't see that much of an issue here in requiring it. Might as well utilize GitHub handles if nixpkgs is hosted on GitHub in my opinion.

I don't actually log into the GitHub website unless I actually need to make a 'pull request'. I go by 'JohnMH' on everything but GitHub, IRC included. "pinging" me here wouldn't help anyone.

That's not ideal, but acceptable. In fact maintainers.nix already contains some attributes that are different from GitHub accounts of their owners and conflict with names of accounts of unrelated people who luckily do not contribute to Nix. (I don't remember who exactly.)

In your GitHub settings under notifications you can set it so that you get an email if you get @ mentioned. And even so, that still means that https://github.com/johnmh would be forced into using something other than their username, and that would just make things worse. The best and easiest way is to stick to exactly what your GitHub username is.

@orivej, I know that @edolstra is one. 😝

vyp · 2017-10-01T17:38:12Z

pkgs/development/web/kore/default.nix

+  src = fetchFromGitHub {
+    owner = "jorisvink";
+    repo = "kore";
+    rev = "a16348d5241b0b82f09c51cf3161872b6916cd76";


rev = "2.0.0-release";

Didn't realize GitHub supported owner/repo/archive/tagname-release.format. I'll go change that now.

Actually the upstream tagname itself is "2.0.0-release", so it's just referring to a git tag, not any GitHub format or anything.

orivej · 2017-10-01T18:46:49Z

Please review my changes in 717e6ac

The patch is needed because without it path/to/kore run will fail when kore is not in PATH.

JohnMH · 2017-10-01T18:57:16Z

@orivej Is that actually necessary? It substitutes PREFIX in before that, and PREFIX is set to $out

orivej · 2017-10-01T19:05:48Z

In kore masterit is no longer necessary (src/cli.c#L1399), but in 2.0.0 it is (src/cli.c#L1231).

JohnMH · 2017-10-01T19:08:07Z

Ah, I see. Thanks for the help with this, both of you.

orivej · 2017-10-01T19:08:33Z

Thank you too!

JohnMH · 2017-10-01T19:47:57Z

@vyp I managed to get the username JohnMH anyway, so am I good now? :)

vyp · 2017-10-02T00:29:53Z

@JohnMH yes of course! And also to be fair to you, people commonly do change their github usernames so a lot of names eventually become out of sync anyway (so the @ mentioning wouldn't work), and there's not much nixpkgs can do about that. So I feel like the email address is a more reliable identifier as I would say people change that much less often. It was just really more of a convenience thing and a way to avoid any potential future conflicts. 🙌

orivej reviewed Oct 1, 2017

View reviewed changes

vyp reviewed Oct 1, 2017

View reviewed changes

JohnMH force-pushed the master branch 2 times, most recently from ecc1167 to 62e438a Compare October 1, 2017 17:31

JohnMH requested a review from edolstra as a code owner October 1, 2017 17:31

vyp reviewed Oct 1, 2017

View reviewed changes

kore: init at 2.0.0

4219de0

JohnMH force-pushed the master branch from 62e438a to 4219de0 Compare October 1, 2017 18:08

kore: fix up

717e6ac

orivej merged commit 016df11 into NixOS:master Oct 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kore: init at 2.0.0 #29989

kore: init at 2.0.0 #29989

JohnMH commented Oct 1, 2017 •

edited

orivej Oct 1, 2017

orivej Oct 1, 2017 •

edited

orivej Oct 1, 2017

vyp Oct 1, 2017

vyp Oct 1, 2017

JohnMH commented Oct 1, 2017

vyp Oct 1, 2017

JohnMH Oct 1, 2017

vyp Oct 1, 2017

JohnMH Oct 1, 2017

orivej Oct 1, 2017 •

edited

vyp Oct 1, 2017 •

edited

vyp Oct 1, 2017

vyp Oct 1, 2017

JohnMH Oct 1, 2017

vyp Oct 1, 2017

orivej commented Oct 1, 2017 •

edited

JohnMH commented Oct 1, 2017

orivej commented Oct 1, 2017

JohnMH commented Oct 1, 2017

orivej commented Oct 1, 2017

JohnMH commented Oct 1, 2017

vyp commented Oct 2, 2017

kore: init at 2.0.0 #29989

kore: init at 2.0.0 #29989

Conversation

JohnMH commented Oct 1, 2017 • edited

Things done

Choose a reason for hiding this comment

orivej Oct 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMH commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orivej Oct 1, 2017 • edited

Choose a reason for hiding this comment

vyp Oct 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orivej commented Oct 1, 2017 • edited

JohnMH commented Oct 1, 2017

orivej commented Oct 1, 2017

JohnMH commented Oct 1, 2017

orivej commented Oct 1, 2017

JohnMH commented Oct 1, 2017

vyp commented Oct 2, 2017

JohnMH commented Oct 1, 2017 •

edited

orivej Oct 1, 2017 •

edited

orivej Oct 1, 2017 •

edited

vyp Oct 1, 2017 •

edited

orivej commented Oct 1, 2017 •

edited