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

File set combinators base: lib.fileset.toSource #245623

Merged
merged 1 commit into from
Sep 2, 2023

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jul 26, 2023

Description of changes

This is the first PR split off from the more complete file set combinators PR, in order to make it easier to incrementally review and merge it. This PR implements a severely limited interface, only containing a single new function:

# Imports ./. into the store, only including files under ./some/path
lib.fileset.toSource {
  root = ./.;
  fileset = ./some/path;
}

Despite that, it's kind of neat how even this simple filtering was previously rather tricky to implement using lib.sources filters, and simple mistakes can break it:

How to do it without this function

Edit: Actually it occurred to me that this might have a mistake, iirc something relating to the filesystem root.

{
  functionBased =
    let
      # The toString and extra "/" is necessary!
      subpath = toString ./some/path + "/";
    in
    lib.cleanSourceWith {
      src = ./.;
      filter = path: type:
        lib.strings.hasPrefix path subpath || lib.strings.hasPrefix subpath path;
    };

  regexBased = lib.sourceByRegex ./. [
    "some"
    "some/path"
    "some/path/.*"
  ];
}

This work is sponsored by Antithesis

Things done
  • Implementation
    • Tested
    • Commented
    • Benchmarked
  • Contributor documentation
    • Design document
  • User documentation
    • Function reference
    • General library documentation

@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 6.topic: policy discussion 6.topic: lib The Nixpkgs function library labels Jul 26, 2023
@infinisil infinisil mentioned this pull request Jul 26, 2023
7 tasks
@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 Jul 26, 2023
@infinisil infinisil force-pushed the fileset.toSource branch 2 times, most recently from 0ef9aeb to 36822c6 Compare July 27, 2023 20:46
doc/functions/fileset.section.md Outdated Show resolved Hide resolved
doc/functions/fileset.section.md Outdated Show resolved Hide resolved
doc/functions/fileset.section.md Outdated Show resolved Hide resolved
doc/functions/fileset.section.md Outdated Show resolved Hide resolved
doc/functions/fileset.section.md Outdated Show resolved Hide resolved
lib/fileset/README.md Show resolved Hide resolved
lib/fileset/default.nix Outdated Show resolved Hide resolved
lib/fileset/default.nix Outdated Show resolved Hide resolved
lib/fileset/default.nix Outdated Show resolved Hide resolved
lib/fileset/tests.sh Outdated Show resolved Hide resolved
@infinisil infinisil force-pushed the fileset.toSource branch 2 times, most recently from 68eb01e to 7b4aec4 Compare August 4, 2023 21:36
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 11, 2023
@infinisil infinisil force-pushed the fileset.toSource branch 4 times, most recently from 92e11e8 to e56062e Compare August 11, 2023 20:48
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 11, 2023
@infinisil
Copy link
Member Author

I'll mark this as ready to review because it's almost done, there's only two relatively minor things left:

  • Clean up and add better commenting in internal.nix
  • Squash the Git history a bit

@infinisil infinisil marked this pull request as ready for review August 12, 2023 07:58
@infinisil infinisil requested a review from edolstra as a code owner August 12, 2023 07:58
@infinisil infinisil requested a review from roberth August 12, 2023 16:09
@infinisil
Copy link
Member Author

This is now very ready!

@roberth roberth self-assigned this Aug 19, 2023
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Just tiny nitpicks in the docs to clarify wording.

Thanks @infinisil and @roberth for making this happen, step by step.

clean code

doc/functions/fileset.section.md Outdated Show resolved Hide resolved
doc/functions/fileset.section.md Outdated Show resolved Hide resolved
doc/functions/fileset.section.md Outdated Show resolved Hide resolved
doc/functions/fileset.section.md Outdated Show resolved Hide resolved
lib/fileset/README.md Outdated Show resolved Hide resolved

Arguments:
- (+) Such paths are usually produced by derivations, which means `toSource` would either:
- Require IFD if `builtins.path` is used as the underlying primitive
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Require IFD if `builtins.path` is used as the underlying primitive
- Require [Import From Derivation](../../pkgs/README.md#import-from-derivation) if `builtins.path` is used as the underlying primitive

Copy link
Member Author

@infinisil infinisil Aug 31, 2023

Choose a reason for hiding this comment

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

Hmm that section is only saying that IFD is disallowed in Nixpkgs, but the file set library isn't really meant to be used in Nixpkgs. I'd say let's leave this for now and wait for NixOS/nix#7332

lib/fileset/README.md Outdated Show resolved Hide resolved
lib/fileset/benchmark.sh Outdated Show resolved Hide resolved
lib/fileset/default.nix Outdated Show resolved Hide resolved
lib/fileset/internal.nix Outdated Show resolved Hide resolved
@infinisil
Copy link
Member Author

Thanks, applied all your suggestions!

@infinisil
Copy link
Member Author

This is as ready as it gets, I'll merge this myself! 🚀

@infinisil infinisil merged commit d66929b into NixOS:master Sep 2, 2023
@infinisil infinisil deleted the fileset.toSource branch September 2, 2023 02:07
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/easy-source-filtering-with-file-sets/29117/11

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 6.topic: policy discussion 8.has: documentation This PR adds or changes documentation 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.

4 participants