nixos/nginx: add cgit sub-service #109915

afix-space · 2021-01-19T07:54:30Z

Motivation for this change

Similarly to module lighttpd/cgit or nginx/gitweb, add cgit sub-service to nginx

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
Built on platform(s)
- [ X] 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 nixpkgs-review --run "nixpkgs-review wip"
[ X] 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)
[ X] Ensured that relevant documentation is up to date
[ X] Fits CONTRIBUTING.md.

hmenke

Ideally the module should also have a nixos integration test.

hmenke · 2021-01-19T08:43:58Z

nixos/modules/services/web-servers/nginx/cgit.nix

+    configText = mkOption {
+      default = "";
+      example = ''
+        virtual-root=/
+        source-filter=''${pkgs.cgit}/lib/cgit/filters/syntax-highlighting.py
+        about-filter=''${pkgs.cgit}/lib/cgit/filters/about-formatting.sh
+        cache-size=1000
+        scan-path=/srv/git
+      '';
+      type = types.lines;
+      description = ''
+        Verbatim contents of the cgit runtime configuration file. Documentation
+        (with cgitrc example file) is available in "man cgitrc". Or online:
+        http://git.zx2c4.com/cgit/tree/cgitrc.5.txt
+      '';
+    };


This way of configuring services is discouraged (see RFC0042). Since the config file has an INI-like format, it should be easy to generate it from Nix expressions. See also https://nixos.org/manual/nixos/stable/index.html#sec-freeform-modules

hmenke · 2021-01-19T08:44:39Z

nixos/modules/services/web-servers/nginx/cgit.nix

+    virtualHost = mkOption {
+      default = "_";
+      type = types.str;
+      description = ''
+        VirtualHost to serve cgit on. Default is catch-all.
+      '';
+    };


This doesn't scale well. What if a user wants to run multiple cGit instances?

hmenke · 2021-01-19T08:46:10Z

nixos/modules/services/web-servers/nginx/cgit.nix

+    environment.systemPackages = [ pkgs.cgit ];
+
+    services.fcgiwrap.enable = true;
+    services.fcgiwrap.socketAddress = "/run/fcgiwrap.cgit.socket";


Suggested change

services.fcgiwrap.socketAddress = "/run/fcgiwrap.cgit.socket";

Leave this at the default or at least use mkDefault.

hmenke · 2021-01-19T08:46:42Z

nixos/modules/services/web-servers/nginx/cgit.nix

+              fastcgi_param   SCRIPT_FILENAME ${pkgs.cgit}/cgit/cgit.cgi;
+              fastcgi_param   QUERY_STRING    $args;
+              fastcgi_param   HTTP_HOST       $server_name;
+              fastcgi_pass    unix:/run/fcgiwrap.cgit.socket;


Use the system default here.

Suggested change

fastcgi_pass unix:/run/fcgiwrap.cgit.socket;

fastcgi_pass unix:${config.services.fcgiwrap.socketAddress}

hmenke · 2021-01-19T08:49:22Z

nixos/modules/services/web-servers/nginx/cgit.nix

+        locations."${cfg.location}" = {
+          extraConfig =
+          ''
+            gzip off;


Why do you turn off gzip?

afix-space · 2021-01-19T09:34:11Z

Thanks for this interesting feedback, i'll try to improve it. (and also fix the error with my git rebase).

hmenke · 2021-01-19T13:09:32Z

I had a go at this myself, since I'm also using cgit. Here you can see the result:
https://gist.github.com/hmenke/de2db3e303192a29e4bb6837e0bf48ff

afix-space · 2021-01-19T13:43:41Z

I had a go at this myself, since I'm also using cgit. Here you can see the result:
https://gist.github.com/hmenke/de2db3e303192a29e4bb6837e0bf48ff

thank you, it's great, I started to watch Freeform submodule but here is a clear example with a test.
thanks again, i will review this sub-service very soon.
Also i might have to open a new pull request because i messed up with git rebase :(
I will be able to learn to use git now with cgit :-)

hmenke · 2021-01-19T13:51:34Z

That's actually not a test (I guess test.nix was a misnomer) but a profile to run a VM using nixos-shell. A nixos test has to built separately still. Let me know if you need any help with that.

afix-space · 2021-01-20T10:21:28Z

it seems to me that the new cgit.nix is a very good base to add to nixpkgs modules. If test needed (or not) i'll play a bit with it.
starting by seeing examples in sources.

afix-space · 2021-01-20T11:50:53Z

in fact there are still some bug to be fixed, I continue to test and I do a summary later.

afix-space · 2021-01-20T13:26:33Z

The problem is that some settings could be multiple (section, repo.url, repo.path, repo.desc, repo.owner...).
This could have been solved by coercedTo like ini in pkgs/pkgs-lib/formats.nix, but they also have to be in a specific order.
Suddenly I need to take a nix pill or a hmenke gist before. :-)

hmenke · 2021-01-20T15:23:46Z

@afix-space I've updated the Gist which should reflect the required changes. For anything that needs to be ordered you can use the include directive to include a cgitrc from a file. It also includes a NixOS test to ensure the functionality. Let me know what you think.

afix-space · 2021-01-20T16:25:50Z

Nice changes, well done !
I learn a lot with your nix gist and it will be great this sub-service.

aanderse · 2021-01-20T22:18:10Z

@hmenke I think a fair bit of the settingsFormat work you have done there is now merged right into nixpkgs. Something like this is equivalent:

format = pkgs.formats.ini { listAsDuplicates = true; };

See https://github.com/NixOS/nixpkgs/blob/ba5f50a76c352df9bbe8ca5771a20d014c10aa99/pkgs/pkgs-lib/formats.nix#L59..L84 for details.

afix-space · 2021-01-21T00:02:44Z

I think that's why it allows more types than necessary.

afix-space · 2021-01-21T03:03:21Z

I had to make a new pull request. (I broke my branche, it's messy to repair)
#110270
I just corrected a few small oversights.

hmenke · 2021-01-21T07:02:47Z

@aanderse Unfortunately, none of the existing settings formats can be used here, because the cgitrc is not quite an INI (no [sections]). Note that I'm using lib.generators.toKeyValue to generate the file instead of lib.generators.toINI (as pkgs.formats.ini would do).

hmenke · 2021-01-22T16:57:36Z

Duplicate of #110270

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jan 19, 2021

hmenke suggested changes Jan 19, 2021

View reviewed changes

afix-space force-pushed the add/nginx-cgit-sub-service branch from 3317ca0 to 3d348c8 Compare January 19, 2021 08:57

nixos/nginx: add cgit sub-service

90922d4

afix-space force-pushed the add/nginx-cgit-sub-service branch from 3d348c8 to 90922d4 Compare January 19, 2021 09:51

afix-space mentioned this pull request Jan 21, 2021

nixos/nginx: add cgit sub-service (v2) #110270

Closed

5 tasks

hmenke marked this as a duplicate of #110270 Jan 22, 2021

hmenke closed this Jan 22, 2021

afix-space deleted the add/nginx-cgit-sub-service branch January 22, 2021 17:57

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/nginx: add cgit sub-service #109915

nixos/nginx: add cgit sub-service #109915

afix-space commented Jan 19, 2021 •

edited

hmenke left a comment

hmenke Jan 19, 2021

hmenke Jan 19, 2021

hmenke Jan 19, 2021

hmenke Jan 19, 2021

hmenke Jan 19, 2021

afix-space commented Jan 19, 2021

hmenke commented Jan 19, 2021

afix-space commented Jan 19, 2021

hmenke commented Jan 19, 2021

afix-space commented Jan 20, 2021

afix-space commented Jan 20, 2021

afix-space commented Jan 20, 2021

hmenke commented Jan 20, 2021

afix-space commented Jan 20, 2021

aanderse commented Jan 20, 2021

afix-space commented Jan 21, 2021 •

edited

afix-space commented Jan 21, 2021

hmenke commented Jan 21, 2021 •

edited

hmenke commented Jan 22, 2021

	fastcgi_pass unix:/run/fcgiwrap.cgit.socket;
	fastcgi_pass unix:${config.services.fcgiwrap.socketAddress}

nixos/nginx: add cgit sub-service #109915

nixos/nginx: add cgit sub-service #109915

Conversation

afix-space commented Jan 19, 2021 • edited

Motivation for this change

Things done

hmenke left a comment

Choose a reason for hiding this comment

hmenke Jan 19, 2021

Choose a reason for hiding this comment

hmenke Jan 19, 2021

Choose a reason for hiding this comment

hmenke Jan 19, 2021

Choose a reason for hiding this comment

hmenke Jan 19, 2021

Choose a reason for hiding this comment

hmenke Jan 19, 2021

Choose a reason for hiding this comment

afix-space commented Jan 19, 2021

hmenke commented Jan 19, 2021

afix-space commented Jan 19, 2021

hmenke commented Jan 19, 2021

afix-space commented Jan 20, 2021

afix-space commented Jan 20, 2021

afix-space commented Jan 20, 2021

hmenke commented Jan 20, 2021

afix-space commented Jan 20, 2021

aanderse commented Jan 20, 2021

afix-space commented Jan 21, 2021 • edited

afix-space commented Jan 21, 2021

hmenke commented Jan 21, 2021 • edited

hmenke commented Jan 22, 2021

afix-space commented Jan 19, 2021 •

edited

afix-space commented Jan 21, 2021 •

edited

hmenke commented Jan 21, 2021 •

edited