Allow passing several labels to `nixpkgs_package` #29

thufschmitt · 2018-09-19T05:11:36Z

Deprecates the repository attribute in favor of the more general
repositories which expects a dict of the form

{
  "@some_label": "some_name",
  "@some_other_label": "some_other_name",
}

and generating the NIX_PATH
some_name=path(@some_label):some_other_name=path(@some_other_label)

I also have a couple of questions regarding this:

I would prefer having the dict be { "name": "@label"} instead of { "@labelb: "name"}, but the docs at https://docs.bazel.build/versions/master/skylark/lib/attr.html only mentions label_keyed_string_dict and not something like string_keyed_label_dict. Does anyone knows whether there is a way to build custom types for attrs?
I have manually updated the documentation in the README. Is it the right way to do so or is it generated in some way?

thufschmitt · 2018-09-19T05:29:05Z

Mh I've absolutely no idea what the CI failure means and it doesn't seem related to my work…

mboes · 2018-09-19T06:07:29Z

Looks like an upstream regression: bazelbuild/bazel#5946. We're hit by this because we're not currently pinning a nixpkgs version in this repository.

mboes · 2018-09-19T06:11:40Z

nixpkgs/nixpkgs.bzl

+        **kwargs
+    )
+
+def nix_instantiate(*args, **kwargs):


Is this function used anywhere? It looks like dead code to me.

Ouch sorry, pushed the wrong commit :/

Should be fixed now

mboes · 2018-09-19T06:12:43Z

nixpkgs/nixpkgs.bzl

    "repository": attr.label(),
    "build_file": attr.label(),
    "build_file_content": attr.string(),
  },
  local = True,
 )

+def nixpkgs_package(*args, **kwargs):


What's the reason for changing this rule to a macro that calls a single rule?

mboes

Could you say more about your use case? The extra generality sounds good to me, but current limitations of Bazel are forcing us to adopt an unfortunate API. While the API works, it's semantically dubious and it's an API we'll have to keep that way forever in the future unless we accept making a breaking change in the future. So ideally, if you can make do with a workaround while we work with upstream to get the feature we want in Bazel, we would delay applying this patch until upstream is fixed.

mboes · 2018-09-19T06:59:31Z

See bazelbuild/bazel#5356 for the upstream ticket.

thufschmitt · 2018-09-19T07:16:09Z

Could you say more about your use case?

It's motivated by tweag/rules_haskell#442 where I need rules_haskell to export a nix function (in haskell/nix/default.nix) which has to be imported and used in the dependent repositories.
The logical solution for that is to add it to the NIX_PATH, which requires this API extension (but obviously if someone has a better solution to this, I'm all for it)

thufschmitt · 2018-09-19T07:21:12Z

Would an approach such as bazelbuild/bazel#5356 be OK for you if we wrap the rule inside a macro which takes as argument a string_keyed_label_dict and unzips it before passing it to the actual rule? (that would be a bit ugly internally but would provide the interface we want)

mboes · 2018-09-19T07:59:35Z

That's a good idea. Provided it solves the forward compatibility problem. That's the main constraint.

Profpatsch · 2018-09-19T09:16:09Z

The logical solution for that is to add it to the NIX_PATH

I think the idea behind the change is sensible, for other uses, but in this case you don’t necessarily need to add your file to NIX_PATH. The path is just a convenience to fill free variables of the form <foo> and can always be replaced by simple lambdas (as long as the path is a local file). That is you can change

env NIX_PATH='foo=./foo:bar=./bar' nix-instantiate -E "<foo> <bar>"

to

nix-instantiate -E "{foo, bar}: foo bar" --arg foo "./foo" --arg bar "./bar"

thufschmitt · 2018-09-19T22:57:40Z

@Profpatsch that's true (and being able to pass arguments to the nix expression would be cool), but the NIX_PATH case is simpler as you know it will only contain paths, so it makes sense to put only labels in it while the arguments can be anything and since labels are fairly magical you can't say something like "this field can be a string or a label".

thufschmitt · 2018-09-20T08:16:29Z

I just stumbled upon another use-case for this: We have a master project using rules_nixpkgs and a custom (in tree) nix overlay and we want to create new projects depending on this master project. These child projects will use rules_nixpkgs as well and their nix config should include the overlay of the master.

Afaik there is no easy way to do this at the moment, while with this change it can be done by adding { "master_overlay": "@master_project//:overlay" } to the repositories field of each nixpkgs_package statement (and referencing it as <master_overlay> in the nix file)

Profpatsch · 2018-10-02T10:34:00Z

nixpkgs/nixpkgs.bzl

@@ -94,7 +104,7 @@ def _nixpkgs_package_impl(ctx):
  timeout = 1073741824

  res = ctx.execute(nix_build, quiet = False, timeout = timeout,
-                    environment=dict(NIX_PATH="nixpkgs=" + nix_path))
+                    environment=dict(NIX_PATH=nix_path))


This breaks the path argument, since it’s not of the form "nixpkgs=path" anymore.

Profpatsch · 2018-10-02T10:35:06Z

I’d rename repositories to nix_path, to follow nix nomenclature more closely, and deprecate path in the process.

Deprecates the `repository` attribute in favor of the more general `repositories` which expects a dict of the form ```python { "@some_label": "some_name", "@some_other_label": "some_other_name", } ``` and generating the `NIX_PATH` `some_name=path(@some_label):some_other_name=path(@some_other_label)`

Make the `repositories` argument of the form `{ path_element_name: label }` instead of `{ label: path_element_name }`

Profpatsch · 2018-10-16T16:13:35Z

Rebased the changes.

This was a holdover from a previous version of #29.

Before #29, we'd write ```python nixpkgs_package(name = "hello", repository = "@nixpkgs") ``` and this would just work. Now we write ```python nixpkgs_package(name = "hello", repositories = { "nixpkgs": "@nixpkgs//:default.nix" }) ``` which is quite a bit more verbose. With this fix, writing the repository label as `"@nixpkgs"` works again. Fixes #41.

PR #29 introduced the `repositories` attribute. But specifying a single repository is a common enough use case that we should continue to support it as before, without extra verbosity.

Before #29, we'd write ```python nixpkgs_package(name = "hello", repository = "@nixpkgs") ``` and this would just work. Now we write ```python nixpkgs_package(name = "hello", repositories = { "nixpkgs": "@nixpkgs//:default.nix" }) ``` which is quite a bit more verbose. With this fix, writing the repository label as `"@nixpkgs"` works again. Fixes #41.

thufschmitt requested a review from Profpatsch September 19, 2018 05:11

mboes reviewed Sep 19, 2018

View reviewed changes

thufschmitt force-pushed the allow-multiple-repositories branch from 17818ef to cdcc316 Compare September 19, 2018 06:32

mboes reviewed Sep 19, 2018

View reviewed changes

Profpatsch requested a review from mrkkrp September 19, 2018 09:04

This was referenced Sep 20, 2018

Attribute path for nixpkgs_package doesn't accept a relative path #21

Closed

Remove HaskellPrebuiltPackageInfo tweag/rules_haskell#442

Merged

Profpatsch reviewed Oct 2, 2018

View reviewed changes

Profpatsch mentioned this pull request Oct 2, 2018

Deprecate the path attribute to nixpkgs_package #32

Closed

thufschmitt added 2 commits October 16, 2018 17:59

Give a nicer interface to nixpkgs_package

2a05982

Make the `repositories` argument of the form `{ path_element_name: label }` instead of `{ label: path_element_name }`

Profpatsch force-pushed the allow-multiple-repositories branch from b81967b to 2a05982 Compare October 16, 2018 16:13

mboes approved these changes Oct 20, 2018

View reviewed changes

Add a shell.nix file. Make CI use it.

ed40f56

mboes merged commit 3087f0d into master Oct 20, 2018

mboes deleted the allow-multiple-repositories branch October 20, 2018 10:28

mboes added a commit that referenced this pull request Oct 25, 2018

Fix flipped description of repositories documentation.

fd9adeb

This was a holdover from a previous version of #29.

mboes mentioned this pull request Nov 13, 2018

Restore short repository labels #43

Merged

mboes mentioned this pull request Nov 13, 2018

Undeprecate the repository attribute #44

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing several labels to `nixpkgs_package` #29

Allow passing several labels to `nixpkgs_package` #29

thufschmitt commented Sep 19, 2018

thufschmitt commented Sep 19, 2018

mboes commented Sep 19, 2018

mboes Sep 19, 2018

thufschmitt Sep 19, 2018

mboes Sep 19, 2018

mboes left a comment

mboes commented Sep 19, 2018

thufschmitt commented Sep 19, 2018

thufschmitt commented Sep 19, 2018

mboes commented Sep 19, 2018

Profpatsch commented Sep 19, 2018 •

edited

Loading

thufschmitt commented Sep 19, 2018

thufschmitt commented Sep 20, 2018

Profpatsch Oct 2, 2018

Profpatsch commented Oct 2, 2018 •

edited

Loading

Profpatsch commented Oct 16, 2018

Allow passing several labels to nixpkgs_package #29

Allow passing several labels to nixpkgs_package #29

Conversation

thufschmitt commented Sep 19, 2018

thufschmitt commented Sep 19, 2018

mboes commented Sep 19, 2018

mboes Sep 19, 2018

Choose a reason for hiding this comment

thufschmitt Sep 19, 2018

Choose a reason for hiding this comment

mboes Sep 19, 2018

Choose a reason for hiding this comment

mboes left a comment

Choose a reason for hiding this comment

mboes commented Sep 19, 2018

thufschmitt commented Sep 19, 2018

thufschmitt commented Sep 19, 2018

mboes commented Sep 19, 2018

Profpatsch commented Sep 19, 2018 • edited Loading

thufschmitt commented Sep 19, 2018

thufschmitt commented Sep 20, 2018

Profpatsch Oct 2, 2018

Choose a reason for hiding this comment

Profpatsch commented Oct 2, 2018 • edited Loading

Profpatsch commented Oct 16, 2018

Allow passing several labels to `nixpkgs_package` #29

Allow passing several labels to `nixpkgs_package` #29

Profpatsch commented Sep 19, 2018 •

edited

Loading

Profpatsch commented Oct 2, 2018 •

edited

Loading