@GrahamcOfBorg test nginx nginx-etag nginx-pubhtml nginx-sso

What's the motivation for this change?

#85752 (comment)
#34378

@GrahamcOfBorg eval

please rebase your branch ontop of master, and force push to fix eval:

git pull -r origin/master
git push --force .. ..

@GrahamcOfBorg test nginx nginx-etag nginx-pubhtml nginx-sso

How to correct add patch?

    }) ++ [
      ./nix-skip-check-logs-path.patch
    ] ++ optionals (stdenv.hostPlatform != stdenv.buildPlatform) [

@Emily reverted commit openresty. I did not find another variant. Please check.

Why was 6d046e1 reverted? OpenResty is a distribution of nginx with additional patches and modules; keeping the derivations wholly separate will lead to code duplication and drift between the two, making maintenance harder.

If they're going to be separate then the third-party module functionality should at least be ported over to the OpenResty derivation; with the revert, there's no way to e.g. use OpenResty with the brotli module. But I don't see the benefit at all currently.

With this variant

    }) ++ [
      ./nix-skip-check-logs-path.patch
    ] ++ optionals (stdenv.hostPlatform != stdenv.buildPlatform) [

Error build openresty:

error: value is a path while a set was expected, at nixpkgs/pkgs/servers/http/openresty/default.nix:20:29

How to fix the error?

The evaluation error can be fixed with:

diff --git a/pkgs/servers/http/openresty/default.nix b/pkgs/servers/http/openresty/default.nix
index 0e87b971985..01ee521a2a2 100644
--- a/pkgs/servers/http/openresty/default.nix
+++ b/pkgs/servers/http/openresty/default.nix
@@ -16,8 +16,8 @@ callPackage ../nginx/generic.nix args rec {
     sha256 = "1a1la7vszv1parsnhphydblz64ffhycazncn3ividnvqg2mg735n";
   };
 
-  fixPatch = patch:
-    runCommand "openresty-${patch.name}" { src = patch; } ''
+  fixPatch = patch: let name = patch.name or (builtins.baseNameOf patch); in
+    runCommand "openresty-${name}" { src = patch; } ''
       substitute $src $out \
         --replace "src/" "bundle/nginx-${nginxVersion}/src/"
     '';

The patch still fails to apply, though; you could either condition on the patch name in fixPatch and replace it with a local OpenResty version (gross), or I think the better approach would be to add skipLogsCheck ? ./nix-skip-checks-log-path.patch to the nginx derivation and pass in an alternate patch path for packages that need it.

Actually, there's no need for a separate patch at all; just need to make OpenResty's fixPatch more robust:

diff --git a/pkgs/servers/http/openresty/default.nix b/pkgs/servers/http/openresty/default.nix
index 0e87b971985..9c01cfb19e1 100644
--- a/pkgs/servers/http/openresty/default.nix
+++ b/pkgs/servers/http/openresty/default.nix
@@ -16,10 +16,11 @@ callPackage ../nginx/generic.nix args rec {
     sha256 = "1a1la7vszv1parsnhphydblz64ffhycazncn3ividnvqg2mg735n";
   };
 
-  fixPatch = patch:
-    runCommand "openresty-${patch.name}" { src = patch; } ''
+  fixPatch = patch: let name = patch.name or (builtins.baseNameOf patch); in
+    runCommand "openresty-${name}" { src = patch; } ''
       substitute $src $out \
-        --replace "src/" "bundle/nginx-${nginxVersion}/src/"
+        --replace "a/" "a/bundle/nginx-${nginxVersion}/" \
+        --replace "b/" "b/bundle/nginx-${nginxVersion}/"
     '';
 
   buildInputs = [ postgresql ];

As a general comment, I think it would be better to remove the directories in a postInstall phase than to patch the nginx build system to not create them at all; the latter is more brittle for updates, forks and variants, and should be handled upstream if possible, as @flokli suggested.

This PR definitely needs a changelog entry, and we need to think about how migration paths should look like.

At least the awstats NixOS module, and the service-runner NixOS test seem to contain references to the /var/spool/nginx, which would need to be updated - possibly more.

@GrahamcOfBorg test nginx
@GrahamcOfBorg test nginx-etag
@GrahamcOfBorg test nginx-pubhtml
@GrahamcOfBorg test nginx-sso

@GrahamcOfBorg test nginx
@GrahamcOfBorg test nginx-etag
@GrahamcOfBorg test nginx-pubhtml
@GrahamcOfBorg test nginx-sso

@GrahamcOfBorg test service-runner

@GrahamcOfBorg build openresty

While I completely agree with this change I've had encounters with people in the nix community who feel very strong about not removing stateDir like options so I'm curious how well received this change will be after it is merged and starts hitting people's servers :man_thinking:

I'm not an nginx user so there is only so much I can say about this specific change, but I'll reiterate that the overall idea of removing stateDir like options has not been well received in the past, and I expect that this change will cause a fair bit of frustration from some users. Just because you or I agree with a change doesn't mean we have consensus and should proceed.

I think this change represents a larger idea that hasn't received enough discussion from all parties impacted by it and merging prematurely wasn't the best way to proceed. At this point I guess we can just wait and see... time will tell how well accepted or not this change is.

Apologies if this is a simple question but as I'm not well versed with nginx / systemd I'm hoping someone might be able to help - is the log directory location guaranteed to be stable ie. always present under /var/log/nginx? Can I simply safely hardcode path in places where I previously used the value of the stateDir option? Sorry if I missed a note / changelog entry somewhere on how to deal with the removal of the option.

Apologies if this is a simple question but as I'm not well versed with nginx / systemd I'm hoping someone might be able to help - is the log directory location guaranteed to be stable ie. always present under /var/log/nginx? Can I simply safely hardcode path in places where I previously used the value of the stateDir option? Sorry if I missed a note / changelog entry somewhere on how to deal with the removal of the option.

Yes it's now in /var/log/nginx and /var/cache/nginx always.