Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DynamicListOf Fixes #157

Merged
merged 29 commits into from
Nov 15, 2021
Merged

DynamicListOf Fixes #157

merged 29 commits into from
Nov 15, 2021

Conversation

lapp0
Copy link
Collaborator

@lapp0 lapp0 commented Oct 27, 2021

@lapp0
Copy link
Collaborator Author

lapp0 commented Oct 29, 2021

As part of this pull request, I'm looking to solve a myriad of issues around Attrs and List parsing.

We would like create a tree with nodes for each option, and child nodes where an option is an attribute or list.

The main problem is that lib.nix doesn't recurse into submodules to get attributes, therefore attributes like 'fileSystems."/".options' are read as fileSystems since (import <nixpkgs/nixos> {configuration={};}).options.fileSystems._type is option, meaning (...).options.fileSystems cannot be recused into any further.

For this reason we have certain attribute paths missing from one of our test cases - they're simply not currently retrieved

# TODO: add these to the expected set when `lib.nix` recurses into submodules
configuration_nix_submodule_attrs = {
'users.extraUsers.isNormalUser',
'users.extraUsers.home',
'users.extraUsers.description',
'users.extraUsers.extraGroups',
'users.extraUsers.uid',
'users.extraUsers.shell',
'users.extraGroups.vboxusers.members',
'services.bookstack.nginx.listen',
'services.unbound.settings.server.cache-min-ttl',
'services.unbound.settings.server.do-tcp',
'services.unbound.settings.server.ssl-upstream',
'services.unbound.settings.forwrad-zone',
'environment.etc."resolv.conf".text',
'hardware.bluetooth.settings.General.Enable',
'fileSystems."/".options',
}
hardware_configuration_nix_submodule_attrs = {
'fileSystems."/".device',
'fileSystems."/".fsType',
'fileSystems."/boot".device',
'fileSystems."/boot".fsType',
'fileSystems."/home".fsType',
'fileSystems."/home".device',
'fileSystems."/home/geir/media".device',
'fileSystems."/home/geir/media".fsType',
}
# TODO: add cases when `lib.nix` recurses into lists, e.g.
# 'swapDevices.[0].device', 'users.extraUsers.sample'.extraGroups.[3]'

Therefore when we must use python (for now) to parse the branch of the syntax tree under which fileSystems is defined. This would be easy, except there are serious edge cases. For example, if we have

fileSystems."/".foo = <definition 0>
fileSystems."/".bar = <definition 1>

our infrastructure will only retrieve the syntax tree node for <definition 0> because that's the only node unsafeGetAttrPos will direct us towards.

Changing lib.nix to recursively get all option (and submodule attribute members) paths

I attempted to make lib.nix recurse through all definitions until either the child definition isn't within the same module OR there is an error. However tryEval couldn't handle certain errors, such as an attribute missing from pkgs. This is a no-go since parsing should be dependent on syntax validity, and not validity under the myriad of different arguments with which a function module can be called, especially considering we're parsing arbitrary function modules, not just configuration.nix.

Changing parser.py to recursively get all attribute paths and expression strings

We already have the infrastructure to recursively get all attribute paths and expression strings given a starting point - the position in the file of the "base level" attribute path. For example, given the location of hardware = {...}, we can get data for every option within that NODE_ATTR_SET.

However if hardware.pulseaudio.enable = ... is elsewhere in the module, it won't be found in this call.

For this approach to work we need to find every instance in which an option definition occurs.

Lazy approach

One way to make this work is collect every definition (NODE_KEY_VALUE) in the syntax tree and recurse for NODE_ATTR_SET to collect each attribute name and corresponding expression string. Then we can compare it against a list of options to filter out any definitions that aren't option definitions.

E.g. If we have a let ... in statement, local variables defined there will be captured, but then filtered out.

This method introduces many edge cases, for example if a variable named hardware is defined locally, it will be treated just the same as the option definition and won't be handled properly.

Less lazy, less prone to failure, but definitely incomplete approach:

Collect all definition nodes in the syntax tree as we currently do using unsafeGetAttrPos. Assume the scope in which a definition exists is a valid scope for other option definitions to exist. Collect parent nodes of all found definition nodes. Recurse through these parent nodes and find option definitions.

This still of course leaves us with edge cases, for example

  let
    bar = "noatime"
  in
    fileSystems."/".options = [ "noatime" "nodiratime" "discard" ];
  let
    foo = "sampledrive";
  in 
    fileSystems."/".label = foo;

In this case, fileSystems is an option, and their parent node is a LET_IN node. Therefore lib.nix / unsafeGetAttrPos will only find one of these options, and our heuristic of checking the parent node will fail.

Summary

None of these solutions are very good, I need to do more thinking.

@lapp0
Copy link
Collaborator Author

lapp0 commented Oct 29, 2021

@adrianparvino any thoughts on whether recursing into submodule attributes is possible within lib.nix?

I wrote this but tryEval doesn't work as I'd like it to.

  collectDeclarationPositions = {module_path, declarations, option_path ? []}:
    if isAttrs declarations
    then lib.concatMap
      # If we can get the children of declaration and they're within the same module, recurse
      (k:
        let
          collectedChildDeclarations = builtins.tryEval(
            self.collectDeclarationPositions {
              module_path = module_path;
              declarations = declarations."${k}";
              option_path = (option_path ++ [k]);
            }
          );
        in
          if collectedChildDeclarations.success # && module_path is same
          then collectedChildDeclarations.value
          else [{
            loc = option_path ++ [k];
            position = builtins.unsafeGetAttrPos k declarations;
          }]
      )
      (builtins.attrNames declarations)
    else [];

Unfortunately tryEval doesn't catch all errors, including this one. Maybe there's a way we can stop the recursion before getting to the failing expression without using (import <nixpkgs/nixos> {configuration={};}).options?

error: attribute 'pulseaudioFull' missing

       at /home/andrew/p/nix-gui/nixui/tests/sample/configuration.nix:151:15:

          150|     enable = true;
          151|     package = pkgs.pulseaudioFull;
             |               ^
          152|     extraModules = [ pkgs.pulseaudio-modules-bt ];

       … while evaluating the attribute 'package'

       at /home/andrew/p/nix-gui/nixui/tests/sample/configuration.nix:151:5:

          150|     enable = true;
          151|     package = pkgs.pulseaudioFull;
             |     ^
          152|     extraModules = [ pkgs.pulseaudio-modules-bt ];

       … while evaluating 'collectDeclarationPositions'

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:11:33:

           10|   */
           11|   collectDeclarationPositions = {module_path, declarations, option_path ? []}:
             |                                 ^
           12|     if builtins.isAttrs declarations

       … from call site

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:18:13:

           17|           collectedChildDeclarations = builtins.tryEval(
           18|             self.collectDeclarationPositions {
             |             ^
           19|               module_path = module_path;

       … while evaluating anonymous lambda

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:15:8:

           14|       # If we can get the children of declaration and they're within the same module, recurse
           15|       (k:
             |        ^
           16|         let

       … from call site

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:13:10:

           12|     if builtins.isAttrs declarations
           13|     then lib.concatMap
             |          ^
           14|       # If we can get the children of declaration and they're within the same module, recurse

       … while evaluating 'collectDeclarationPositions'

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:11:33:

           10|   */
           11|   collectDeclarationPositions = {module_path, declarations, option_path ? []}:
             |                                 ^
           12|     if builtins.isAttrs declarations

       … from call site

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:18:13:

           17|           collectedChildDeclarations = builtins.tryEval(
           18|             self.collectDeclarationPositions {
             |             ^
           19|               module_path = module_path;

       … while evaluating anonymous lambda

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:15:8:

           14|       # If we can get the children of declaration and they're within the same module, recurse
           15|       (k:
             |        ^
           16|         let

       … from call site

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:13:10:

           12|     if builtins.isAttrs declarations
           13|     then lib.concatMap
             |          ^
           14|       # If we can get the children of declaration and they're within the same module, recurse

       … while evaluating 'collectDeclarationPositions'

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:11:33:

           10|   */
           11|   collectDeclarationPositions = {module_path, declarations, option_path ? []}:
             |                                 ^
           12|     if builtins.isAttrs declarations

       … from call site

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:18:13:

           17|           collectedChildDeclarations = builtins.tryEval(
           18|             self.collectDeclarationPositions {
             |             ^
           19|               module_path = module_path;

       … while evaluating anonymous lambda

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:15:8:

           14|       # If we can get the children of declaration and they're within the same module, recurse
           15|       (k:
             |        ^
           16|         let

       … from call site

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:13:10:

           12|     if builtins.isAttrs declarations
           13|     then lib.concatMap
             |          ^
           14|       # If we can get the children of declaration and they're within the same module, recurse

       … while evaluating 'collectDeclarationPositions'

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:11:33:

           10|   */
           11|   collectDeclarationPositions = {module_path, declarations, option_path ? []}:
             |                                 ^
           12|     if builtins.isAttrs declarations

       … from call site

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:75:5:

           74|   in
           75|     collectDeclarationPositions {module_path = module_path; declarations = config;};
             |     ^
           76|

       … while evaluating 'get_modules_defined_attrs'

       at /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix:70:31:

           69|   */
           70|   get_modules_defined_attrs = module_path: let
             |                               ^
           71|     inherit (self) collectDeclarationPositions evalModuleStub;

       … from call site

       at «string»:1:1:

            1| (import /nix/store/j3zd4a0qim303f2vrjv9f4naxxhxnix4-python3.9-nix-gui-0.1.2/lib/python3.9/site-packages/nixui/nix/lib.nix).get_modules_defined_attrs /home/andrew/p/nix-gui/nixui/tests/sample/configuration.nix
             | ^

pkgs.pulseaudioFull is a valid attribute set, just one which isn't defined within the module, so it makes sense that the expression recurses into it, however ideally the expression doesn't recurse into pkgs.pulseaudioFull (hence # && module_path is same.

I'd prefer doing this in lib.nix if possible, as all the solutions involving parser.py doing the work mentioned in the previous comment would be very hacky.

Another question, what risks do you see in getting declarations via import module_path {inherit pkgs; ...} (currently we have pkgs = {}.

@lapp0 lapp0 changed the title WIP: Finish DynamicListOf DynamicListOf Fixes Nov 8, 2021
@lapp0 lapp0 merged commit 2012f95 into master Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement CHECK DESCENDANTS Complete Implementation of ListOf
1 participant