Update: easyjson, hydron, meguca, nodePackages: meguca; Improve: quicktemplate #43792

ghost · 2018-07-19T00:43:31Z

Motivation for this change

Update 4 packages, make 1 better conform to standards.

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 nox --run "nox-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)
Fits CONTRIBUTING.md.

ghost · 2018-07-25T01:25:58Z

cc @Mic92 @matthewbauer

matthewbauer · 2018-07-25T17:23:50Z

@GrahamcOfBorg build easyjson hydron meguca quicktemplate

GrahamcOfBorg · 2018-07-25T17:35:01Z

Success on x86_64-linux (full log)

Attempted: easyjson, hydron, meguca, quicktemplate

Partial log (click to expand)

shrinking /nix/store/jyi941xb9cpkyil61fddairlg53zvfh8-meguca-unstable-2018-07-25-bin/bin/meguca
strip is /nix/store/a3nk8z2i7m7wa3jdckgv710n7j3yx4b5-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/jyi941xb9cpkyil61fddairlg53zvfh8-meguca-unstable-2018-07-25-bin/bin
patching script interpreter paths in /nix/store/jyi941xb9cpkyil61fddairlg53zvfh8-meguca-unstable-2018-07-25-bin
checking for references to /build in /nix/store/jyi941xb9cpkyil61fddairlg53zvfh8-meguca-unstable-2018-07-25-bin...
strip is /nix/store/a3nk8z2i7m7wa3jdckgv710n7j3yx4b5-binutils-2.30/bin/strip
/nix/store/03djwdrdcydqvh6rqjh2iymv0p5dpiri-easyjson-unstable-2018-07-24-bin
/nix/store/lzjfcm3zs6yqj3p6hrlrbbl6ddkalnwz-hydron-unstable-2018-07-25-bin
/nix/store/jyi941xb9cpkyil61fddairlg53zvfh8-meguca-unstable-2018-07-25-bin
/nix/store/z4srjg7rhxz9wrh5bhqjq75dv7wkj9wl-quicktemplate-unstable-2018-04-30-bin

GrahamcOfBorg · 2018-07-25T18:20:37Z

Success on aarch64-linux (full log)

Attempted: easyjson, hydron, quicktemplate

The following builds were skipped because they don't evaluate on aarch64-linux: meguca

Partial log (click to expand)

shrinking RPATHs of ELF executables and libraries in /nix/store/gilf9ghi57mqpsfr5vb3km4fs8583876-hydron-unstable-2018-07-25-bin
shrinking /nix/store/gilf9ghi57mqpsfr5vb3km4fs8583876-hydron-unstable-2018-07-25-bin/bin/hydron
strip is /nix/store/a245zacjzf3qw0davhvlfarihcy2yyrc-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/gilf9ghi57mqpsfr5vb3km4fs8583876-hydron-unstable-2018-07-25-bin/bin
patching script interpreter paths in /nix/store/gilf9ghi57mqpsfr5vb3km4fs8583876-hydron-unstable-2018-07-25-bin
checking for references to /build in /nix/store/gilf9ghi57mqpsfr5vb3km4fs8583876-hydron-unstable-2018-07-25-bin...
strip is /nix/store/a245zacjzf3qw0davhvlfarihcy2yyrc-binutils-2.30/bin/strip
/nix/store/mirjvcikrwm3d9f06yrwk75vyhs97ma4-easyjson-unstable-2018-07-24-bin
/nix/store/gilf9ghi57mqpsfr5vb3km4fs8583876-hydron-unstable-2018-07-25-bin
/nix/store/5f08bdwf2bnzw6w38hzsk45p9fllam11-quicktemplate-unstable-2018-04-30-bin

GrahamcOfBorg · 2018-07-25T19:19:52Z

Success on x86_64-darwin (full log)

Attempted: easyjson, hydron, meguca, quicktemplate

Partial log (click to expand)

find: '/nix/store/a5cg07vadhhha113rwcxdfr4wm7mih3n-meguca-unstable-2018-07-25': No such file or directory
find: '/nix/store/a5cg07vadhhha113rwcxdfr4wm7mih3n-meguca-unstable-2018-07-25': No such file or directory
find: '/nix/store/a5cg07vadhhha113rwcxdfr4wm7mih3n-meguca-unstable-2018-07-25': No such file or directory
find: '/nix/store/a5cg07vadhhha113rwcxdfr4wm7mih3n-meguca-unstable-2018-07-25': No such file or directory
find: '/nix/store/a5cg07vadhhha113rwcxdfr4wm7mih3n-meguca-unstable-2018-07-25': No such file or directory
find: '/nix/store/a5cg07vadhhha113rwcxdfr4wm7mih3n-meguca-unstable-2018-07-25': No such file or directory
/nix/store/0n93mhik0irzvr6alw2jbi5f70zvimxi-easyjson-unstable-2018-07-24-bin
/nix/store/kja6zg9yh8hvywd90ggmcl3k8313fpaf-hydron-unstable-2018-07-25-bin
/nix/store/k4qfr7i255cj8qwgbg3jnbr94cfm2c2a-meguca-unstable-2018-07-25-bin
/nix/store/1nrdgv06hii9a7k534df1qq326arqpyn-quicktemplate-unstable-2018-04-30-bin

ghost · 2018-08-01T20:31:36Z

Don't merge yet, please.

ghost · 2018-08-02T19:57:34Z

Okay, we're good now. The last meguca commit had a bug in upstream, and I didn't realize until after the push.
CC @matthewbauer

infinisil · 2018-08-02T21:15:46Z

nixos/modules/services/web-servers/meguca.nix

  options.services.meguca = {
    enable = mkEnableOption "meguca";

-    baseDir = mkOption {
+    dataDir = mkOption {


This will break configs of people using the baseDir option. You'd need to use mkRenamedOptionModule to hint at a rename.

Like this, right?

imports = [ (mkRenamedOptionModule [ "baseDir" ] [ "dataDir" ]) ];

Yup, but you need to use the full paths to the options

infinisil · 2018-08-02T21:16:23Z

nixos/modules/services/web-servers/meguca.nix

      type = types.path;
-      default = "/run/meguca";
+      default = "/var/lib/meguca";


Changing the default will break everybody's setup that hasn't set the option explicitly.

It was already broken since /run deletes the data each time.

Oh haha, I see, never mind then

infinisil · 2018-08-02T21:20:48Z

nixos/modules/services/web-servers/hydron.nix

+
+    postgresArgs = mkOption {
+      type = types.str;
+      default = "user=hydron password=" + cfg.password + " dbname=hydron sslmode=disable";


This would rebuild the manual every time one changes the password. Assign a default in the config section with mkDefault or use defaultText = to avoid that.

infinisil · 2018-08-02T21:22:17Z

nixos/modules/services/web-servers/hydron.nix

+    security.sudo.enable = cfg.enable == true;
+    services.postgresql.enable = cfg.enable == true;
+
+    services.hydron.passwordFile = mkDefault (toString (pkgs.writeTextFile {


You can use the shorter pkgs.writeText "hydron-password-file" cfg.password. Also toString shouldn't be needed.

infinisil · 2018-08-02T21:22:32Z

nixos/modules/services/web-servers/hydron.nix

@@ -47,16 +77,40 @@ in with lib; {
  };

  config = mkIf cfg.enable {
+    security.sudo.enable = cfg.enable == true;
+    services.postgresql.enable = cfg.enable == true;


Use just services.postgresql.enable = cfg.enable

infinisil · 2018-08-02T21:26:47Z

nixos/modules/services/web-servers/hydron.nix

+        mkdir -p ${escapeShellArg cfg.dataDir}/{.hydron,images}
+        cp ${pkgs.hydron}/share/hydron/db_conf.json ${escapeShellArg cfg.dataDir}/.hydron
+        sed -i "s/user=hydron password=hydron dbname=hydron sslmode=disable/$(cat ${escapeShellArg cfg.postgresArgsFile})/g" \
+          ${escapeShellArg cfg.dataDir}/.hydron/db_conf.json


Since this db_conf.json file gets copied every time, I think it's better to build the file with Nix instead and have it be a symlink to the Nix-built version. Then the user won't think of modifying it. Something like

ln -s ${pkgs.runCommand "db_conf.json" {} "cp && sed"} ${cfg.dataDir}/.hydron/db_conf.json

ghost · 2018-08-03T00:58:00Z

@infinisil How does this look?

infinisil · 2018-08-03T14:29:42Z

nixos/modules/services/web-servers/hydron.nix

+        {
+          "driver": "postgres",
+          "connection": "user=hydron password='' + cfg.password + " " + ''dbname=hydron sslmode=disable"
+      '' + "}";


The manual will prefer defaultText over default, so using the same for both doesn't do anything. You'll want to use something like password=\${cfg.password} to not have Nix actually insert the password. Or use password=${options.services.hydron.password.default}. But on second thought, it might be better to not set the default here, but instead use mkDefault in the config section, such that nothing is duplicated, up to you.

And what's up with the weird quotes? I'd prefer to only use one type everywhere, you can use string interpolation foo=${foo} as well.

Also why is this a json string now?

db_conf.json is a json string, so I'd rather just put the string directly, and we avoid a lot of cruft this way.
The weird quotes are just to make it look pretty on-disk, since for some reason the last } keeps it's tabbing and doesn't strip white space.

Also, for some reason, the quote after cfg.password does strip whitespace, so you end up getting password=whateverdbname=, without that extra quote, anyway.

Looks like string interpolation fixed my problem with the last } being tabbed.

infinisil · 2018-08-03T14:33:50Z

nixos/modules/services/web-servers/hydron.nix

@@ -30,6 +30,7 @@ in with lib; {
    password = mkOption {
      type = types.str;
      default = "hydron";
+      defaultText = "hydron";


Not needed, will use default as text by default.

infinisil · 2018-08-03T14:36:23Z

Same thing with meguca

infinisil · 2018-08-03T15:36:50Z

nixos/modules/services/web-servers/hydron.nix

+    security.sudo.enable = cfg.enable;
+    services.postgresql.enable = cfg.enable;
+    services.hydron.passwordFile = mkDefault (pkgs.writeText "hydron-password-file" cfg.password);
+    services.hydron.password = mkDefault ("hydron");


No need to set the default here, put it into the option directly. The mkDefault thing is only needed when the default depends on another option value.

infinisil · 2018-08-03T15:37:47Z

nixos/modules/services/web-servers/hydron.nix

+        "driver": "postgres",
+        "connection": "user=hydron password=${cfg.password} dbname=hydron sslmode=disable"
+      }
+    '');


Unnecessary parens

ghost · 2018-08-03T15:59:52Z

@infinisil hai dozo

infinisil

LGTM, arigatou gozaimasu!

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Jul 19, 2018

ghost mentioned this pull request Jul 21, 2018

Start on boot bakape/shamichan#803

Closed

matthewbauer approved these changes Jul 25, 2018

View reviewed changes

infinisil requested changes Aug 2, 2018

View reviewed changes

infinisil requested changes Aug 3, 2018

View reviewed changes

GrahamcOfBorg added the 2.status: merge conflict label Aug 3, 2018

infinisil reviewed Aug 3, 2018

View reviewed changes

Madouura added 7 commits August 3, 2018 10:40

quicktemplate: Use fetchFromGitHub, not fetchgit

405e4fb

easyjson: 2018-06-06 -> 2018-07-30

8e690b6

nodePackages: meguca: 1.0.8 -> 1.0.9

4f99b18

hydron: 2018-07-15 -> 2018-07-30

74a57e2

meguca: 2018-07-01 -> 2018-08-02

6de26d6

nixos/hydron: Various fixes, create db_conf.json and link to it

d49b5bd

nixos/meguca: Various fixes

36ab899

GrahamcOfBorg removed the 2.status: merge conflict label Aug 3, 2018

infinisil approved these changes Aug 3, 2018

View reviewed changes

infinisil merged commit a35a488 into NixOS:master Aug 3, 2018

ghost deleted the dev/hydron branch August 3, 2018 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: easyjson, hydron, meguca, nodePackages: meguca; Improve: quicktemplate #43792

Update: easyjson, hydron, meguca, nodePackages: meguca; Improve: quicktemplate #43792

ghost commented Jul 19, 2018 •

edited by ghost

Loading

ghost commented Jul 25, 2018

matthewbauer commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

ghost commented Aug 1, 2018

ghost commented Aug 2, 2018

infinisil Aug 2, 2018

ghost Aug 2, 2018

infinisil Aug 2, 2018

infinisil Aug 2, 2018

ghost Aug 2, 2018

infinisil Aug 2, 2018

infinisil Aug 2, 2018

infinisil Aug 2, 2018

infinisil Aug 2, 2018

infinisil Aug 2, 2018

ghost commented Aug 3, 2018

infinisil Aug 3, 2018

ghost Aug 3, 2018

ghost Aug 3, 2018 •

edited by ghost

Loading

ghost Aug 3, 2018

infinisil Aug 3, 2018

infinisil commented Aug 3, 2018

infinisil Aug 3, 2018 •

edited

Loading

infinisil Aug 3, 2018

ghost commented Aug 3, 2018

infinisil left a comment

Update: easyjson, hydron, meguca, nodePackages: meguca; Improve: quicktemplate #43792

Update: easyjson, hydron, meguca, nodePackages: meguca; Improve: quicktemplate #43792

Conversation

ghost commented Jul 19, 2018 • edited by ghost Loading

Motivation for this change

Things done

ghost commented Jul 25, 2018

matthewbauer commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

GrahamcOfBorg commented Jul 25, 2018

ghost commented Aug 1, 2018

ghost commented Aug 2, 2018

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

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

ghost commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Aug 3, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Aug 3, 2018

infinisil Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 3, 2018

infinisil left a comment

Choose a reason for hiding this comment

ghost commented Jul 19, 2018 •

edited by ghost

Loading

ghost Aug 3, 2018 •

edited by ghost

Loading

infinisil Aug 3, 2018 •

edited

Loading