Fail when 'nix_file_deps' is not exhaustive #76

guibou · 2019-03-19T15:55:43Z

If nix_file_deps does not contain all the direct and indirect
dependencies of the nix package, bazel won't rebuild it if any of
these file change.

This patch track the dependencies by reading nix-build verbose output
and fails if theses dependencies are not explicitly listed in
nix_file_deps or repositories.

Closes #74

nixpkgs/nixpkgs.bzl

aherrmann · 2019-03-19T17:23:23Z

I tried this out and found an instance where it failed because of a directory that was not listed in nix_file_deps. However, adding a directory to nix_file_deps also fails in Bazel's symlink rule with "not a regular file". The check for nix_file_deps entries should probably ignore directories.

guibou · 2019-03-19T17:31:06Z

Good catch @aherrmann. Do you have a detailed example I can work on?. Is that a nix derivation which includes ./. (or similar) in its src?

aherrmann · 2019-03-19T17:36:49Z

@guibou It's a Nix derivation that includes ./.. It's a local override, but it looks very much like this nixpkgs package.

guibou · 2019-04-25T12:21:08Z

@aherrmann could you give it a new look? I've fixed the issue by detecting subfiles in directories.

aherrmann

@guibou I tested it again. The directory issue seems to be resolved. Thank you, this is great!

aherrmann · 2019-04-25T13:18:33Z

nixpkgs/nixpkgs.bzl

+You need to update the repository rule *{repo_name}* and set/extend *nix_file_deps* with the following values:
+
+nix_file_deps = [
+    "{deps_listing}",


This can contain duplicates. That may be fine. Just wanted to point it out.

E.g.

".../sass/Gemfile", ".../sass/Gemfile.lock", ".../sass/Gemfile", ".../sass/Gemfile.lock",

Hey, nice catch. I'll convert to set prior to display.

Well, actually, there is no set by default in skylark, and just immutable depset which may not be suitable for this kind of job.

On the other hand, I'm afraid all this list O(n) behavior may slow down the phase.

I've changed everything to dict with None values.

nixpkgs/nixpkgs.bzl

If `nix_file_deps` does not contain all the direct and indirect dependencies of the nix package, bazel won't rebuild it if any of these file change. This patch track the dependencies by reading nix-build verbose output and fails if theses dependencies are not explicitly listed in nix_file_deps or repositories.

Profpatsch · 2019-05-02T20:21:40Z

Yeah no, doesn’t work:

ERROR: Analysis of target '//:example' failed; build aborted: no such package '@io_tweag_rules_haskell_ghc-nixpkgs//': 

Non hermetic configuration for repository io_tweag_rules_haskell_ghc-nixpkgs.

The following dependencies are not declared in *nixpkgs_package* attributes.

You need to update the repository rule *io_tweag_rules_haskell_ghc-nixpkgs* and set/extend *nix_file_deps* with the following dependencies (adapted to your workspace):

nix_file_deps = [
    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/lib/minver.nix",
    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/pkgs/top-level/impure.nix",
    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/pkgs/top-level/default.nix",
    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/pkgs/stdenv/booter.nix",
    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/lib/default.nix",
    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/lib/fixed-points.nix",
    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/lib/lists.nix",
    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/pkgs/stdenv/default.nix",
    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/lib/trivial.nix",
    
… many paths elided …

    "/private/var/tmp/_bazel_tweag/a22e3857dd8ea95edb398b1bda1d86d8/external/nixpkgs/pkgs/development/compilers/ghc/backport-dylib-command-size-limit.patch",
]

guibou · 2019-05-02T21:36:57Z

Where does this /private mountpoint comes from?

guibou · 2019-05-02T21:38:09Z

Actually, that's your nixpkgs repository. It works as expected, you must add theses files to the nix_file_deps, as far as I know.

Profpatsch · 2019-05-03T12:30:13Z

It’s running the start script with this WORKSPACE file: tweag/rules_haskell#855 (comment) (change rules_nixpkgs to the latest commit) on OSX. I haven’t tried it on NixOS yet

So it the heuristics don’t work with the most minimal setup?

guibou · 2019-05-06T09:51:53Z

So it the heuristics don’t work with the most minimal setup?

I'm not sure I understand you.

From my point of view, it works as expected. You must list all the dependencies of your nix process, which, in your case, are all the files listed inside nixpkgs_git_repository. I admit it is not convenient. We may be able to tune the heuristic to ignore files from the nixpkgs_git_repository command.

Profpatsch · 2019-05-07T11:19:22Z

You must list all the dependencies of your nix process, which, in your case, are all the files listed inside nixpkgs_git_repository.

This means you have a list of every single nixpkgs file somewhere that you update every time your nixpkgs checkout changes? That does not sound reasonable to me.

The example I gave is the most minimal setup of a rules_haskell repository with rules_nixpkgs as a dependency. It’s the absolute base case that should be supported everywhere and all the time.

As long as it’s broken like that, I’m afraid we have to revert the heuristics, since they obviously don’t work even for the most basic use-case.

guibou · 2019-05-07T11:39:03Z

This means you have a list of every single nixpkgs file somewhere that you update every time your nixpkgs checkout changes? That does not sound reasonable to me.

Sounds better than a non hermetic build. However we can fix this issue by checking for files both the nix_file_deps and all the other attributes of nixpkgs_package, such as the file contained in the repository or repositories argument.

I did not do that in the first version of this work because I'm not using nixpkgs_git_repository, so I did not realize the issue.

The example I gave is the most minimal setup of a rules_haskell repository with rules_nixpkgs as a dependency. It’s the absolute base case that should be supported everywhere and all the time.

You can imagine a even more minimal setup such as:

haskell_register_ghc_nixpkgs(
    version = "8.4.3",
    repositories = { "nixpkgs": ":nixpkgs.nix" }
)

Where nixpkgs.nix contains the relevant fetchTarball call. This solution does not use nixpkgs_git_repository and is more reproducible. Actually, I just realized that nixpkgs_git_repository does not forces the usage of the sha256 argument and hence is not reproducible.

As long as it’s broken like that, I’m afraid we have to revert the heuristics, since they obviously don’t work even for the most basic use-case.

I do not agree. The heuristics works (as far as I know) and is doing its job correctly and brings hermeticity and reproducibility. It is however not convenient in a specific use case which appears to be the one we document.

This being said, it can be fixed and we should open an issue about it.

To check that dependencies of `nix_file_deps` are correctly declared, we copy all declared dependencies into the external repository directory. If any dependencies were missed, then nix evaluation will fail. To test that this really works, remove the ``` nix_file_deps = ["//tests:pkgname.nix"], ``` line in `@nix-file-deps-test`. I tried writing a failure test for this using Skylibs `analysistest` framework, but that doesn't seem to work for failures in external repositories. Like #76, this likewise closes #74, but in a simpler way.

guibou requested a review from Profpatsch March 19, 2019 15:56

guibou mentioned this pull request Mar 19, 2019

Fix non-hermetic nixpkgs_package tweag/rules_haskell#756

Merged

aherrmann reviewed Mar 19, 2019

View reviewed changes

nixpkgs/nixpkgs.bzl Outdated Show resolved Hide resolved

guibou force-pushed the warn-incomplete-nix-file-deps branch from 4aa2cc0 to 8772541 Compare April 25, 2019 12:20

guibou requested a review from aherrmann April 25, 2019 12:21

aherrmann approved these changes Apr 25, 2019

View reviewed changes

guibou force-pushed the warn-incomplete-nix-file-deps branch from 251a96f to 456bb28 Compare April 25, 2019 13:38

guibou merged commit 67a80de into master Apr 25, 2019

guibou deleted the warn-incomplete-nix-file-deps branch April 25, 2019 13:48

This was referenced May 9, 2019

Remove nixpkgs_git_repository, it clashes with nix_file_deps heuristics #81

Closed

Upstream the listing of nix evaluation dependencies to nix target/lorri#77

Open

guibou mentioned this pull request Jun 2, 2019

Verbosity of nixpkgs_package #82

Closed

mboes mentioned this pull request Jul 20, 2019

Simplify nix_file_deps checking #86

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail when 'nix_file_deps' is not exhaustive #76

Fail when 'nix_file_deps' is not exhaustive #76

guibou commented Mar 19, 2019 •

edited

Loading

aherrmann commented Mar 19, 2019

guibou commented Mar 19, 2019

aherrmann commented Mar 19, 2019

guibou commented Apr 25, 2019

aherrmann left a comment

aherrmann Apr 25, 2019

guibou Apr 25, 2019

guibou Apr 25, 2019

Profpatsch commented May 2, 2019 •

edited

Loading

guibou commented May 2, 2019

guibou commented May 2, 2019

Profpatsch commented May 3, 2019 •

edited

Loading

guibou commented May 6, 2019

Profpatsch commented May 7, 2019

guibou commented May 7, 2019 •

edited

Loading

Fail when 'nix_file_deps' is not exhaustive #76

Fail when 'nix_file_deps' is not exhaustive #76

Conversation

guibou commented Mar 19, 2019 • edited Loading

aherrmann commented Mar 19, 2019

guibou commented Mar 19, 2019

aherrmann commented Mar 19, 2019

guibou commented Apr 25, 2019

aherrmann left a comment

Choose a reason for hiding this comment

aherrmann Apr 25, 2019

Choose a reason for hiding this comment

guibou Apr 25, 2019

Choose a reason for hiding this comment

guibou Apr 25, 2019

Choose a reason for hiding this comment

Profpatsch commented May 2, 2019 • edited Loading

guibou commented May 2, 2019

guibou commented May 2, 2019

Profpatsch commented May 3, 2019 • edited Loading

guibou commented May 6, 2019

Profpatsch commented May 7, 2019

guibou commented May 7, 2019 • edited Loading

guibou commented Mar 19, 2019 •

edited

Loading

Profpatsch commented May 2, 2019 •

edited

Loading

Profpatsch commented May 3, 2019 •

edited

Loading

guibou commented May 7, 2019 •

edited

Loading