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

doc: lib/textClosures #351872

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

doc: lib/textClosures #351872

wants to merge 1 commit into from

Conversation

hsjobeki
Copy link
Contributor

@dasJ can you help me to document this function? Its used in the activation script through
textClosureMap

I think most of the stuff is described in the header. But i dont know the details what a textClosure refers to, and the other descriptions are either incorrect or i dont understand them.

The usage could also be simplified by just using lib.concatLines instead of mapping with lib.id ?

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@hsjobeki hsjobeki marked this pull request as ready for review October 28, 2024 12:07
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Oct 28, 2024
@hsjobeki hsjobeki requested a review from dasJ October 28, 2024 12:07
@nix-owners nix-owners bot requested a review from infinisil October 28, 2024 12:08
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 28, 2024
@dasJ
Copy link
Member

dasJ commented Oct 29, 2024

Sorry, I have never seen this in my life, neither do I understand it

it's way too overloaded and almost but not quite computes a
topological sort of the depstrings.
:::
*/
textClosureList = predefined: arg:
Copy link
Member

@h7x4 h7x4 Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, this seems to be what the function is doing. It is used this way in php.buildEnv in order to sort import statements by the order they depend on each other. However I'm not the maintainer or a direct user of the function, so please take it with a grain of salt.

Is it okay to use the AttrsOf notation? Also, I'm not sure what was meant with the "almost but not quite computes a topological sort" comment. I wonder if there are some edge cases left over or if it's just that it doesn't fill the formal definition of a topological sort somehow.

/**
    Topologically sort a collection of dependent strings.
    Only depstrings listed in `arg` and their dependencies will be included in the result.

    # Inputs

    `predefined`

    : 1\. Function argument

    `arg`

    : 2\. Function argument

    # Type

    ```
    textClosureList :: AttrsOf { deps :: [String], text :: String } -> [String] -> [String]
    ```

    # Examples
    :::{.example}
    ## `lib.stringsWithDeps.textClosureList` usage example

    ```nix
    textClosureList {
      a = {
        deps = [ "b" "c" "e" ];
        text = "a: depends on b, c and e";
      };
      b = {
        deps = [ ];
        text = "b: no dependencies";
      };
      c = {
        deps = [ "b" ];
        text = "c: depends on b";
      };
      d = {
        deps = [ "c" ];
        text = "d: not being depended on by anything in `arg`";
      };
      e = {
        deps = [ "c" ];
        text = "e: depends on c, depended on by a, not in `arg`";
      };
    } [
      "a"
      "b"
      "c"
    ]
    => [
      "b: no dependencies"
      "c: depends on b"
      "e: depends on c, depended on by a, not in `arg`"
      "a: depends on b, c and e"
    ]
    ```

    :::
  */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't fill the formal definition of a topological sort
Describes it maybe best.

I also found textClosureMap to be used in the activation. Those list entries are generated from noDepEntry fullDepEntry or packEntry as the header suggests. But their meaning is not explained either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at them, they just seem like helpers, similar to how lib.nameValuePair is meant to be used with lib.listToAttrs. textClosureMap is probably to lib.textClosureList like lib.concatMapStringsSep is to lib.concatStringsSep. Not exactly, but in the sense that it's purpose is to be a shortand to map something before the textClosureList operation (and in this case concat with newlines as well).

We could reuse the helpers in the example to encourage their usage. I'm not sure if they are actually that useful though, might only be unnecessary overhead. What do you think about this?

textClosureList {
  a = fullDepEntry "a: depends on b, c and e" [ "b" "c" "e" ];
  b = noDepEntry "b: no dependencies";
  c = fullDepEntry "c: depends on b" [ "b" ];
  d = fullDepEntry "d: not being depended on by anything in `arg`" [ "c" ];
  e = fullDepEntry "e: depends on c, depended on by a, not in `arg`" [ "c" ];
} [
  "a"
  "b"
  "c"
]
=> [
  "b: no dependencies"
  "c: depends on b"
  "e: depends on c, depended on by a, not in `arg`"
  "a: depends on b, c and e"
]

As for packEntry, it doesn't seem to be used anywhere. I can't think of a good usecase for anonymous nodes at the moment ¯\_(ツ)_/¯

stringAfter looks like it's used around the module system. Usage looks similar to how it's used in the home-manager DAG system, used for specifying that something should come after something else in a resulting string/file, overriding the order the module system might merge it in. See also doc and usage example.

Considering the use in the module system, it might make more sense to write about the helpers not in the context of usage with the closure functions directly, but rather together with the module system. Maybe not in their doc comments, but in the nixos manual somewhere? What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants