modules/performance/byte-compile-lua: support compiling directories and fixed-output derivations

[stasjok]

This PR:

• re-enables modules/performance tests disabled in [main] Update flake.lock & generated files  #3562
• fixes byte-compiling of fixed-output derivations, e.g. fetchurl
• adds ability to byte-compile directories added in modules/extraFiles: refactor to use symlinks and support directories #4016

Note, that for now it supports only simple directories like ./dir. It doesn't support derivations, like fetchFromGitHub.

[MattSturgeon]

• adds ability to byte-compile directories added in

I was recently considering a new approach to byte-compiling extraFiles, which might tie in nicely with another potential refactor to demote init.lua to being "just another" extraFile.

---

Essentially, we add build.extraFiles.passthru.byteCompiled, which is effectively a symlinkJoin of build.extraFiles but every lua symlink gets replaced with a byte-comp version.

(psudocode):

passthru.byteCompiled = symlinkJoin {
  paths = [ finalAttrs.finalPackage ];
  postBuild = ''
    lua_files=("$out"/**/*.lua)
    byte_compile_and_replace "''${lua_files[@]}"
  '';
}

Alternatively, instead of a bash array of lua symlinks, we could:

1. construct an associative array of out-path -> symlink target
2. remove all lua symlinks (array keys)
3. byte-compile all targets (array values)
4. write the results to the respective out paths (array keys)

Or whatever variation on the concept that ends up working best.

---

Once we have this build.extraFiles and its passthru.byteCompiled wrapper, we can then select which one to include in the actual rtp: if config.performance.byteCompileLua.enable then config.build.extraFiles.byteCompiled else config.build.extraFiles

[MattSturgeon]

Thanks for addressing these issues!

On my initial read through, I don't see any obvious technical issues with the changes. However, I do feel like the issues we're needing to solve highlight how we could benefit from a simpler, more delayed approach.

I'd appreciate you're thoughts on my above concept; whether you think the concept is better than the current architecture; whether you'd like to implement something like that; whether you'd like to continue with this PR (first/instead); whether you'd be interested in reviewing if someone else worked on the idea; etc

[MattSturgeon]

Fetchers doesn't execute postFixup hooks, therefore byteCompileLuaHook
doesn't work. And even if it would work, fetchers are fixed-output
derivation which has outputHash known beforehand. We can't override such
derivations using byteCompileLuaDrv.
This commit uses byteCompileLuaFile for fixed-output derivations.

Looking at this with fresh eyes, I'm questioning why we are overriding derivations at all; it's usually better to wrap them (e.g. with symlinkJoin) and then mess with the outputs in a fresh derivation instead of rebuilding the original derivation.

The exception being when the wrapped derivation and the wrapper derivation essentially need to do the same compilation step anyway.

I'm not keen on how we end up handling paths, FODs, drvs, files, directories, (etc) differently from one and other.

---

My proposal in #4151 (comment) would mean dropping the finalSource option entirely and handling this much later, in a wrapper of build.extraFiles.

[MattSturgeon]

This commit adds support for byte-compiling path directories. Only paths
like ./dir are supported. Directory derivations e.g. fetchers are not
supported.

Again, my instinct is that delaying byte-comp until during/after building build.extraFiles, as per #4151 (comment), would massively simplify this. We wouldn't need any eval-time inspection. Instead all inspection of "symlink, file, or directory" would be done in the build script.

[stasjok]

Essentially, we add build.extraFiles.passthru.byteCompiled, which is effectively a symlinkJoin of build.extraFiles but every lua symlink gets replaced with a byte-comp version.

There is one caveat. build.extraFiles aren't used when wrapRc = false (home-manager default, nixos default disabled, but can be enabled). In that case extraFiles are propagated to the corresponding xdg.config options skipping build.extraFiles.

I guess if the handling of runtimepath is changed (some of the discussion was in #1920), it could be the thing. For me it doesn't matter whether it passthru.byteCompiled attr or yet another "final" option.

There is another thing. The code need to be much smarter than

nixvim/lib/builders.nix

Lines 129 to 137 in 03a6382

	while IFS= read -r -d "" file; do
	tmp=$(mktemp -u "$file.XXXX")
	# Ignore invalid lua files
	if ${luajit} -bd -- "$file" "$tmp"; then
	mv "$tmp" "$file"
	else
	echo "WARNING: Ignoring byte compiling error for '$file' lua file" >&2
	fi
	done < <(find "$out" -type f,l -name "*.lua" -print0)

This simple approach doesn't handle symlinks to directories (which symlinkJoin produce). Technically symlink could point anywhere, so it need to resolve all links, verify that they point to the nix store, replace directory-simlinks with the actual directories and file-symlinks in them etc.

Looking at this with fresh eyes, I'm questioning why we are overriding derivations at all; it's usually better to wrap them (e.g. with symlinkJoin) and then mess with the outputs in a fresh derivation instead of rebuilding the original derivation.

Originally this was implemented like this for optimization. In the build time, overridden derivation needs to be built once. With symlink it needs to be built twice: one derivation for source (converted from text, or any use-provided derivation), and another derivation for finalSource (which has a dependency on the source derivation). In the runtime, extra symlinks adds a tiny overhead for resolving them compared to regular files.

For this reason I wanted to determine if byte-compilation is actually needed during evaluation (also as a precaution, who knows what can be in source). This was simple enough when extraFiles were actually files. Now when it can be directories, it's not simple. It's impossible to determine if the directory needs compilation without IFD.

my instinct is that delaying byte-comp until during/after building build.extraFiles, as per #4151 (comment), would massively simplify this

Working on this, I already thought that the current approach is limiting. I don't like this if monstrosity. And it was already an idea that we'll have to switch to build-time byte-compilation at some point. I opted to the most direct/simple approach for now.

To move forward, e.g. supporting byte-compilation of any directory-like path/string/derivation, I think the only option is to write a new lib.builders that would be smart enough to handle directory-symlinks and use it without conditions, effectively dropping optimizations I described above (I didn't resolved on that yet).

byteCompileLuaDrv can be used too, but it doesn't work for symlinked directories (usually it's a rare case like symlinkJoin). Technically this already passes all test cases:

          finalSource =
            # Byte compile lua files if performance.byteCompileLua option is enabled
            if byteCompileLua then
              builders.byteCompileLuaDrv (
                pkgs.stdenvNoCC.mkDerivation {
                  name = derivationName;
                  src = config.source;
                  phases = "installPhase fixupPhase";
                  installPhase = ''
                    if [ -d $src ]; then
                      cp -r --no-preserve=mode $src $out
                    else
                      cp --no-preserve=mode $src $out
                    fi
                  '';
                }
              )
            else
              config.source;

I'd appreciate you're thoughts on my above concept; whether you think the concept is better than the current architecture;

I don't think there is much difference at what point it's compiled. Originally I searched for the single place to add my changes, so all other places would work without changes. Same for passthru attr/another option. I don't particularly favor one "storage" for the compiled derivation over another.

whether you'd like to implement something like that;

I think I won't be doing a big refactoring. If build.extraFiles would be the only possible source for files (see home-manager), then it would be simple to move compilation there. Until that current finalSource approach looks simple enough.

whether you'd like to continue with this PR (first/instead); whether you'd be interested in reviewing if someone else worked on the idea; etc

Currently I think this PR is a simple fix. The thing I'm not sure about is this complicated if statement. It can be dropped in favor of the code above with the potential of improving/rewriting of byteCompileLuaDrv. This will simplify the code at the cost of small performance overhead, but it will support other directory sources.

[MattSturgeon]

There is one caveat. build.extraFiles aren't used when wrapRc = false (home-manager default, nixos default disabled, but can be enabled). In that case extraFiles are propagated to the corresponding xdg.config options skipping build.extraFiles.

Yes, I hadn't thought about this when initially considering the idea, but quickly discovered it when experimenting locally.

I think there's a few ways to handle this:

1. Drop the idea of propagating extraFiles to the host
   One alternative being to install an entire directory to ~/.config/nvim.
2. We could map over extraFiles while propagating, applying a lua builder to the sources.
   If the lua builder handles directories, then it could be the same one used for build.extraFiles.
   This does mean some duplication of impl (although the host-extraFiles-propagation is already a duplication in a way).

This simple approach doesn't handle symlinks to directories (which symlinkJoin produce). Technically symlink could point anywhere, so it need to resolve all links, verify that they point to the nix store, replace directory-simlinks with the actual directories and file-symlinks in them etc.

Yes, I'd already considered this but wanted to go over high-level concepts before discussing details 🙂

My idea would be to recursively copy the base drv's output tree into the wrapper's $out, following symlinks-to-directories recursively (checking for loops and broken links). When reaching non-directory (leaf) nodes, it'd either symlink to the file or byte-compile it and install the result into $out.

This could work as a new builder in lib.builders, something like lib.builders.byteCompileTree?

Originally this was implemented like this for optimization. In the build time, overridden derivation needs to be built once. With symlink it needs to be built twice: one derivation for source (converted from text, or any use-provided derivation), and another derivation for finalSource (which has a dependency on the source derivation).

This makes sense in the typical case, where source is a trivial derivation (e.g. writeText). If someone decided to assign an expensive derivation to source, then wrapping becomes cheaper than overriding, but I appreciate that is unlikely when we're mostly dealing with text (lua/vim) files.

Currently I think this PR is a simple fix. The thing I'm not sure about is this complicated if statement. It can be dropped in favor of the code above with the potential of improving/rewriting of byteCompileLuaDrv. This will simplify the code at the cost of small performance overhead, but it will support other directory sources.

Is that something you'd like to explore here or in future work?

I think I won't be doing a big refactoring.

Sure. I'll consider looking into it after this PR then, if it's still bothering me by then.

[stasjok]

Currently I think this PR is a simple fix. The thing I'm not sure about is this complicated if statement. It can be dropped in favor of the code above with the potential of improving/rewriting of byteCompileLuaDrv. This will simplify the code at the cost of small performance overhead, but it will support other directory sources.

Is that something you'd like to explore here or in future work?

To be honest, I'm fine with it as is. But if you think that the logic became too complicated, I can change it to something like above. Though I'm not a fan if all non-lua files will be symlinks (since most files are generated from source text, I think it's better to generate them directly in target).