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

Initialize Hound package / module #19498

Merged
merged 2 commits into from
Oct 15, 2016
Merged

Initialize Hound package / module #19498

merged 2 commits into from
Oct 15, 2016

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Oct 12, 2016

Motivation for this change

https://search.nix.gsc.io is powered by Hound, and I figured it couldn't hurt to upstream the code for it.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • 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.

@grahamc grahamc added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: package (new) This PR adds a new package 8.has: module (new) This PR adds a module in `nixos/` labels Oct 12, 2016
@mention-bot
Copy link

@grahamc, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @joachifm and @offlinehacker to be potential reviewers.

@@ -276,6 +276,7 @@
telegraf = 256;
gitlab-runner = 257;
postgrey = 258;
hound = 259;
Copy link
Contributor

Choose a reason for hiding this comment

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

What sort of data does hound generate? Is it not sufficient to ensure eventually consistent ownership via preStart or similar mechanisms?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm really not sold on using preStart's to chmod a bunch of data. One of my hound installs has 700GB of owned data. I definitely don't want to be running chmod during a restart.

Copy link
Member

Choose a reason for hiding this comment

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

sounds reasonable.


goDeps = ./deps.nix;

# TODO: add metadata https://nixos.org/nixpkgs/manual/#sec-standard-meta-attributes
Copy link
Member

Choose a reason for hiding this comment

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

obsolete.

User = cfg.user;
Group = cfg.group;
WorkingDirectory = cfg.home;
ExecStartPre = "${pkgs.git}/bin/git config --global --add http.sslCAinfo /etc/ssl/certs/ca-certificates.crt";
Copy link
Member

Choose a reason for hiding this comment

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

this adds a new line per start.
I would prefer the following instead:

ExecStartPre = "${pkgs.git}/bin/git config --global --replace-all http.sslCAinfo /etc/ssl/certs/ca-certificates.crt";

};

systemd.services.hound = {
description = "Hound Code Search from Etsy";
Copy link
Member

Choose a reason for hiding this comment

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

I think Hound Code Search is enough. On startup it will then show:

Started Hound Code Search.

'';
};

extraGroups = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

what is the use case for this option?

@Mic92
Copy link
Member

Mic92 commented Oct 15, 2016

Last but not least the test should be also added to release.nix:

diff --git a/nixos/release.nix b/nixos/release.nix
index 10c624a..fbd3efd 100644
--- a/nixos/release.nix
+++ b/nixos/release.nix
@@ -245,6 +245,7 @@ in rec {
   tests.gnome3-gdm = callTest tests/gnome3-gdm.nix {};
   tests.grsecurity = callTest tests/grsecurity.nix {};
   tests.hibernate = callTest tests/hibernate.nix {};
+  tests.hound = callTest tests/hound.nix {};
   tests.i3wm = callTest tests/i3wm.nix {};
   tests.installer = callSubTests tests/installer.nix {};
   tests.influxdb = callTest tests/influxdb.nix {};

@grahamc
Copy link
Member Author

grahamc commented Oct 15, 2016

@Mic92 I addressed the feedback, other than adding it to the release. I don't really want it to block the tested job. Will this?

@Mic92
Copy link
Member

Mic92 commented Oct 15, 2016

@grahamc well, otherwise this tests is never evaluated by hydra. What do you mean by block?

@grahamc
Copy link
Member Author

grahamc commented Oct 15, 2016

Fixed. :) Thanks for the review!

@grahamc grahamc merged commit fbadf2d into NixOS:master Oct 15, 2016
@grahamc grahamc deleted the hound branch October 15, 2016 17:56
@ttuegel
Copy link
Member

ttuegel commented Oct 15, 2016

This breaks nixos-rebuild with

Option `services.hound.config' has no description.

even for configurations that don't have Hound enabled.

@grahamc
Copy link
Member Author

grahamc commented Oct 15, 2016

D'oh, thank you for the heads up. Curious that it errors there, but didn't error in my local usage. Preparing a fix.

@grahamc
Copy link
Member Author

grahamc commented Oct 15, 2016

Fixed up. Sorry about that. Is this a new check not in 16.09? I've used this module in 16.09.

@bjornfor
Copy link
Contributor

$ nix-env -f . -qa '*' --meta --xml --drv-path --show-trace
[...]
error: while querying the derivation named ‘go1.7-hound-unstable-20160919-f95e9a9’:
attribute ‘homepage’ missing, at /home/bfo/proj/code/forks/nixpkgs3/pkgs/development/tools/misc/hound/default.nix:19:28

@grahamc
Copy link
Member Author

grahamc commented Oct 16, 2016

@bjornfor guh, sorry. 😳

I fixed hound in 104d696

and in testing found another PR broke it as well, which I fixed in 33ac1e1 (/cc @Profpatsch, @Mic92 to learn what I just learned, too :) )

is this the best way to find these sorts of errors in the future?

This seems like a really good test to have in Travis-CI -- which shouldn't be so hard to run as to fail regularly.

@bjornfor
Copy link
Contributor

I'm surprised travis doesn't run that check. I have put that command in a script, for my own use. It catches a lot of those eval issues:

$ cat ~/bin/nix-check-before-push.sh 
#!/bin/sh
# This checks for evaluation errors in nixpkgs (and nixos?) git repositories.
# Run it before commit/push.

set -x
nix-env -f . -qa \* --meta --xml --drv-path --show-trace >/dev/null || { echo FAILED; exit 1; }
nix-build pkgs/top-level/release.nix -A tarball || { echo FAILED; exit 1; }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (new) This PR adds a module in `nixos/` 8.has: package (new) This PR adds a new package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants