Isn't this condition always true?

Looks like it. This check is in the existing module, and I hadn't looked closely at it, apart from updating the directory ref.

To get the docs rendered as intended, these spaces need to have a

</para>
<para>

Also applies to the other options where this is used.

All these should be replaced with the dedicated <option>someOption</option> xml annotation (in all options).

I think there's <command></command> for that

I think it would be better to just use types.nullOr, null signifying all available processors. Also why is this number limited to exactly 44738?

This is the valid range of values for the option as described by the configuration setting. I chose an enum to make it easy to understand what the actual Neo4j config setting defaults to. Using nullOr with mention of what the null means in the docstring works too, I just thought using the enum would be a more direct way of saying so.

Or even: Use the integer 0 to represent all available ones. Nix does the same with the --cores argument / nix.buildCores option.

I think this module is a good candidate to convert to a semantic config thing, similar to how I did it in #41467. The idea is to represent the full config as a Nix value, so something like

services.neo4j.config = {
  dbms.jvm.additional = [
    "-XX:+UseG1GC"
    ...
  ];
  dbms.shell.enabled = true
}

Then use a function to convert this to a neo4j readable config file. The advantages are:

• Users can easily override whatever they want instead of being bounded to the given config file
• The config gets a free syntax check
• More modular via NixOS module system, merging of values
• This module gets cleaner, less stringy

I'll gladly help with getting this to work

It's not necessary, but I'm very much in favor of doing this.

I think this module is a good candidate to convert to a semantic config thing

For the entire module or just the extraServerConfig option?

The entire config file, instead of using giant strings to declare the config, you'd use nix values. See this as an example of how assigning default values would look like.

This __policyname__ replacement business isn't really necessary, because in the end you're gonna have the name at the definition anyways:

services.neo4j.policies.foo = {
  trustedDir = "/some/foo";
}

I'll comment below how you can use the policy name in the options default value without having to use this replacement.

I figured there was a way but couldn't pinpoint it..

To get rid of the __policyname__ thing, use type = with types; attrsOf (submodule ({ name, ... }: { in line 354, and add a section here like this:

config = {
  trustedDir = mkDefault "${cfg.directories.certificates}/${name}/trusted";
  ...
}

And remove the defaults declared in the options.

The Nix manual wraps all description fields with a <para></para> already, so you can (and have to) remove the first <para> and the last </para> to make it work. Use nix-build nixos/release.nix -A manual.x86_64-linux to check that the manual builds. It also produces result/share/doc/nixos/options.html you can view in a browser to see what the rendered description looks like.

Currently, when the user sets baseDirectory of some policy, the revokedDir still uses the "old" default base directory ("${cfg.directories.certificates}/${name}"). To have it use the policies baseDirectory, you can add config to the submodules arguments (next to name), and use default = "${config.baseDirectory}/revoked"

Same for trustedDirs

Nitpick: space missing

What allowed values are there for this? Is it just "legacy" and the ones defined in the policies option? If so, you could add an assertion that checks this (can't use types.enum, because it depends on the users config), something like the following in this modules config section:

let policies = ["legacy"] ++ map (p: p.name) cfg.policies; in
...
assertions = [{
  assertion = elem cfg.bolt.sslPolicy policies;
  message = "Invalid `services.neo4j.bolt.sslPolicy`, valid ones are: ...";
}]

Although, this might not work 100% reliably because of the extraConfig option (since if policies can be declared there, this module won't know about them).

The same for the other options that use policies.

Added a few assertions.

Nixpick: enable should be the topmost option. (sidenote: The manual also uses this special ordering rule when building the manual, so you'll always find enable options at the top)

Also, you could use mkEnableOption "Neo4j Community Edition"

You could remove those parens because function application has a higher precedence than the ! operator

I didn't think of this assertion, very nice :o

There's a nice list of all precedences in the wiki: https://nixos.wiki/wiki/Nix_Expression_Language#Operators :)

This needs to be adjusted to the options for the certificate, data, import and plugins paths. Probably just not creating those when they're not the default values.

A neat way of knowing whether the default value is used (without having to do string comparison) is to add options to the argument list at the top, then checking options.services.neo4j.directories.certificates.highestPrio >= 1500. This value 1500 comes from

A filter for that over builtins.attrValues options.services.neo4j.directories should then get you a list of directories to create. (use .value to get the value of an option, config.some.option == options.some.option.value)

Shouldn't this be the policies directory?

Also, you can use the same highestPrio thing here to not have to even do this string comparison.

you can use the same highestPrio thing here

How does one access an attribute within the policy without knowing its name?
options.services.neo4j.ssl.policies.?.baseDirectory.highestPrio

filter builtins.attrValues options.services.neo4j.ssl.policies based on (opt: opt.baseDirectory.highestPrio < 1500), then an if on whether there are any results at all, then a concatMapStringsSep " " over (opt: opt.baseDirectory.value).

@infinisil

filter builtins.attrValues options.services.neo4j.ssl.policies based on (opt: opt.baseDirectory.highestPrio < 1500)

This doesn't work because options.services.neo4j.ssl.policies is an option itself, and the attrValues of options.services.neo4j.ssl.policies are not options as you expected. A trace of the attrNames is:

[ "_type" "declarations" "default" "definitions" "description" "files" "highestPrio" "isDefined" "loc" "options" "type" "value" ]

The options attribute of this set sounded promising, but alas is empty.

I attempted to access options of a specific defined policy by first collecting the policy names, which also fails, i.e. options.services.neo4j.ssl.policies."name" results is attribute 'name' missing, even when the 'name' policy is defined.

The current push incorrectly uses the definitions attribute, with the assumption that any option not defined therein must still be at its default. Incorrectly in that it doesn't merge all attrsets in definitions before checking for this assumption (naively chooses the first definition attrset).

Of course it's Nixos' job to merge these definitions, but I still cannot figure out how/where to access a policy's option attrset.

I think it would be better to remove all toString functions from all those directories. How it is now would make certificates = ./path/to/certs behave as if you specified "/absolute/path/to/certs", but what one would expect (just from what all other modules do) is that ./path/to/certs gets imported into the store and result in "/nix/store/<hash>-certs". toString can still be applied from the user if it's what they need like this.

@infinisil I'm not understanding something about the list of option values as generated here.

while evaluating anonymous function at /home/pattern/repos/nixpkgs/nixos/modules/services/databases/neo4j.nix:570:49, called from undefined position:
value is a list while a set was expected, at /home/pattern/repos/nixpkgs/nixos/modules/services/databases/neo4j.nix:570:54

I thought builtins.attrValues options.services.neo4j.directories would return a list of the sets defined in options.services.neo4j.directories

I think I'm on the right path, but am stuck.

Ah I got it, this is a fix:

diff --git a/nixos/modules/services/databases/neo4j.nix b/nixos/modules/services/databases/neo4j.nix
index bdf6f44dd1d..50c386ce9b6 100644
--- a/nixos/modules/services/databases/neo4j.nix
+++ b/nixos/modules/services/databases/neo4j.nix
@@ -567,11 +567,11 @@ in {
       validPolicyNameList = ["legacy"] ++ policyNameList;
       validPolicyNameString = concatStringsSep ", " validPolicyNameList;
 
-      defaultDirectoriesList = builtins.filter (dir: dir.highestPrio >= 1500) (builtins.attrValues options.services.neo4j.directories);
+      defaultDirectoriesList = builtins.filter (dir: isOption dir && dir.highestPrio >= 1500) (builtins.attrValues options.services.neo4j.directories);
       certDirIsDefault = options.services.neo4j.directories.certificates.highestPrio >= 1500;
       defaultPoliciesList =
         if !certDirIsDefault then []
-        else builtins.filter (pol: pol.baseDirectory.highestPrio >= 1500) (builtins.attrValues options.services.neo4j.ssl.policies);
+        else builtins.filter (pol: isOption pol && pol.baseDirectory.highestPrio >= 1500) (builtins.attrValues options.services.neo4j.ssl.policies);
     in
 
     mkIf cfg.enable {

The reason for this is that options.services.neo4j.directories actually also includes a value for _definedNames, which is a list used by the module system probably, only just now realized this, sorry! The fix just makes sure that all values are options via isOption.

Admittedly this is a bit ugly, but imo that's better than the alternative (explicitly repeating code for all 5 directories)

@infinisil highestPrio doesn't seem to exist on these sets.

defaultDirectoriesList = builtins.filter (
  dir: (builtins.trace dir dir).highestPrio >= 1500
) (builtins.filter (dir: isOption dir) (builtins.attrValues options.services.neo4j.directories));

trace: { _type = "option"; declarations = <CODE>; default = <CODE>; definitions = <CODE>; description = "Directory for storing ..."; files = <CODE>; isDefined = <CODE>; loc = <CODE>; options = <CODE>; type = <CODE>; value = <CODE>; }

...
attribute 'highestPrio' missing, at /home/pattern/repos/nixpkgs/nixos/modules/services/databases/neo4j.nix:574:14

Cannot reproduce: https://gist.github.com/8f954a39abcdf0f9e4903057134a6f69

I'm using the latest commit from here

@infinisil This appears to be due to my building against a branch made from my local nixos-version (18.03 stable). Looks like these commits made re: highestPrio 4017fdc and 441796c are found in nixos-unstable, but not 18.03. I will work to build agains unstable instead. I suppose this technique would then not be compatible for backport to 18.03?

Updates won't get backported anyways.

An incorrect naive solution. To be replaced.

There's concatMapStringsSep :)

The outer pair of parens can be removed