Added composable version of `filterSource` to `lib` #33312

ElvishJerricco · 2018-01-02T05:35:44Z

ElvishJerricco · 2018-01-02T05:36:15Z

@shlevy, how does this look?

ElvishJerricco · 2018-01-02T05:49:11Z

@GrahamcOfBorg what constitutes rebuild-darwin/rebuild-linux? AFAICT, this should not require rebuilding any existing uses of the functions in sources.nix.

EDIT: Oh that's a bot.

ElvishJerricco · 2018-01-02T05:54:06Z

It'd be nice if we could transparently use lib.filterSource on all kinds of sources inputs, including builtins.filterSource, fetchFromGitHub, and of course paths. Would eliminate the complexity in callCabal2nix. Of course it would only be beneficial on paths, but it would be convenient nonetheless.

ElvishJerricco · 2018-01-02T06:18:19Z

I suppose it's not actually possible to apply builtins.filterSource to a derivation output no matter what, so I've settled for a convenience function that checks if a source can be filtered (and added an assertion to make sure we're being given an argument we can use)

shlevy · 2018-01-02T16:04:16Z

@ElvishJerricco The OfBorg tags say there are 0 linux rebuilds and 0 darwin rebuilds.

shlevy

A few small changes. Have you tested with callCabal2nix?

shlevy · 2018-01-02T16:06:55Z

lib/sources.nix

+  # intermediate copies being put in the nix store.
+  filterSource = f: src:
+    let isPath = builtins.typeOf src == "path";
+    in assert isLocalSource src; rec {


Instead of asserting here, why don't we just do builtins.storePath src if it's in the store already? You can't filter in that case but it's already in the store so there's no need anyway.

Or maybe not even do storePath, just pass src as-is.

I don't think it's a good idea to allow filterSource to accept inputs that it won't filter. That would be very misleading

Filtering is an optimization, though...

I don't think so. Filtering can be semantically relevant. e.g. If they want to filter out a particular source file.

Hmm, OK. Better to fail now and be permissive later if we need it I suppose.

Maybe this should take its arguments in an attrset so that we can extend it with new arguments (like, fallBackNoFilter) in the future?

shlevy · 2018-01-02T16:10:18Z

lib/sources.nix

+  # allowing you to chain multiple calls together without any
+  # intermediate copies being put in the nix store.
+  filterSource = f: src:
+    let isPath = builtins.typeOf src == "path";


Seems like it would be better to switch on src ? __is-lib-filterSource than isPath. E.g. hydra will pass in paths as { outPath = the-actual-path; etc. } so this will spuriously fail here.

Wouldn't builtins.filterSource fail on such inputs as well? AFAIK, this will succeed on anything builtins.filterSource succeeds on

filterSource calls coerceToPath, which respects outPath etc.

Just pushed a change. Is that better?

shlevy · 2018-01-02T16:15:28Z

lib/sources.nix

+      filter = if isPath then f else name: type: f name type && src.filter name type;
+      origSrc = if isPath then src else src.origSrc;
+      outPath = builtins.filterSource filter origSrc;
+      __is-lib-filterSource = true;


There's nothing standardized here but IMO double underscore should be reserved by nix itself.

Single underscore then?

shlevy · 2018-01-02T16:57:33Z

lib/sources.nix

+  # intermediate copies being put in the nix store.
+  filterSource = f: src:
+    let isFiltered = src ? _is-lib-filterSource;
+    in assert isLocalSource src; rec {


Hmm what does this assertion actually gain us? builtins.filterSource itself will throw here. And note that typeOf src == "path" is not quite exhaustive, as you can call builtins.filterSource on, say toString ./. (though I'm not sure if there are actual cases where that matters)

Hm. Yea I suppose the only way to exhaustively check that builtins.filterSource can take the argument would be to actually try it and catch any thrown error; but that would defeat the point since it would evaluate the intermediate filtered sources.

I was using the assertion to avoid an error message that's difficult to debug, but you're right; I don't think it will work. But the question remains: How do we have callCabal2nix determine when it's appropriate to try to filter?

callCabal2nix can do something like check (src.type or "") != "derivation"

That's not exhaustive though... I think either you have to do something like check if src is a string and if so if it's in the nix store, or just take an arg to callCabal2nix.

@edolstra Would you object to a hasContext primop? That would solve this...

ElvishJerricco · 2018-01-02T17:10:29Z

@shlevy Alright I think I've addressed all your points. Is this what you had in mind?

shlevy · 2018-01-02T17:16:58Z

lib/sources.nix

+  # intermediate copies being put in the nix store.
+  filterSource = { filter, src }:
+    let isFiltered = src ? _is-lib-filterSource;
+    in assert isLocalSource src; rec {


Does isLocalSource exist any more?

Derp. Fixed

edolstra · 2018-01-02T17:26:54Z

lib/sources.nix

+      filter = if isFiltered then name: type: filter name type && src.filter name type else filter;
+      origSrc = if isFiltered then src.origSrc else src;
+      outPath = builtins.filterSource filter origSrc;
+      _is-lib-filterSource = true;


Please don't use dashes in variable names.

ElvishJerricco · 2018-01-02T17:29:47Z

@shlevy One other thing we should probably think about is how callCabal2nix will make use of this. You suggested using src.type or "" == "derivation" to select which sources can't be filtered, but as you said, it's non-exhaustive. In particular, the only case that matters IMO is when src is itself a builtins.filterSource. I'd be ok with just failing in this case if callCabal2nix were a new feature, since users could just use lib.filterSource. But that's not acceptable since we have to be backward compatible.

edolstra · 2018-01-02T17:30:10Z

lib/sources.nix

+  # Like `builtins.filterSource`, except it will compose with itself,
+  # allowing you to chain multiple calls together without any
+  # intermediate copies being put in the nix store.
+  filterSource = { filter, src }:


Please don't use the same name as a builtin function. Call it composableFilterSource or something like that.

EDIT: Wrong thread

I was doing as fetchTarball/fetchurl do. I'm hesitant to call it something like composableFilterSource, since I think that this should be the recommended way to filter sources instead of the builtin function.

@peti @edolstra How about cleanSourceWith? Avoids the naming clash but remains generic.

cleanSourceWith sounds good to me.

shlevy · 2018-01-02T17:34:12Z

@ElvishJerricco IMO we shouldn't hold ourselves to especially strong compatability guarantees across NixOS releases, but I'll defer to @peti on that one.

shlevy · 2018-01-02T17:34:47Z

@ElvishJerricco I think NixOS/nix#1772 would be a general solution though.

peti

please address https://github.com/NixOS/nixpkgs/pull/33312/files#r159273889

ElvishJerricco · 2018-01-02T17:44:16Z

@shlevy Forgive me; I don't know much about Nix internals. Would NixOS/nix#1772 require evaluating builtins.filterSource to do the check? Wouldn't that put the source into the nix store prematurely, defeating the purpose?

ElvishJerricco · 2018-01-02T18:15:57Z

@peti Do you think it's ok to break existing uses of haskellPackages.callCabal2nix "foo" (builtins.filterSource ...) {} and instruct users to switch to cleanSourceWith? If not, we may want to hold off on merging this, as it doesn't really solve what I was originally hoping to solve.

3noch · 2018-01-03T05:50:28Z

lib/sources.nix

+      filter = filter';
+      outPath = builtins.filterSource filter' origSrc;
+      _isLibCleanSourceWith = true;
+    };

  # Filter sources by a list of regular expressions.
  #
  # E.g. `src = sourceByRegex ./my-subproject [".*\.py$" "^database.sql$"]`


IIRC this is actually a misleading example. I believe this will not recurse into directories unless they match these regexes. So in many cases this example would actually filter out everything and leave you with an empty folder. I need to double-check that this is the case but that's how it has appeared in my experience.

I realize this is tangential to your actual change. Feel free to ignore while I look into opening a separate PR.

I've confirmed this. I'll open a separate PR.

3noch · 2018-01-03T06:11:19Z

lib/sources.nix

+
+  # Like `builtins.filterSource`, except it will compose with itself,
+  # allowing you to chain multiple calls together without any
+  # intermediate copies being put in the nix store.


An example would be nice.

3noch · 2018-01-03T06:15:20Z

lib/sources.nix

+  # Like `builtins.filterSource`, except it will compose with itself,
+  # allowing you to chain multiple calls together without any
+  # intermediate copies being put in the nix store.
+  cleanSourceWith = { filter, src }:


An additional/alternative design that I've used is to write composeFilter and just use that to build up bigger filters.

peti · 2018-01-03T10:28:30Z

Do you think it's ok to break existing uses of haskellPackages.callCabal2nix "foo" (builtins.filterSource ...) {} and instruct users to switch to cleanSourceWith?

I don't know ... I have no idea how many people are actually using this idiom and would therefore be affected by this change. Personally, I think that it's okay'ish to break backwards-compatibility if there is a sufficiently self-explanatory error message.

ElvishJerricco · 2018-01-09T23:59:58Z

Can this be merged?

ElvishJerricco requested review from edolstra and nbp as code owners January 2, 2018 05:35

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 2, 2018

ElvishJerricco changed the title ~~Added composable version of filterSource to lib~~ WIP: Added composable version of filterSource to lib Jan 2, 2018

ElvishJerricco force-pushed the composable-filter-source branch from 78ecca2 to d46e7d8 Compare January 2, 2018 06:16

ElvishJerricco force-pushed the composable-filter-source branch 2 times, most recently from f85e792 to a60642a Compare January 2, 2018 06:21

ElvishJerricco changed the title ~~WIP: Added composable version of filterSource to lib~~ Added composable version of filterSource to lib Jan 2, 2018

shlevy requested changes Jan 2, 2018

View reviewed changes

ElvishJerricco force-pushed the composable-filter-source branch from a60642a to 2a83537 Compare January 2, 2018 16:54

shlevy reviewed Jan 2, 2018

View reviewed changes

ElvishJerricco force-pushed the composable-filter-source branch from 2a83537 to 89d7af0 Compare January 2, 2018 17:09

shlevy reviewed Jan 2, 2018

View reviewed changes

ElvishJerricco force-pushed the composable-filter-source branch from 89d7af0 to af5eea1 Compare January 2, 2018 17:17

shlevy approved these changes Jan 2, 2018

View reviewed changes

edolstra reviewed Jan 2, 2018

View reviewed changes

ElvishJerricco force-pushed the composable-filter-source branch from af5eea1 to 96894ec Compare January 2, 2018 17:30

ElvishJerricco force-pushed the composable-filter-source branch from 96894ec to a35ccbc Compare January 2, 2018 17:41

peti requested changes Jan 2, 2018

View reviewed changes

ElvishJerricco force-pushed the composable-filter-source branch 3 times, most recently from 4db2546 to 9048189 Compare January 2, 2018 18:13

3noch reviewed Jan 3, 2018

View reviewed changes

peti approved these changes Jan 3, 2018

View reviewed changes

Added lib.cleanSourceWith as composable version of filterSource

7e5f37d

ElvishJerricco force-pushed the composable-filter-source branch from 9048189 to 7e5f37d Compare January 9, 2018 23:59

peti merged commit a45ae54 into NixOS:master Jan 10, 2018

matthewbauer mentioned this pull request Jan 7, 2019

make-disk-image: use filterSource instead of cleanSource #52594

Merged

Added composable version of filterSource to lib #33312

Added composable version of filterSource to lib #33312

Conversation

ElvishJerricco commented Jan 2, 2018

ElvishJerricco commented Jan 2, 2018

ElvishJerricco commented Jan 2, 2018 • edited

ElvishJerricco commented Jan 2, 2018

ElvishJerricco commented Jan 2, 2018

shlevy commented Jan 2, 2018

shlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvishJerricco commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvishJerricco commented Jan 2, 2018

Choose a reason for hiding this comment

ElvishJerricco Jan 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlevy commented Jan 2, 2018

shlevy commented Jan 2, 2018

peti left a comment

Choose a reason for hiding this comment

ElvishJerricco commented Jan 2, 2018

ElvishJerricco commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peti commented Jan 3, 2018

ElvishJerricco commented Jan 9, 2018

Added composable version of `filterSource` to `lib` #33312

Added composable version of `filterSource` to `lib` #33312

ElvishJerricco commented Jan 2, 2018 •

edited

ElvishJerricco Jan 2, 2018 •

edited