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

lib: Add listFilesRecursive function to filesystem #101096

Merged
merged 1 commit into from Oct 20, 2020

Conversation

@fzakaria
Copy link
Contributor

@fzakaria fzakaria commented Oct 19, 2020

Motivation for this change

Add a friendly function to easily return a flattened list of files within a directory.

This is useful if you want to easily iterate or concatStringsSep the list of
files all found within a directory. (i.e. when constructing Java's CLASSPATH)

❯ tree /nix/store/g87va52nkc8jzbmi1aqdcf2f109r4dvn-maven-repository | head
/nix/store/g87va52nkc8jzbmi1aqdcf2f109r4dvn-maven-repository
├── antlr
│   └── antlr
│       └── 2.7.2
│           ├── antlr-2.7.2.jar -> /nix/store/d027c8f2cnmj5yrynpbq2s6wmc9cb559-antlr-2.7.2.jar
│           └── antlr-2.7.2.pom -> /nix/store/mv42fc5gizl8h5g5vpywz1nfiynmzgp2-antlr-2.7.2.pom
├── avalon-framework
│   └── avalon-framework
│       └── 4.1.3
│           ├── avalon-framework-4.1.3.jar -> /nix/store/iv5fp3955w3nq28ff9xfz86wvxbiw6n9-avalon-framework-4.1.3.jar


❯ nix-repl> listFilesRecursive /nix/store/g87va52nkc8jzbmi1aqdcf2f109r4dvn-maven-repository
[ /nix/store/g87va52nkc8jzbmi1aqdcf2f109r4dvn-maven-repository/antlr/antlr/2.7.2/antlr-2.7.2.jar /nix/store/g87va52nkc8jzbmi1aqdcf2f109r4dvn-maven-repository/antlr/antlr/2.7.2/antlr-2.7.2.pom /nix/store/g87va52nkc8jzbmi1aqdcf2f109r4dvn-maven-repository/avalon-framework/avalon-framework/4.1.3/avalon-framework-4.1.3.jar /nix/store/g87va52nkc8jzbmi1aqdcf2f109r4dvn-maven-repository/avalon-framework/avalon-framework/4.1.3/avalon-framework-4.1.3.pom /nix/store/g87va52nkc8jzbmi1aqdcf2f109r4dvn-maven-repository/backport-util-concurrent/backport-util-concurrent/3.1/backport-util-concurrent-3.1.jar ..
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Member

@roberth roberth left a comment

Not so sure about the name actually. Perhaps listFilesRecursively?

lib/filesystem.nix Outdated Show resolved Hide resolved
@fzakaria fzakaria force-pushed the faridzakaria/readTree branch from a2aa7f5 to 42387c1 Oct 19, 2020
@fzakaria fzakaria force-pushed the faridzakaria/readTree branch 2 times, most recently from 409150f to 6a55ba0 Oct 19, 2020
lib/filesystem.nix Outdated Show resolved Hide resolved
Copy link
Member

@Infinisil Infinisil left a comment

@roberth I'd say listFilesRecursive (similar as mapAttrsRecursive)

Add a friendly function to easily return a flattened list of files
within a directory.

This is useful if you want to easily iterate or concatSep the list of
files all found within a directory.
(i.e. when constructing Java's CLASSPATH)

Style improvements

Co-authored-by: Silvan Mosberger <github@infinisil.com>
@fzakaria fzakaria force-pushed the faridzakaria/readTree branch from ac7439b to 5f1d1bc Oct 19, 2020
@fzakaria fzakaria changed the title lib: Add readTree function to filesystem lib: Add listFilesRecursive function to filesystem Oct 19, 2020
@fzakaria fzakaria requested a review from Infinisil Oct 19, 2020
@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Oct 20, 2020

@Infinisil made the change :)
Thakns for the review

@Infinisil Infinisil merged commit 6a94d64 into NixOS:master Oct 20, 2020
16 checks passed
@grahamc
Copy link
Member

@grahamc grahamc commented Oct 20, 2020

Note that:

This is useful if you want to easily iterate or concatStringsSep the list offiles all found within a directory. (i.e. when constructing Java's CLASSPATH)

sounds likely to create accidental import from derivation.

@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Oct 20, 2020

Note that:

This is useful if you want to easily iterate or concatStringsSep the list offiles all found within a directory. (i.e. when constructing Java's CLASSPATH)

sounds likely to create accidental import from derivation.

Can you elaborate ? Is it because the returned lists are PATH and not string ?

@grahamc
Copy link
Member

@grahamc grahamc commented Oct 20, 2020

@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Oct 20, 2020

Ah that sounds like what @roberth was commenting at in another PR of mine at #100660 (comment)

I had the impression that if you had the outPath interpolated then it must have built. I guess Nix is smart enough to discern between simply referencing the /nix/store entry and reading it.

Thank you for the clarification.

@edolstra
Copy link
Member

@edolstra edolstra commented Oct 21, 2020

I really don't like the listFilesRecursive, which is not very grammatically correct. Maybe listTree or something like that?

It's also not clear that the API is very good. E.g. it takes a single dir argument, but maybe we'd want to add other arguments in the future (like a filter argument to stop descent into certain directories), which the current interface precludes. So we're sure to end up with a gazillion variants of this function.

BTW we're currently adding lib functions in a fairly ad hoc way, with no naming or API conventions. Maybe we can take some inspiration from Rust. (They probably also don't add standard library functions after one day of review. Everything we add here is technical debt that we need to support forever.)

@roberth
Copy link
Member

@roberth roberth commented Oct 21, 2020

For filtering we can make it accept a cleanSourceWith result in addition to paths.
I had that in mind when reviewing it, but I dismissed it because I couldn't think of a good place to document unimplemented features. I guess I could have suggested an inline comment.

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Oct 21, 2020

I'm not sure if filtering is needed, because the expensive part of builtins.path that requires filtering is the store importing. This function however only lists files (like find), which isn't very expensive, even for TB big directories.

And the result can still be filtered with just the list filter

@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Oct 21, 2020

No, it is because you can’t know what is in the result of a build before you build it, but the evaluation is a distinct phase before any building happens.

On Tue, Oct 20, 2020, at 7:28 PM, Farid Zakaria wrote: > Note that: >> This is useful if you want to easily iterate or concatStringsSep the list offiles all found within a directory. (i.e. when constructing Java's CLASSPATH) > sounds likely to create accidental import from derivation. Can you elaborate ? Is it because the returned lists are PATH and not string ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#101096 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAASXLBCFURKPDJXLFJ2FWTSLYMIHANCNFSM4SWYGOPQ.

For anyone curious about @grahamc feedback I wrote a little writeup on it https://fzakaria.com/2020/10/20/nix-parallelism-import-from-derivation.html

@edolstra I had the same intention as @Infinisil that the filtering can always happen afterwards on the full list.
Happy to augment it though.
Funny you mentioned readTree since that was my first draft of the function name before renaming it to recursive with feedback on this PR to match other parts of nixpkgs (mapAttrsRecursive); so even when trying to align names we fail ;)

Happy to make any changes we land on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants