nixos/nextcloud: Add option to pre-compress assets

[Mynacol]

Description of changes

The schema is similar to Mastodon's packaging script, gnerating gzip and brotli compressed files ahead-of-time.

The find command only selects those folders that are publicly viewable according to the nginx config, and only compresses file types that are allowed in the nginx config.
The index.html and robots.txt files are too small to be relevant.

The gzip files are used through the default nginx config, brotli requires the administrators to set services.nginx.recommendedBrotliSettings = true.

Space changes

560MB /nix/store/i6b1aqrbjgvx6jc8psv1kcyhkayqg8xx-nextcloud-27.0.0 (previous)
683MB /nix/store/h2ai1zkdqp280xv4wp7xrfn4ax86l9xf-nextcloud-27.0.0 (new)

Bandwidth savings

I observed the login page with Firefox. The server has services.nginx.recommendedGzipSettings and services.nginx.recommendedBrotliSettings enabled.
16.53MB uncompressed (according to Firefox dev tools)
3.94MB on-the-fly compressed (old)
1.64MB ahead-of-time compressed (new)
That's a 2.4x compression improvement!

Compute savings

No measurements for CPU compute savings on the server side, but we avoid compressing the same files over and over again. Helps the environment 🌍

Brotli and gzip should be equally fast at decompression irrespective of compression level.

Build time increase

The build time increases by 15-23 minutes. Most of this can be attributed to zopfli. Only brotli (and standard gzip) would take more like 4 minutes.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/): tested nextcloud26 in production
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[Mynacol]

Previous discussion at Mastodon

And ping @onny

[ajs124]

This will break the integrity check, no?

[Ma27]

I'd strongly prefer to have the compression artifacts moved to another output (i.e. to make this opt-in).

Most of this can be attributed to zopfli

Please elaborate why we also create zopfli artifacts in addition to gzip and brotli.

[Mynacol]

This will break the integrity check, no?

I haven't thought of that, but my Nextcloud (with this PR) doesn't complain in any way. Probably because we just add new files alongside the unmodified files.

Most of this can be attributed to zopfli

Please elaborate why we also create zopfli artifacts in addition to gzip and brotli.

Zopfli is not another algorithm, but a special compressor for gzip trading high computational resources for a slightly better compression ratio.
I checked again for some numbers:

	file size	compression time
gzip	77.5M	56s
zopfli	76.0M	9min 39s
brotli (for comparison)	68.6M	2min 55s

While this seems like a bad tradeoff at first, it might be worthwile for resources being transferred millions of times. Zopfli is useful the earlier in the deployment chain it's applied, so ideal for nixpkgs.

I'd strongly prefer to have the compression artifacts moved to another output (i.e. to make this opt-in).

That would indeed be preferable. I updated the PR accordingly.
To me it's just unclear how we integrate that with the Nextcloud service. Should we add another option, and then using symlinkJoin to create a combined document root? Should I add this to another PR?
This is my first time working with multiple outputs, bear with me.

[Ma27]

Should we add another option, and then using symlinkJoin to create a combined document root?

Rough idea: enableCompressedAssets-option. Not sure if that works, but ideally one could simply do a try_files then which looks in nextcloud.compressed first and then in nextcloud.out.
Otherwise, you may wanna take a look at how gitea.data-compressed works and then adapt it accordingly here.

[Mynacol]

I just added a commit introducing the requested option. I opted to use symlinkJoin instead of the try_files method because it reduces the amount of places we need to change and think about this option in the future.

(My last test didn't include the compressed output, but all the previous did. I have no time right now but wanna confirm everything later)

This will break the integrity check, no?

At first, my Nextcloud didn't complain, but now it does...
Should we disable integrity check via the config if enableCompressedAssets is true?

I also thought about splitting the brotli and gzip outputs in two and using two options to selectively enable them, but I'm hesitant adding more configuration options than needed. On the other hand we enable transparent gzip compression regardless of user preference, so we might include these artifacts in all cases, but leave brotli separate.

[Ma27]

Random thought: if we only use the symlinkJoin for assets, but the "correct" package for the PHP stuff then we could "fix" the integrity check errors, correct?

[SuperSandro2000]

Please elaborate why we also create zopfli artifacts in addition to gzip and brotli.

Yes, it will because it complains about the new files.

(I just wanted to attach a screenshot but somehow the error is now not appearing. Not sure why but that already happened in the past for no reason and after some time it appeared again.)

[Mynacol]

I tried to implement the recommendations today, but failed at a specific line, lndir ${gitea.data}/ $out/.

In Nextcloud I edit a function that generalizes over the different versions (nextcloud 25, 26, 27). How can I reference the output of the correct version? (Using services.nextcloud.package is not possible, as package building should be service config independent, correct?)

[SuperSandro2000]

you probably want to implement the change here https://github.com/NixOS/nixpkgs/blob/master/pkgs/servers/nextcloud/default.nix#L14

[Mynacol]

@ofborg build nextcloud29
(idk why ofborg won't build it automatically)

[Ma27]

I think this should use #292324 as soon as it's ready.

[SuperSandro2000]

Linked PR got merged

[SuperSandro2000]

I've used the following in my config:

      package = let
        # hacks around hardcoded caBundle override from
        # https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/web-apps/nextcloud.nix#L13-L15
        pkg = (pkgs.compressDrvWeb (pkgs.nextcloud31.override {
          inherit (config.security.pki) caBundle;
        }) {
          extraFindOperands = ''-not -iregex ".*(\/apps\/.*\/l10n\/).*"'';
        }).overrideAttrs ({ passthru, ...}: {
          passthru = passthru // {
            override = _: pkg;
          };
        });
      in
        pkg;

[Mynacol]

I finally got time to look at this PR again. compressDrvWeb definitely makes things simpler. I however have my doubts about the correct integration into nextcloud.

• Applying the compression at the nixos module is easy but misses my point opening this PR – the compressed version would not be built by hydra. ❌
• Unconditionally wrapping around the package(s) annoys people changing something with the packages, as this always requires a rebuild with compression. ❌
• The proposed solution, moving the compressed artifacts into passthru leads to doubts on my side. Wouldn't people get confused if they change the package, but when forgetting to turn off compression, still get served unmodified assets? A detection/warning in that case would be good. Otherwise, this solution is doable. ❓️
• Another method I considered: Adding pkgs.nextcloud31-compressed package next to pkgs.nextcloud31 (and for v30). This way, people changing stuff either use the uncompressed version anyway, or also trigger a recompress on the compressed version. In some way, this is the best of all worlds. The only downside I see: Cluttering the pkgs namespace and a tiny additional maintenance effort when new major versions are added. ❓️

I get the feeling that @SuperSandro2000's overriding of passthru.override serves an important purpose, but I don't quite get what.

I made a new experimental draft at #432093. My remaining issue is that using passthru.compressed, a new derivation is used and the base derivation arguments cannot easily be overridden.
I also tried experimenting with lib.extendDerivation, but that didn't really help either.

[Ma27]

I must say a of this back and forth and the fact that this will add more complexity to Nextcloud and others makes me heavily question the gain of pre-compressed artifacts over letting nginx do it.

[Mynacol]

Alright, this package is (understandably) just too complex to just do what I had in mind. To get things forward, I significantly reduced the scope for now. This time, the compression is not happening in the package, which would allow Hydra to do the work for all users, but in the nixos module. This makes the code almost trivial and users can just enable compression without having to do any overrides.

This version has no problem with people modifying the underlying package because it is the last step before finalizing the webroot. Either the original or the modified package will be compressed if the option is enabled. If not, no compression is done.
This cannot break any changes on the package, as it is "wrapped" by compressDrvWeb in a later step.

Now we can discuss:

• Whether to enable this by default.
• Whether to add a release note.
• Whether and how to modify the parameters to compressDrvWeb to include/exclude filetypes, directories or compressors.

And for some assurance:
@ofborg test nextcloud

---

When someone else tries to push the compression to a point earlier in the chain, they have to cope with above mentioned problems.

Some additional notes:

• Would it make sense to add the nextcloud nixos tests to the nixos module's passthru.tests (so ofborg will run them on any change here)?

[Ma27]

Did anybody actually measure if the improvements are noticeable?

CCing the actual Nextcloud maintainers as well: @bachp @britter @provokateurin @dotlambda

Would it make sense to add the nextcloud nixos tests to the nixos module's passthru.tests (so ofborg will run them on any change here)?

This is already the case for quite a while though?

[Ma27]

Am I getting it right that we're serving storing the same data compressed in several formats now by default?

[Mynacol]

Yes. compressDrvWeb by default compresses for gzip, brotli and zstd.
I wonder where we should draw the line between flexibility and option overload.
How would you think about the option type being a list of enum gzip, br and zstd? Or we leave it as-is, either compression for all three or none.

[Mynacol]

Did anybody actually measure if the improvements are noticeable?

At least transfer size wise is it noticeable. See the original post for (old, but probably comparable) numbers.

This is already the case for quite a while though?

In the package, the nixos tests are linked. But I don't see that for the nixos module? After I learned you can add e.g. meta.maintainers to nixos modules I assumed you can add tests to nixos modules as well (also based on ofborg's output).

Thanks for notifying the actual maintainers, I missed that!

[Mynacol]

I think this is in shape to be re-reviewed. The change is now really compact. Mentioning the Nextcloud maintainers to ask them for a review: @bachp @britter @dotlambda @Ma27 @provokateurin. Edit: also @SuperSandro2000 which previously reviewed this PR already.

I already pointed out potential remaining questions:

• Whether to enable this by default.
• Whether to add a release note.
• Whether and how to modify the parameters to compressDrvWeb to include/exclude filetypes, directories or compressors.
• Whether to offer more fine-grained options to only provide e.g. gzip compression.
• The exact variable and documentation wording.

[Ma27]

At least transfer size wise is it noticeable. See the original post for (old, but probably comparable) numbers.

First of all, no measurement of the timings, i.e. what the actual impact is when using on-the-fly compression. Then, what are the compression parameters used for on-the-fly vs. build-time compression (and the timing-differences when using actually equivalent configuration)?

Btw, what's the reason for doing this in nixpkgs instead of when building the upstream tarball?

The one thing I'm interested in is that this isn't the default considering the build-time compression speeds this will make testing cycles way more annoying.

I'll defer the decisionmaking to someone else from the maintainer team though.

[provokateurin]

I'm not really convinced by this change. Browsers already cache assets and you can also configure nginx to cache assets (but you can easily break your instance on upgrade if your cache invalidation is not working), so the amount of on-the-fly compression should not be high. Even though it's optional, IMO this adds complexity without much benefit. You can also easily have this in your NixOS configuration by overriding the services.nextcloud.package option without needing this change in the module. So I'm not in favor of merging this, but it could be added to the docs instead.

[Mynacol]

no measurement of the timings

so the amount of on-the-fly compression should not be high [with e.g. nginx caching]

I'm not sure what the correct way to fairly compare timings is. The pre-compressed method has by definition (almost) no overhead. On my laptop with 8 cores, compressing the whole tarball with gzip -4 (the default on-the-fly setting) takes about 9 seconds. Doing it single-threaded takes 40 seconds.
But the advantage of ahead-of-time compression is not primarily in timing, but transfer size (hence moderate (4) vs. high (best/9) compression ratios are used). An improvement of factor 2.4 is great (number in the original post). I can re-do the transfer size comparison with a recent version, but it will probably be comparable to the numbers from two years ago.

what are the compression parameters used for on-the-fly vs. build-time compression

On-the-fly: The nextcloud nginx config in NixOS always enables gzip compression with level 4. More broadly, services.nginx.recommendedGzipSettings uses level 5, services.nginx.recommendedBrotliSettings uses level 5 and services.nginx.recommendedZstdSettings uses level 9.
Pre-compressed: compressDrvWeb by default uses zopfli for gzip, which produces better results than gzip best/level 9, brotli with the level 11, and zstd with level 19.

Browsers already cache assets

Except my browser purposefully deletes caches on exit for privacy reasons. That's a me thing tho.

that this isn't the default considering the build-time compression speeds this will make testing cycles way more annoying.

If y'all want to, we can disable it by default, no problem. However, I believe the overwhelming majority won't have any testing cycles at all while profiting from the smaller transfer sizes. A release note is a good idea anyway.

what's the reason for doing this in nixpkgs instead of when building the upstream tarball

Good question. I didn't ask the upstream project. I reckoned this is in the working field of a distribution, tweaking and providing upstream projects as fit to the specific distribution.

You can also easily have this in your NixOS configuration by overriding the services.nextcloud.package option

Correct, that's how I use it for years already. However, it is more complicated because of this:

services.nextcloud.package = (pkgs.compressDrvWeb pkgs.nextcloud31 {}).overrideAttrs ({passthru, ...}: {
  passthru =
   passthru
    // {
      override = _: config.services.nextcloud.package;
    };
});

IMO this adds complexity without much benefit

I kept the complexity to an absolute minimum. A single line of implementation code that wraps the webroot once. The package and module as a whole get more complicated, correct, but IMO in a reasonable amount.
Also, I want to remind you that at least the Gitea, Movim and Libretranslate NixOS modules successfully use pre-compressed assets.

it could be added to the docs instead

That would be a great and useful alternative if it is ultimately not merged.

[Mynacol]

@bachp @britter @dotlambda @Ma27 @provokateurin

Alright, this is my final straw. Since my last comment, I rebased this PR and disabled the option by default. I also slightly expanded the option description. Either you take this feature (potentially with some improvements), or I don't see a way to alter it so it suits you.

The remaining motivation for this PR is to avoid people needing to do something like the following:

    package = (pkgs.compressDrvWeb pkgs.nextcloud31 {}).overrideAttrs ({passthru, ...}: {
      passthru =
        passthru
        // {
          override = _: config.services.nextcloud.package;
        };
    });

and instead offer the same with a one-liner:

enableCompressedAssets = true;

@ofborg test nextcloud