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

afix-space · 2021-01-21T02:47:52Z

Second version, greatly improved by hmenke.
Especially free form, test, and allows to have several instances of
cgit...

Motivation for this change

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
[ X] 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.

afix-space · 2021-01-21T02:56:53Z

I bugged the branch of my first pull request.
First discussion is here :
#109915

hmenke

I don't quite understand why you renamed server into machine in the test, but otherwise it looks good.

hmenke

Sorry, approval retracted. The test has to be enabled.

--- a/nixos/tests/all-tests.nix
+++ b/nixos/tests/all-tests.nix
@@ -265,6 +265,7 @@ in
   nghttpx = handleTest ./nghttpx.nix {};
   nginx = handleTest ./nginx.nix {};
   nginx-auth = handleTest ./nginx-auth.nix {};
+  nginx-cgit = handleTest ./nginx-cgit.nix {};
   nginx-etag = handleTest ./nginx-etag.nix {};
   nginx-pubhtml = handleTest ./nginx-pubhtml.nix {};
   nginx-sandbox = handleTestOn ["x86_64-linux"] ./nginx-sandbox.nix {};

hmenke · 2021-01-21T08:56:07Z

@ofborg test nginx-cgit

hmenke · 2021-01-21T09:02:55Z

Oops, evaluation failed because you haven't added yourself to the maintainer list:

--- a/maintainers/maintainer-list.nix
+++ b/maintainers/maintainer-list.nix
@@ -10478,4 +10478,10 @@
     github = "zupo";
     githubId = 311580;
   };
+  afix-space = {
+    name = "<FILL ME IN>";
+    email = "<FILL ME IN>";
+    github = "afix-space";
+    githubId = 18408666;
+  };
 }

hmenke · 2021-01-21T09:28:44Z

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

+            root = "${pkgs.cgit}/cgit/";
+            tryFiles = "$uri @cgit";
+          };
+          locations."~* ^/(cgit.(css|png)|favicon.ico|robots.txt)" = {


I think this actually should be

Suggested change

locations."~* ^/(cgit.(css|png)|favicon.ico|robots.txt)" = {

locations."~ ^${cfg.location}/(cgit.(css|png)|favicon.ico|robots.txt)" = {

but I'm not 100% sure since I'm not too familiar with nginx's location matching. From what I know ~* would be a case-insensitive regex match, which seems wrong and ^/(...) would only match correctly if location = "/". But again, I could be wrong.

You are right,
this can be done with :
locations."~ ^${cfg.location}/(cgit.(css|png)|favicon.ico|robots.txt)$" = { alias = "${pkgs.cgit}/cgit/$1"; };
We also need to do a test and change default cgitrc values, but i lack skill with nix.
I'm trying to do this:
css = mkDefault "${cfg.location}/cgit.css";
But i get "infinite recursion encountered" error.
i need to learn nix in the meantime, help would be welcome.

maybe the problem is not that trivial in fact. anyway it's just default options and this shouldn't cause backward compatibility problem in case of improvement.

hmenke · 2021-01-21T20:41:45Z

@ofborg test nginx-cgit

hmenke

I think we should also add a second test with a different location that fetches one of the static assets to ensure that the rewriting works.

hmenke · 2021-01-21T20:48:53Z

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

+    type = types.attrsOf (types.submodule ({ config, ... }:
+      let
+        cfg = config.cgit;
+        pathLocationFix = if (cfg.location == "/") then  "" else cfg.location;


That is way too non-local. Move this closer to the point of use. Or just remove it altogether as suggested below.

I will see for the test afterwards, on the other hand now that the pull request has been updated. how should I do? I keep pushing on my branch? thanks.

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

hmenke · 2021-01-21T21:37:14Z

I've updated the Gist https://gist.github.com/hmenke/de2db3e303192a29e4bb6837e0bf48ff incorporating your changes, adding a second test, and simplifying the first one to only use a single node. Then finally you should also rebase and squash your branch so that it looks like

* (HEAD) nixos/nginx: add cgit sub-service
* maintainers: add afix-space

hmenke

Sorry, there was another misunderstanding. You squashed everything into a single commit with the message

nixos/nginx: add cgit sub-service
maintainers: add afix-space

but what I meant was to split it into two commits where the first one has the message maintainers: add afix-spaceand only contains the change to maintainers/maintainer-list.nix and the second one with the message nixos/nginx: add cgit sub-service contains everything else. It's important that adding yourself as a maintainer comes before the other stuff in the history. Otherwise the tests will fail, because they also check the metadata.

hmenke · 2021-01-22T15:21:56Z

nixos/tests/nginx-cgit.nix

+# ./result/bin/nixos-test-driver
+# >>> test_script()
+{
+  "http-clone" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: {


Suggested change

"http-clone" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: {

"http-clone" = import ./make-test-python.nix ({ lib, ... }: {

hmenke · 2021-01-22T15:22:13Z

nixos/tests/nginx-cgit.nix

+# nix-build test.nix -A driver
+# ./result/bin/nixos-test-driver
+# >>> test_script()


Remove unnecessary comments.

hmenke · 2021-01-22T15:22:28Z

nixos/tests/nginx-cgit.nix

+    '';
+  });
+
+  "location" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: {


Suggested change

"location" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: {

"location" = import ./make-test-python.nix ({ lib, ... }: {

afix-space · 2021-01-22T16:00:16Z

it's done, sorry I hadn't thought of that.

hmenke · 2021-01-22T16:03:07Z

@ofborg test nginx-cgit.http-clone nginx-cgit.location

hmenke · 2021-01-22T16:15:18Z

nixos/tests/nginx-cgit.nix

@@ -0,0 +1,91 @@
+{


That has to be a function. Sorry, that was an oversight on my part.

Suggested change

{

{ ... }: {

Once that is fixed and the tests pass I will ping someone to merge.

hmenke · 2021-01-22T16:31:52Z

@ofborg test nginx-cgit.http-clone nginx-cgit.location

hmenke · 2021-01-22T16:39:05Z

@aanderse You have already had a look at #109915 which is superseded by this PR, so maybe you could guide us through the final steps to merge it? The tests work on x86_64 but fail on aarch64 with

nix-instantiate: src/libexpr/attr-set.hh:55: void nix::Bindings::push_back(const nix::Attr&): Assertion `size_ < capacity_' failed.

Do you have any idea why or how to fix this? If not, I guess we could just disable the test for aarch64.

Ma27 · 2021-01-22T21:30:41Z

Can you explain to me why a program like cgit is supposed to be a subservice of nginx?

hmenke · 2021-01-22T21:47:06Z

Great question! You could ask the same thing about gitweb and I guess the answer is: Because nginx is the most popular webserver.

Ma27 · 2021-01-22T21:52:24Z

Because nginx is the most popular webserver.

Well I like nginx as well :) And I understand that we may want to have a module with an nginx config for cgit. But what's wrong with adding its own module in its own namespace (the same is done for e.g. services.nextcloud and a few other things that mostly need nginx and some kind of backend behind that).

That said I've just seen that there seems to be a module in the httpd "namespace" already, so I'm actually wondering why this was added in the first place 😄

afix-space · 2021-01-22T22:28:52Z

@Ma27 In my newbie opinion the location is consistent with the current structure of nixpkgs. but the question is relevant ...

bsima · 2021-04-28T02:28:08Z

But what's wrong with adding its own module in its own namespace

This is what gitweb does. We have services.gitweb for gitweb-specific configuration, and then nginx.gitweb and lighttpd.gitweb for the networking stuff. Sounds reasonable to me.

That said I have this PR applied to my config and its working, proof: https://simatime.com/git/nixpkgs.git/

stale · 2021-10-30T17:09:30Z

I marked this as stale due to inactivity. → More info

schnusch · 2022-04-21T17:02:02Z

I missed this PR and created #167319.

The main difference is the configuration is split into repos/scanPath and settings. https://github.com/NixOS/nixpkgs/blob/9e777a4791ad1e1b750e0dc175b84b566ad3d2f9/nixos/modules/services/web-servers/nginx/cgit.nix#L72-L107

This ensures that the options are in the correct order in the cgitrc. https://github.com/NixOS/nixpkgs/blob/9e777a4791ad1e1b750e0dc175b84b566ad3d2f9/nixos/modules/services/web-servers/nginx/cgit.nix#L19-L38

Also the FastCGI params are moved to services.nginx.virtualHosts.<name>.locations.<name>.fastcgiParams instead of extraConfig.

A location prefix is unsupported, but should we move to #167319 or should I try to merge my changes into this?

schnusch · 2022-06-06T10:29:35Z

I added a location prefix to #167319 and move to options to services.nginx.virtualHost.….cgit as well. #167319 should now do everything this PR does as well as git clone over http and proper settings order in cgitrc.

SuperSandro2000 · 2023-01-30T19:26:39Z

Closing in favor of the other listed PRs.

ofborg bot added 6.topic: nixos 8.has: module (update) labels Jan 21, 2021

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jan 21, 2021

afix-space mentioned this pull request Jan 21, 2021

nixos/nginx: add cgit sub-service #109915

Closed

6 tasks

afix-space changed the title ~~nixos/nginx: add cgit sub-service~~ nixos/nginx: add cgit sub-service (v2) Jan 21, 2021

hmenke approved these changes Jan 21, 2021

View reviewed changes

hmenke suggested changes Jan 21, 2021

View reviewed changes

hmenke reviewed Jan 21, 2021

View reviewed changes

hmenke suggested changes Jan 21, 2021

View reviewed changes

afix-space force-pushed the add/nginx-cgit-sub-service_part2 branch 3 times, most recently from a96026d to 3327387 Compare January 22, 2021 09:55

hmenke suggested changes Jan 22, 2021

View reviewed changes

maintainers: add afix-space

d16c121

afix-space force-pushed the add/nginx-cgit-sub-service_part2 branch 2 times, most recently from 64644a8 to 0afbf57 Compare January 22, 2021 15:59

hmenke reviewed Jan 22, 2021

View reviewed changes

nixos/nginx: add cgit sub-service

345145d

afix-space force-pushed the add/nginx-cgit-sub-service_part2 branch from 0afbf57 to 345145d Compare January 22, 2021 16:21

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 30, 2021

Lassulus mentioned this pull request Apr 21, 2022

nixos/cgit: init #167319

Merged

13 tasks

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 21, 2022

Artturin added the 12.approvals: 1 label May 7, 2022

bobby285271 removed the 12.approvals: 1 label Jun 10, 2022

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 8, 2023

SuperSandro2000 closed this Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

afix-space commented Jan 21, 2021

afix-space commented Jan 21, 2021

hmenke left a comment

hmenke left a comment

hmenke commented Jan 21, 2021

hmenke commented Jan 21, 2021

hmenke Jan 21, 2021

afix-space Jan 21, 2021 •

edited

afix-space Jan 21, 2021

hmenke commented Jan 21, 2021

hmenke left a comment

hmenke Jan 21, 2021

afix-space Jan 21, 2021

hmenke commented Jan 21, 2021

hmenke left a comment

hmenke Jan 22, 2021

hmenke Jan 22, 2021

hmenke Jan 22, 2021

afix-space commented Jan 22, 2021

hmenke commented Jan 22, 2021

hmenke Jan 22, 2021

hmenke Jan 22, 2021

hmenke commented Jan 22, 2021

hmenke commented Jan 22, 2021

Ma27 commented Jan 22, 2021

hmenke commented Jan 22, 2021

Ma27 commented Jan 22, 2021

afix-space commented Jan 22, 2021 •

edited

bsima commented Apr 28, 2021 •

edited

stale bot commented Oct 30, 2021

schnusch commented Apr 21, 2022

schnusch commented Jun 6, 2022

SuperSandro2000 commented Jan 30, 2023

	locations."~* ^/(cgit.(css\|png)\|favicon.ico\|robots.txt)" = {
	locations."~ ^${cfg.location}/(cgit.(css\|png)\|favicon.ico\|robots.txt)" = {

	"http-clone" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: {
	"http-clone" = import ./make-test-python.nix ({ lib, ... }: {

	"location" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: {
	"location" = import ./make-test-python.nix ({ lib, ... }: {

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

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

Conversation

afix-space commented Jan 21, 2021

Motivation for this change

Things done

afix-space commented Jan 21, 2021

hmenke left a comment

Choose a reason for hiding this comment

hmenke left a comment

Choose a reason for hiding this comment

hmenke commented Jan 21, 2021

hmenke commented Jan 21, 2021

Choose a reason for hiding this comment

afix-space Jan 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmenke commented Jan 21, 2021

hmenke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmenke commented Jan 21, 2021

hmenke left a 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

afix-space commented Jan 22, 2021

hmenke commented Jan 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmenke commented Jan 22, 2021

hmenke commented Jan 22, 2021

Ma27 commented Jan 22, 2021

hmenke commented Jan 22, 2021

Ma27 commented Jan 22, 2021

afix-space commented Jan 22, 2021 • edited

bsima commented Apr 28, 2021 • edited

stale bot commented Oct 30, 2021

schnusch commented Apr 21, 2022

schnusch commented Jun 6, 2022

SuperSandro2000 commented Jan 30, 2023

afix-space Jan 21, 2021 •

edited

afix-space commented Jan 22, 2021 •

edited

bsima commented Apr 28, 2021 •

edited