-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
nixos/nginx: add cgit sub-service (v2) #110270
nixos/nginx: add cgit sub-service (v2) #110270
Conversation
I bugged the branch of my first pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand why you renamed server
into machine
in the test, but otherwise it looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {};
@ofborg test nginx-cgit |
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;
+ };
} |
root = "${pkgs.cgit}/cgit/"; | ||
tryFiles = "$uri @cgit"; | ||
}; | ||
locations."~* ^/(cgit.(css|png)|favicon.ico|robots.txt)" = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this actually should be
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@ofborg test nginx-cgit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
type = types.attrsOf (types.submodule ({ config, ... }: | ||
let | ||
cfg = config.cgit; | ||
pathLocationFix = if (cfg.location == "/") then "" else cfg.location; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is way too non-local. Move this closer to the point of use. Or just remove it altogether as suggested below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
|
a96026d
to
3327387
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-space
and 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.
nixos/tests/nginx-cgit.nix
Outdated
# ./result/bin/nixos-test-driver | ||
# >>> test_script() | ||
{ | ||
"http-clone" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"http-clone" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: { | |
"http-clone" = import ./make-test-python.nix ({ lib, ... }: { |
nixos/tests/nginx-cgit.nix
Outdated
# nix-build test.nix -A driver | ||
# ./result/bin/nixos-test-driver | ||
# >>> test_script() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary comments.
nixos/tests/nginx-cgit.nix
Outdated
''; | ||
}); | ||
|
||
"location" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"location" = import <nixpkgs/nixos/tests/make-test-python.nix> ({ lib, ... }: { | |
"location" = import ./make-test-python.nix ({ lib, ... }: { |
64644a8
to
0afbf57
Compare
it's done, sorry I hadn't thought of that. |
@ofborg test nginx-cgit.http-clone nginx-cgit.location |
nixos/tests/nginx-cgit.nix
Outdated
@@ -0,0 +1,91 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That has to be a function. Sorry, that was an oversight on my part.
{ | |
{ ... }: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once that is fixed and the tests pass I will ping someone to merge.
0afbf57
to
345145d
Compare
@ofborg test nginx-cgit.http-clone nginx-cgit.location |
@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
Do you have any idea why or how to fix this? If not, I guess we could just disable the test for aarch64. |
Can you explain to me why a program like cgit is supposed to be a subservice of |
Great question! You could ask the same thing about gitweb and I guess the answer is: 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. 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 😄 |
@Ma27 In my newbie opinion the location is consistent with the current structure of nixpkgs. but the question is relevant ... |
This is what gitweb does. We have That said I have this PR applied to my config and its working, proof: https://simatime.com/git/nixpkgs.git/ |
I marked this as stale due to inactivity. → More info |
I missed this PR and created #167319. The main difference is the configuration is split into 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 A location prefix is unsupported, but should we move to #167319 or should I try to merge my changes into this? |
Closing in favor of the other listed PRs. |
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
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)