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.sourceSubdirs: init #234297

Closed
wants to merge 2 commits into from
Closed

lib.sourceSubdirs: init #234297

wants to merge 2 commits into from

Conversation

alyssais
Copy link
Member

Description of changes
Things done

This is really useful for using Nix in monorepo layouts, where you
might have a component build that needs access to multiple
subdirectories of the repository. Compared to settings "srcs", it's
more convenient, because it preserves the relative
structure (including parent directories), and it avoids unnecessary
rebuilds compared to not filtering for subdirectories.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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.

alyssais added 2 commits May 26, 2023 20:40
The name is borrowed from the lib.strings function, in the spirit of
Rust's std::path library reusing names from std::string methods, but
for more path-appropriate behaviour.
This is really useful for using Nix in monorepo layouts, where you
might have a component build that needs access to multiple
subdirectories of the repository.  Compared to settings "srcs", it's
more convenient, because it preserves the relative
structure (including parent directories), and it avoids unnecessary
rebuilds compared to not filtering for subdirectories.
@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 May 26, 2023
@infinisil
Copy link
Member

infinisil commented May 29, 2023

Could you take a look at the file set combinators in #222981 (the best start is here and the reference it links to starting here)? That functionality is very widely applicable and composable. In particular it would be equivalent to

with lib.fileset;
unions [
  ./one/directory
  ./another/directory
]

Or if you want to also import it into the store:

with lib.fileset;
importToStore {
  entryPoint = ./.;
  fileset = unions [
    ./one/directory
    ./another/directory
  ];
}

It would be great to get feedback if this works for your use case. Or if you're seeing a problem with this interface.

@alyssais
Copy link
Member Author

It would be great to get feedback if this works for your use case.

I was hoping you'd chime in with a better way! I tried it, but it doesn't look like it works:

error: the string 'gqwi6j6d4l9zs8svwhzwbxnglv3zwzl3-source' is not allowed to refer to a store path (such as '')

       … while evaluating anonymous lambda

       at /nix/store/2pplzbz7d5jmi4mnqmfi7lds5bz2z5hx-source/lib/filesystem.nix:40:6:

           39|     # Nix <2.14 compatibility shim
           40|     (path:
             |      ^
           41|       if ! pathExists path

       … from call site

       at /nix/store/2pplzbz7d5jmi4mnqmfi7lds5bz2z5hx-source/lib/fileset.nix:494:8:

          493|     in
          494|     if pathType entryPoint != "directory" then
             |        ^
          495|       # This would also be caught by the `may be influenced` condition further down, because of how files always set their containing directories as the base

       … while evaluating 'importToStore'

       at /nix/store/2pplzbz7d5jmi4mnqmfi7lds5bz2z5hx-source/lib/fileset.nix:453:19:

          452|   */
          453|   importToStore = { name ? "source", base ? entryPoint, entryPoint, fileset }:
             |                   ^
          454|     let

       … from call site

My entryPoint is the result of lib.cleanSource, so it's itself a store path. It seems like unlike cleanSource, the filesystem stuff doesn't compose — it should just merge the filters like cleanSource does, because otherwise even if you can fix the reference problem there'll be a redundant intermediate copy.

@infinisil
Copy link
Member

infinisil commented May 31, 2023

@alyssais Thanks for the feedback!

On one hand, the lib.fileset functions should be able to fully replace lib.sources, but safer and with better composition! So the idea would be to instead write a lib.fileset.clean, and use that instead of lib.sources.cleanSource. I haven't pushed such a function to the PR yet, limiting it to just the core functionality for now, but it can very much be implemented and I should do that at some point.

However you also made be realize that it is very much possible to convert lib.sources-produced values to be used in lib.fileset (not the other way around though!), which I now implemented, including better error messages when appropriate.

Could you give the same code another try with the updated PR?

@roberth roberth added the 6.topic: lib The Nixpkgs function library label Jul 8, 2023
@alyssais alyssais added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2023
@infinisil
Copy link
Member

infinisil commented Oct 16, 2023

lib.fileset.unions is now merged, so this is now supported:

lib.fileset.toSource {
  root = ./.;
  fileset = lib.fileset.unions [
    ./host/rootfs
    ./scripts
  ];
}

@infinisil infinisil closed this Oct 16, 2023
@alyssais
Copy link
Member Author

alyssais commented Oct 17, 2023

@infinisil sorry I never got round to retesting, but I still don't think it works for what I want to do — unless I'm mistaken there's still no way to use filesets with the result of lib.cleanSource, is there?

@infinisil
Copy link
Member

@alyssais Good point, indeed that's not yet possible. In the WIP PR #222981 there's a lib.fileset.fromSource which allows that. I'll add this to my TODO list.

Though note that for the future it would be great to have a fileset-based lib.cleanSource instead though.

@infinisil infinisil mentioned this pull request Oct 17, 2023
3 tasks
@infinisil
Copy link
Member

Now opened #261732 for this, I should be able to polish that soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 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