Force explicit nixpkgs #25

guibou · 2018-08-08T12:30:03Z

This closes #24 by forcing an explicit nixpkgs clone.

If the user provides no nix_file, and no path or repository, it fails with a bazel error
If the user provides a nix_file, there is no solution to detect if the file is indeed using a pinned nixpkgs clone. We just override NIX_PATH to an invalid path and hop for the best

I'm not happy with the way I'm abusing NIX_PATH to write an error message. Any better idea?

mboes

The spirit of what you want is simple: NIX_PATH should never be inherited. So if you change the code to do exactly that, it'll become simpler than what's currently there.

If path is set, set NIX_PATH accordingly.
If repository is set, set NIX_PATH accordingly.
If neither, set NIX_PATH to empty (or in fact don't set it all - that probably works too).

With your current patch, hermeticity is not completely ensured. Because (quoting from the nix-build man page):

Add a path to the Nix expression search path. [...] Paths added through -I take precedence over NIX_PATH.

mboes · 2018-08-08T12:39:31Z

nixpkgs/nixpkgs.bzl

+  # 'nix_file_content' point to a pinned nixpkgs repository, so
+  # 'NIX_PATH' contains an error message, which will be seen by bazel user
+  res = ctx.execute(nix_build, quiet = False, timeout = timeout, environment=dict(
+    NIX_PATH="nixpkgs=implicit nixpkgs value is disabled for hermeticity reason."


Why do this? Setting NIX_PATH= (i.e. to the empty string) works equally well.

Yes, but there is no clear if user provides a nix-file with implicit nixpkgs.

Compare, with this complex NIX_PATH:

[nix-shell:~/tweag/rules_nixpkgs]$ bazel test //... INFO: Build options have changed, discarding analysis cache. warning: Nix search path entry 'implicit nixpkgs value is disabled for hermeticity reason. Please set either 'path' or 'repository' in your rule OR set a 'nix_file' or 'nix_file_content' which explicitly fetch a pinned nixpkgs' does not exist, ignoring error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I), at (string):1:19 ERROR: /home/guillaume/tweag/rules_nixpkgs/tests/BUILD:3:2: no such package '@expr-test//': Cannot build Nix attribute expr-test. stdout: stderr: warning: Nix search path entry 'implicit nixpkgs value is disabled for hermeticity reason. Please set either 'path' or 'repository' in your rule OR set a 'nix_file' or 'nix_file_content' which explicitly fetch a pinned nixpkgs' does not exist, ignoring error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I), at (string):1:19 and referenced by '//tests:run-expr-test' ERROR: Analysis of target '//tests:run-expr-test' failed; build aborted: no such package '@expr-test//': Cannot build Nix attribute expr-test. stdout: stderr: warning: Nix search path entry 'implicit nixpkgs value is disabled for hermeticity reason. Please set either 'path' or 'repository' in your rule OR set a 'nix_file' or 'nix_file_content' which explicitly fetch a pinned nixpkgs' does not exist, ignoring error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I), at (string):1:19 INFO: Elapsed time: 0.617s INFO: 0 processes. FAILED: Build did NOT complete successfully (4 packages loaded) FAILED: Build did NOT complete successfully (4 packages loaded)

With empty NIX_PATH:

[nix-shell:~/tweag/rules_nixpkgs]$ bazel test //... INFO: Build options have changed, discarding analysis cache. error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I), at (string):1:19 ERROR: /home/guillaume/tweag/rules_nixpkgs/tests/BUILD:3:2: no such package '@expr-test//': Cannot build Nix attribute expr-test. stdout: stderr: error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I), at (string):1:19 and referenced by '//tests:run-expr-test' ERROR: Analysis of target '//tests:run-expr-test' failed; build aborted: no such package '@expr-test//': Cannot build Nix attribute expr-test. stdout: stderr: error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I), at (string):1:19 INFO: Elapsed time: 1.300s INFO: 0 processes. FAILED: Build did NOT complete successfully (4 packages loaded) FAILED: Build did NOT complete successfully (4 packages loaded)

This is like using 0 as an error value instead of a Maybe type, or Verisign replacing all "Domain not found" errors with Site Finder. a) we should not be in the business of abusing environment variables meant for structured input to the Nix interpreter as unstructured output to the user. b) we should not assume that users can't follow a structured debugging process. Give them good documentation and get out of their way.

I agree that the solution is hackish, but I rather prefer that than an error with limited details.

mboes · 2018-08-08T12:44:52Z

nixpkgs/nixpkgs.bzl

@@ -30,13 +30,19 @@ def _nixpkgs_package_impl(ctx):
  else:
    ctx.template("BUILD", Label("@io_tweag_rules_nixpkgs//nixpkgs:BUILD.pkg"))

+  strFailureImplicitNixpkgs = (
+    "You must provide a 'path' or 'repository' or a nix file which point to a"


Simplify the error message:

One of 'path' or 'repository' must be provided. The NIX_PATH environment variable is not inherited.

But what about nix_file which point to a pinned repository. User have the right to do that and the error message should tell them, no?

Then adjust the error message accordingly.

One of 'path', 'repository', nix_file or nix_file_content must be provided. The NIX_PATH environment variable is not inherited.

Error messages should be pithy, to the point and assume as little about what the user is doing as possible.

mboes · 2018-08-08T12:51:15Z

nixpkgs/nixpkgs.bzl

@@ -65,12 +71,12 @@ def _nixpkgs_package_impl(ctx):
    path = ["-I", "nixpkgs={0}".format(ctx.path(ctx.attr.repository).dirname)]
  elif ctx.attr.path:
    path = ["-I", "nixpkgs={0}".format(ctx.attr.path)]
+  elif not ctx.attr.nix_file and not ctx.attr.nix_file_content:


Use De Morgan's law for a more readable guard.

I disagree about readability, but I'm sure that's a matter of personal taste, I'll follow yours for that one.

guibou · 2018-08-08T14:49:34Z

@mboes I addressed the points of your review, please have a new look.

Done

- providing no `path`, `repository`, `nix_file`, `nix_file_content` means that nixpkgs is implicit and will fail in bazel - we ensure that NIX_PATH cannot be used.

guibou · 2018-08-10T12:26:42Z

@mboes I addressed your review, added a CHANGELOG entry and updated the documentation.

guibou requested review from mboes and Fuuzetsu August 8, 2018 12:30

mboes previously requested changes Aug 8, 2018

View reviewed changes

guibou force-pushed the gb/force_explicit_nixpkgs branch from f13ad0e to 785fa3b Compare August 8, 2018 14:39

guibou force-pushed the gb/force_explicit_nixpkgs branch 2 times, most recently from 1ec4b2e to 689e297 Compare August 8, 2018 15:34

Fix explicit nixpkgs

7fd76cb

- providing no `path`, `repository`, `nix_file`, `nix_file_content` means that nixpkgs is implicit and will fail in bazel - we ensure that NIX_PATH cannot be used.

guibou force-pushed the gb/force_explicit_nixpkgs branch from 689e297 to 7fd76cb Compare August 10, 2018 12:24

mboes approved these changes Aug 10, 2018

View reviewed changes

mboes merged commit fb89dc9 into master Aug 10, 2018

mboes deleted the gb/force_explicit_nixpkgs branch August 10, 2018 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force explicit nixpkgs #25

Force explicit nixpkgs #25

guibou commented Aug 8, 2018

mboes left a comment

mboes Aug 8, 2018

guibou Aug 8, 2018

mboes Aug 8, 2018

guibou Aug 8, 2018

guibou Aug 8, 2018

mboes Aug 8, 2018

guibou Aug 8, 2018

mboes Aug 8, 2018

mboes Aug 8, 2018

guibou Aug 8, 2018

guibou commented Aug 8, 2018 •

edited

Loading

guibou commented Aug 10, 2018

Force explicit nixpkgs #25

Force explicit nixpkgs #25

Conversation

guibou commented Aug 8, 2018

mboes 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

guibou commented Aug 8, 2018 • edited Loading

guibou commented Aug 10, 2018

guibou commented Aug 8, 2018 •

edited

Loading