[DEVOPS-810] Fix nix-shell env #3174
Conversation
lib.nix
Outdated
| @@ -12,10 +12,23 @@ let | |||
| then result | |||
| else default; | |||
|
|
|||
| filterSource = src: | |||
There was a problem hiding this comment.
There's a builtin. by the same name. Using a different name, like filterSrc, may reduce confusion/mistakes.
There was a problem hiding this comment.
OK, will rename to localLib.cleanHaskellSource.
default.nix
Outdated
| if (builtins.typeOf args.src) == "path" | ||
| then builtins.filterSource cleanSourceFilter args.src | ||
| else args.src or null; | ||
| src = localLib.filterSource args.src; |
There was a problem hiding this comment.
As filterSource is implemented I think some filters are getting dropped, like:
# Filter out .git repo# Filter out editor backup / swap files.# Filter out nix-build result symlinks
Is that what we want?
There was a problem hiding this comment.
The good thing about pkgs.lib.cleanSourceWith (as opposed to builtins.filterSource) is that it's chainable. In localLib.filterSource it calls the "default" pkgs.lib.cleanSource function which provides the above filters. I verify this by running nix-build and checking that a cached build is used.
rvl
force-pushed
the
devops-810-fix-gitrev-env
branch
2 times, most recently
from
8cbfdf7
to
8ec88de
Compare
lib.nix
Outdated
| # Filter out cabal build products | ||
| baseName == "dist" || | ||
| # Filter out files which don't affect cabal build | ||
| lib.hasSuffix ".nix" baseName |
There was a problem hiding this comment.
I'm not convinced that having a *.nix file exclusion inside a function called cleanHaskellSource is a good idea.
Granted, it's probably OK for the cardano-sl codebase because we don't arrange Haskell packages this way, but generally is quite common to have the Nix package definition for a Haskell package inside the package's source directory. But even if it weren't common, cleanHaskellSource is probably the wrong name for a filter that excludes *.nix files.
There was a problem hiding this comment.
Thanks for the review. The goal of these source filters is to avoid nix-build rebuilds of cabal packages when only "irrelevant" files have been changed. For example, the common case of when I edit a .nix file within the package's source directory. Try this:
nix-build -A cardano-sl-wallet-new # should download from hydra cache
touch wallet-new/hello
nix-build -A cardano-sl-wallet-new # now it wants to rebuild everything
There was a problem hiding this comment.
Yeah, the purpose is clear. It's just that one wouldn't normally expect a function named “clean haskell source” to pay attention to .nix files.
There was a problem hiding this comment.
OK, now it's called localLib.cleanSourceTree.
rvl
force-pushed
the
devops-810-fix-gitrev-env
branch
2 times, most recently
from
8bee667
to
76da2ba
Compare
|
I have added a few more useful filters to |
| # This is so that cached build products can be used whenever possible. | ||
| # It also applies the lib.cleanSource filter from nixpkgs which | ||
| # removes VCS directories, emacs backup files, etc. | ||
| cleanSourceTree = src: |
There was a problem hiding this comment.
Can we add doc files to this function as well so changing md files doesn't trigger new builds?
There was a problem hiding this comment.
Probably not because there is usually extra-source-files: README.md in the cabal file.
This was not working:
nix-shell default.nix -A cardano-sl-wallet-new.env
rvl
force-pushed
the
devops-810-fix-gitrev-env
branch
from
76da2ba
to
1eb2cb2
Compare
Description
The change to
justStaticExecutablesGitRevbroke the.envattribute of haskell derivations.Also there is a new nixpkgs lib function for filtering sources.
Linked issue
https://iohk.myjetbrains.com/youtrack/issue/DEVOPS-810
https://iohk.myjetbrains.com/youtrack/issue/DEVOPS-937
Type of change
QA Steps