feat(wrapperModules.direnv): init

[zenoli]

I think this should cover all configuration options (direnv.toml, direnvrc, and lib/*.sh)
There is still some things left to do, mainly

• Adding documentation and descriptions
• Adding tests
• Add mise integration from home-manager

I also have a question about constructFiles (marked with TODO).

Similar to the issue in the zsh wrapper where adding extraPackages as env vars to the wrapper script did not work, adding DIRENV_CONFIG won't work here as well. I tried to outline why in the comments.

Would be glad to get some feedback on this.

[BirdeeHub]

config.constructFiles.<name>.key

The current convention for this:

nix-wrapper-modules/wrapperModules/h/helix/module.nix

Lines 112 to 126 in d9fdf67

	// lib.pipe config.themes [
	(lib.filterAttrs (_: theme: theme != { }))
	(wlib.mapAttrsToList0 (
	i: name: theme:
	lib.nameValuePair name {
	key = "theme_${toString i}";
	relPath = lib.mkOverride 0 "${config.binName}-config/helix/themes/${name}.toml";
	output = lib.mkOverride 0 config.generatedConfig.output;
	content = if builtins.isString theme then theme else builtins.toJSON theme;
	${if builtins.isString theme then null else "builder"} =
	''mkdir -p "$(dirname "$2")" && ${pkgs.remarshal}/bin/json2toml "$1" "$2"'';
	}
	))
	builtins.listToAttrs
	];

(the mkOverrides in that example are to make sure it remains grouped in the generated directory)

[BirdeeHub]

it only matters for the created value in drv

The name in config.constructFiles.<name> can still be anything, but you have to set .key if it could be an invalid shell variable

Unfortunately, I have not found a way around this yet (without forcing everyone to use __structuredAttrs)

The problem is that "${name}Path" must be a valid shell variable.

nix-wrapper-modules/modules/constructFiles/module.nix

Lines 111 to 112 in d9fdf67

	local sourceDrvVarToConstruct=${lib.escapeShellArg "${name}Path"}
	constructFile "''${!sourceDrvVarToConstruct}" ${lib.escapeShellArg path}

^ I promise I really tried, and I at least got it to give a warning that makes sense.

[BirdeeHub]

Similar to the issue in the zsh wrapper where adding extraPackages as env vars to the wrapper script did not work, adding DIRENV_CONFIG won't work here as well. I tried to outline why in the comments.

Yeah I kinda expected to have some kind of similar trouble with this one... if you figure it out, let me know lol

On first inspection of the comment about it in the code though, it looks like you are fighting with placeholders. Which isnt necessarily the first place I would expect the problem to start.

Maybe using wrapperVariants to wrap something other than the main binary in context of the final wrapper derivation can help you here? Or using buildCommand directly?

I will look into this further when I get time to sit down with it properly.

[zenoli]

Similar to the issue in the zsh wrapper where adding extraPackages as env vars to the wrapper script did not work, adding DIRENV_CONFIG won't work here as well. I tried to outline why in the comments.

Yeah I kinda expected to have some kind of similar trouble with this one... if you figure it out, let me know lol

On first inspection of the comment about it in the code though, it looks like you are fighting with placeholders. Which isnt necessarily the first place I would expect the problem to start.

Maybe using wrapperVariants to wrap something other than the main binary in context of the final wrapper derivation can help you here?

I will look into this further when I get time to sit down with it properly.

The "proper" fix would be to get this PR merged I created but I think the chances are rather slim. I tested this already by rebuilding direnv with that patch.

[BirdeeHub]

Oh, also, unrelated, this discussion, #370 <- thoughts? (there though if you have any). You usually have good input so figured I would ask.

[BirdeeHub]

That PR is not a bad idea actually. It doesn't feel like it is special-cased just for us. I didn't read the whole implementation, to see all the things around the changes, but the idea seems relatively non-intrusive and the tool itself is somewhat intrusive so more escape hatches seems like a good idea.

I will look into the implementation of the wrapper module properly sometime soonish beyond just the constructFiles thing, and see if there is anything we can do on our end too

[BirdeeHub]

Also maybe of interest

BirdeeHub/nixCats-nvim#398

Having a direnv wrapper module would maybe let us pass through stuff like that?

[zenoli]

Also maybe of interest

BirdeeHub/nixCats-nvim#398

Having a direnv wrapper module would maybe let us pass through stuff like that?

hmm... I think as long we don't have the possibility to configure DIRENV_EXE_PATH there won't be much we could do to solve that issue.

As of now, the native, unwrapped direnv binary will be run when entering/exiting .envrc directories. Having DIRENV_EXE_PATH would at least give us some entrypoint to add logic that might prevent the issue you linked. But then again, I'm not sure if we should even do this.

I share your view about not switching directories from within neovim and if I occasionally do I'd rather not want direnv to change the environment so I would not have this issue.

[zenoli]

@BirdeeHub is there a way to run only the checks for a given wrapper?
It would make iterating on tests more convenient as runnig the full check takes too long.

I never used nix flake check. I think I should be able to somehow just pass it the generated test name (I think it would be wrapperModule-direnv) but I struggle to get it to work.

[zenoli]

@BirdeeHub I made a proof of concept on how to write tests in a more flexible and readable way.

Can you quickly tell me if you're ok with this approach? I plan to write more tests this way and want to make sure it does not get rejected :-)

If you think this is useful we can also think about how to make this reusable for other modules.

[BirdeeHub]

is there a way to run only the checks for a given wrapper?

nix build ./ci#checks.<system>.thatone

[BirdeeHub]

@BirdeeHub I made a proof of concept on how to write tests in a more flexible and readable way.

Can you quickly tell me if you're ok with this approach? I plan to write more tests this way and want to make sure it does not get rejected :-)

If you think this is useful we can also think about how to make this reusable for other modules.

It is a good start. I think breaking them up into separate drvs every time they use the helper is probably too much though.

If you want to develop it a bit and help fix up CONTRIBUTING.md maybe we could pass it to the tests

nix-wrapper-modules/ci/flake.nix

Lines 16 to 56 in 364e000

	checks = forAllSystems (
	system:
	let
	pkgs = import nixpkgs {
	inherit system;
	config.allowUnfree = true;
	};
	# Load checks from checks/ directory
	checkFiles = builtins.readDir ./checks;
	importCheck = name: {
	name = lib.removeSuffix ".nix" name;
	value = import (./checks + "/${name}") {
	inherit pkgs;
	self = self;
	};
	};
	checksFromDir = builtins.listToAttrs (
	map importCheck (builtins.filter (name: lib.hasSuffix ".nix" name) (builtins.attrNames checkFiles))
	);
	importModuleCheck = prefix: name: value: {
	name = "${prefix}-${name}";
	value = import value {
	inherit pkgs;
	self = self;
	};
	};
	checksFromModules = builtins.listToAttrs (
	builtins.filter (v: v.value or null != null) (
	lib.mapAttrsToList (importModuleCheck "module") (wlib.checks.helper or { })
	)
	);
	checksFromWrapperModules = builtins.listToAttrs (
	builtins.filter (v: v.value or null != null) (
	lib.mapAttrsToList (importModuleCheck "wrapperModule") (wlib.checks.wrapper or { })
	)
	);
	in
	checksFromDir // checksFromModules // checksFromWrapperModules
	);

^ tests get called from there, pass whatever extra, add extra options for things you can return, as long as the existing ones still run and receive pkgs and self, you can pretty much do whatever, thats why it is in a separate flake, you can even pull an input if you want in that flake.

An extra thing to add, your helper should also check config.meta.platforms for the user.

Maybe we also edit the receiving code in that flake such that it would also be able to return a set of drvs which get combined with the final set of tests, for if you really really want to return a few properly separate tests, that can also be ran separately?

You shouldn't be making so many intermediate drvs and running them all in an outer test drv. If you want actually separate ones, you should add the ability to return multiple. You could then have runTests, which runs several "scripts" within 1 drv, as well.

This is for speed reasons, and because if you are going to have them as separate drvs, you may as well allow yourself to actually run them separately too.

If you do that, maybe as a separate PR would be preferred?

[zenoli]

nix build ./ci#checks..thatone

I swear I tried this yesterday but must have done something wrong. Thanks!

[zenoli]

An extra thing to add, your helper should also check config.meta.platforms for the user.

Good point. Agreed.

Maybe we also edit the receiving code in that flake such that it would also be able to return a set of drvs which get combined with the final set of tests, for if you really really want to return a few properly separate tests, that can also be ran separately?

You shouldn't be making so many intermediate drvs and running them all in an outer test drv. If you want actually separate ones, you should add the ability to return multiple. You could then have runTests, which runs several "scripts" within 1 drv, as well.

Although I agree that introducing the outer test drv is hacky, I nice property of my current solution is, that it gives you a nice explanation what failed:

error: Cannot build '/nix/store/j5k2m7da343jrkz1ng6j8rd9m9cx7xqn-if-nix-direnv-is-enabled-then-lib-nix-direnv.sh-exists.drv'.
       Reason: builder failed with exit code 1.
       Output paths:
         /nix/store/qnxwsi91va9dkf4v70iqg3d9zbjzjk4w-if-nix-direnv-is-enabled-then-lib-nix-direnv.sh-exists
       Last 1 log lines:
       > No such file /nix/store/vjcfmmw41gbshs9fmyp46azy9y266zdr-direnv-2.37.1/direnv-dot-dir/lib/nix-direnv.sh
       For full logs, run:
         nix log /nix/store/j5k2m7da343jrkz1ng6j8rd9m9cx7xqn-if-nix-direnv-is-enabled-then-lib-nix-direnv.sh-exists.drv
error: Cannot build '/nix/store/n0jqmqxblizwncsscww9h2n5jsm4567d-test-group-nix-direnv.drv'.
       Reason: 1 dependency failed.
       Output paths:
         /nix/store/ii33q6yw05jbxz4q25520hggqsjfirb0-test-group-nix-direnv
error: Cannot build '/nix/store/8g72ydmbsqk9yk6sjw4djjlnirxg82hw-test-group-direnv-all.drv'.
       Reason: 1 dependency failed.
       Output paths:
         /nix/store/gf1pjs7cafn4msa83q5q95qvh3iwyvs0-test-group-direnv-all

There was an error in the direnv wrapper (test-group-direnv)...
...specifically around the nix-direnv feature (test-group-nix-direnv)
...specifically in this test "if-nix-direnv-is-enabled-then-lib-nix-direnv.sh-exists"

Maybe this is overkill for a rather simple wrapper such as direnv but i can see this being usefull in the more sophisticated wrappers where we might group related tests around specific features.

That being said, I can live with not having this hierarchical grouping and just be able to define a flat list of tests that can be returned. I was planning on returning a list of drv instead of just a drv but I am not sure whether this is what you suggested in this quote:

You could then have runTests, which runs several "scripts" within 1 drv, as well.

Can you clarify what you mean by this?

This is for speed reasons,

Does this really hurt us? If tests become more readable and mainainers are encouraged to write more tests, longer running tests would be a worthy sacrifice to me. Am I underestimating the negative impact of this?

If you do that, maybe as a separate PR would be preferred?

Yes if we agree on what needs to be done, and this seems to be touching ci/flake.nix then I'd keep the tests here in the "old" style and move it to a separate PR. That's why I decided to ask early as I anticipated this outcome :-)

[BirdeeHub]

If from check.nix you could return

{
  testa = pkgs.runCommand...
  testb = pkgs.runCommand...
}

And have that map to <modname>-<testname>

The warning would be like

warning failed in <modname>-<testname>, which has basically the same properties as your warning, except you would also be able to run it individually too.

You could then have runTests, which runs several "scripts" within 1 drv, as well.

Can you clarify what you mean by this?

pkgs.runCommand {} ''
  ${builtins.concatStringsSep "\n" scripts}
  touch $out
''

but with better error printouts and checks stuff from the wrapper module like meta.platforms as well, and maybe makes the path to it available as $src for each test or something

[zenoli]

{
testa = pkgs.runCommand...
testb = pkgs.runCommand...
}

So this still has multiple drvs. This would be fine with me.

pkgs.runCommand {} ''
  ${builtins.concatStringsSep "\n" scripts}
  touch $out
''

But this is an alternative solution right?
Here you would only produce a single derivation but won't have the benefit of being able to run testa/testb from your above example (assuming these would be two "scripts" you pass to them?)

[BirdeeHub]

Not alternative, we should offer both.

They should be allowed to return a set containing multiple tests, (which would be achieved by editing the code in ci/flake.nix that calls the tests). That would look like that first snippet you mentioned, which would be returned by check.nix

And they should also be allowed to use our helper to nicely build 1 or multiple cases into a single test, with access to your test helpers in the bash code for the test, and checked for meta.platform. Which would look like that second snippet but, with more stuff in it and actually thought out arguments and stuff.

That way they can break them up into separate DRVs, but also have an ergonomic experience creating each individual test which each may contain 1 or more cases.

in short, a check.nix could decide to return multiple, and that would be like

{
  pkgs, self, testlib??
}:
{ # <- first snippet
  testa = testlib.runthetests { ?? }; # <- second snippet
  testb = testlib.runthetests { ?? };
}

And then in the ci/flake.nix you would make that map to <modname>-<testname>

You could also have some kind of recursive import that returns that structure, if you want to add a file-based helper too.

{
  pkgs, self, testlib??
}:
testlib.returntestdir { ?? } ./checks #<- returns a set of tests rather than just 1

Or, if they just want 1 test drv, then they can just do this

{
  pkgs, self, testlib??
}:
testlib.runthetests { ?? }; # <- second snippet, returns just 1 test (or null)

And then everyone benefits from the new helpers, you can still disable particular tests by returning null instead, and you can run them all individually when you break them up into multiple

[zenoli]

For the sake of this PR I came up with a solution that:

• creates and returns a single drv (no more "dummy drv that collects tests")
• Still has the nice error messages for the given assertion that fails, the runTest it occured and the runTests it occured in:

error: Cannot build '/nix/store/s35n2843vpwwal7rl0x71wpizkk9fiwy-direnv-test.drv'.
       Reason: builder failed with exit code 1.
       Output paths:
         /nix/store/z7w7zwav7sp2qrdrkggxblpsq32sws8s-direnv-test
       Last 2 log lines:
       > No such file /nix/store/vjcfmmw41gbshs9fmyp46azy9y266zdr-direnv-2.37.1/direnv-dot-dir/lib/nix-direnv.sh
       > test "if nix-direnv is enabled then lib/nix-direnv.sh should exists" failed
       For full logs, run:
         nix log /nix/store/s35n2843vpwwal7rl0x71wpizkk9fiwy-direnv-test.drv
error: build of '/nix/store/nwfywyx86k10528ijq04qbgl3vkayj9d-formatting-check.drv', '/nix/store/s35n2843vpwwal7rl0x71wpizkk9fiwy-direnv-test.drv' failed

The format is as follows:

runTests "direnv-test" [   # <-- will create a single drv by running `runCommand`
# and transforming the test list and its assertions to a single large script
  (runTest "test-a" (  # <-- a test groups together assertions. If any assertion fails, the test fails.
    [
      (isDirectory "some-directory")   # <-- we provide pre-defined assertions functions
      (isCheckingXyz "some-arg")     # <-- but we can easily define our own using `createAssertion`
      ''[ 1 -eq 0 ] || echo "One does not equal zero" >&2; return 1''  # <-- or write plain bash assertions

    ]
  ))
  (runTest "test-b" (isDirectory (getDotdir wrapper)))   # <-- single assertions will automatically be transformed a list

I would like to merge this directly within the direnv wrapper and refactor it out + add the config.meta.platforms feature if you agree to it.

Having had some time to play around with it, I think I would not extend the ci flake to handle lists or sets of tests but keep that part as it is. Here are some points why:

1. Having the option to run tests per module is more than enough.
   Being able to run nix build ./ci#checks.x86_64-linux.wrapperModule-direnv is instant. The only reason why I can see myself wanting to narrow the scope of tests I run, is if tests become so slow that it gets annoying. This is the case when you run nix flake check -Lv ./ci but this issue is completely gone why running the tests only for a single wrapper.

2. As of now, the "framework" defines the names of the tests. I always know that the tests for a given wrapper will be named wrapperModule-{wrapperName}. If every wrapper returns a list/set of tests, I wouldn't be able to run everything related to the wrapper in a single command. If I have to always look up the name of "that specific direnv test" I can see it get annoying. Also we would need to prefix all tests with the wrapper name to avoid name clashes across wrappers. (We could write a script that runs all tests with a given wrapper prefix to run all tests belonging to a wrapper, but I really don't see a reason for this added complexity for now.

So yeah, let me know if this is worth being moved to the ci flake.
I will finish up the remaining features and documentation next.

[zenoli]

Yes, the run function gets redefined every test but it is working fine (which is why I'd rather use this instead of using the testname (name) because I would have to ensure that the name does not contain chars that cannot be used in a function definition)

[BirdeeHub]

It is fine to redefine a bash function each time you call runTest

Defining a bash function is plenty fast enough.

A bunch of nested derivations would be slower, which is why I asked that either we split them up into actually separate tests, or improve the case where you have many cases within 1 derivation.

Within that derivation you can do basically whatever, although you should still preferably avoid IFD still.

[zenoli]

Within that derivation you can do basically whatever, although you should still preferably avoid IFD still.

What does "IFD" mean?

[BirdeeHub]

import from derivation

https://nix.dev/manual/nix/2.26/language/import-from-derivation

When you take a file from inside a derivation, and use it in nix code rather than another derivation.

[zenoli]

We need to "&&" each assertion to ensure the error state is correctly propagated.

If we simply had newlines here, the error code of the function would be the error code of the last statement only.

[zenoli]

I added documentation, lifted mise integration from home manager and wrote the tests.

There is one more thing that I am a bit lost still and might need some input @BirdeeHub :

Even though env.DIRENV_CONFIG does not work at the moment due to reasons discussed already, I would still want to inject the variable into the wrapper.

The problem is that I cannot simply set it to "${config.wrapper.${config.outputName}}/${config.configDirname}" as I did when setting passthru.DIRENV_CONFIG because I get an infinite recursion bug.

I initially set it to env.DIRENV_CONFIG = "${placeholder "out"}/${config.configDirname}" which does not result in infinite recursion but raises another issue:

I use the direnv wrapper as a subWrapperModule option in my zsh wrapper config. When my zsh config is built, the placeholder points to the zsh wrapper.

Do you have an idea how to solve this?

[BirdeeHub]

The problem is that I cannot simply set it to "${config.wrapper.${config.outputName}}/${config.configDirname}" as I did when setting passthru.DIRENV_CONFIG because I get an infinite recursion bug.

I initially set it to env.DIRENV_CONFIG = "${placeholder "out"}/${config.configDirname}" which does not result in infinite recursion but raises another issue:

I use the direnv wrapper as a subWrapperModule option in my zsh wrapper config. When my zsh config is built, the placeholder points to the zsh wrapper.

Do you have an idea how to solve this?

You would do the first of those, the one you originally were doing for passthru

You would not be able to

env.DIRENV_CONFIG."${config.wrapper.${config.outputName}}/${config.configDirname}"

you can only passthru.DIRENV_CONFIG."${config.wrapper.${config.outputName}}/${config.configDirname}"

This is because the env options need to be evaluated and placed inside the derivation, whereas passthru does not, it just gets // into the resulting derivation at the end.

It is also because attrsOf is not lazy, which env uses, but passthru uses lazyAttrsRecursive for its option type (constructFiles gets around this problem by being a submodule option, so attrsOf doesnt trigger evaluation of .outPath)

And then in your zsh wrapper you would grab the value from passthru however and add it to its env option.

[BirdeeHub]

Everything that relies on placeholder "out" won't work if used as a subwrapperModule.

It will work when used by config.wrapper of that subWrapperModule, or when passed to other options which go there.

But yes, if you use them outside of that submodule, that is the case.

That is why constructFiles also makes an .outPath available, for example. Which can be used from outside the subWrapperModule

Every file made with constructFiles is accessible from outside of the derivation in this way.

# in some other wrapper module or derivation
''
${yourWrappedPackage.configuration.constructFiles.<name>}
or
${yourWrappedPackage.configuration.constructFiles.<name>.outPath}
or
${config.wrappers.<wrappername>.constructFiles.<name>}
or
${config.wrappers.<wrappername>.constructFiles.<name>.outPath}
''

And if you use a different derivation for the DIRENV_CONFIG dir thing, placeholders in there would refer to that derivation. So, they could still use placeholders in the options and if you used those options values from config, those would still fail just like they do now. It would just be less useful to use them, so less people would

Definitely still prefer to have it all be in the final derivation, and make anything extra needed that isnt already handled by constructFiles.<name>.outPath available via passthru

In short, constructFiles is actually the solution to this issue, not the cause of the problem. It just isn't one people often think about, until you actually start trying to refer to values with placeholders from somewhere else and find out it doesnt work.

[zenoli]

Thanks for the explanation.

I don't access any of the constructFiles options of my wrapper in zsh.
And also I think my concern was wrong as now the thing I thought was not working actually works... Maybe I need to look at this again with a fresh head tomorrow.

What I am doing now is exporting env.DIRENV_CONFIG = "${placeholder "out"}/${config.configDirname}"; (even though it currently is useless but might be useful in the future if direnv/direnv#1564 gets merged).

I then integrate the direnv wrapper as a subwrapperModule in my zsh wrapper.
My concern was that if I inspect the gereated direnvWrapper script I would find DIRENV_CONFIG=/nix/store/hash-of-zsh-wrapper instead of the correct hash (because I thought I saw this happening a couple of days ago) but I just tested it again and this issue is not present anymore.

Either I had a bug which unintentionally got fixed or I am halucinating. I think it should be fine the way it is now.

I need to look into this stuff further

I would prefer if we do a big test rework, for us to do it in a separate PR

But for now, its just the 1 file for the test stuff, so, its fine for now.

I don't really use direnv much, I will definitely want to mess with it myself before merging it because this one is likely to have some weird behavior to sort out and is a common tool

No hurry. I will keep using the feature branch for now and also smoke test it in the following days.

[BirdeeHub]

well, so, the stuff in env goes into the file, with the hash of the direnv drv, when it is used by the direnv wrapper.

As long as you access the one you exported via config.passthru.DIRENV_CONFIG or via constructFiles.<name>.outPath or whatever in the zsh wrapper, instead of trying to grab config.env.DIRENV_CONFIG.data directly, then it will have the correct value, because those you are grabbing from config.wrapper, the final derivation

It is just config.yoursubmodule.env.DIRENV_CONFIG.data where it will still be a placeholder. It hasnt been passed to a derivation yet, and its value will never change, it will always be a placeholder. If you used this one in the zsh wrapper, it would point to the wrong place, as the placeholder would be substituted when the zsh wrapper grabs it from the zsh wrapper module option.

The other method of accessing it with constructFiles.<name>.outPath or via passthru where you grab it from the output of config.wrapper should work anywhere outside of the wrapper module but very few places inside it other than passthru

This is because most of the options in a wrapper module are necessary to create config.wrapper, so they can't really use that to construct themselves, hence the infinite recursion you mentioned.

passthru is exempt from that though. Intentionally so, I used wlib.types.lazyAttrsRecursive to make that possible, which was further possible due to stdenv.mkDerivation doing similar intentionally as well. They are truly passed through, they do not affect the derivation hash or anything else. That is why you can export the derivation itself via the derivation's passthru. Submodules are also lazy in that way, which is why the ones from constructFiles also are accessible without infinite recursion as long as you dont try to access outPath from within that wrapper module itself. But you can't then use that config.wrapper value from passthru in one of the other options within that subWrapperModule, because then config.wrapper depends on itself again

[zenoli]

I rebased and removed the key property.
@BirdeeHub Just to be sure:
If I omit key it will default to name and your new sanitization function will be applied to name, correct?

[BirdeeHub]

Correct. And there are 2 phases

First, the option uses .apply to call the sanitization function

However, this cannot prevent duplicates, it only knows about that 1 item

Then when they are collected and added to config.drv, if there are any conflicts, it will add the _0, _1, etc... to them, but only if they conflict