From 62c08271b460076e9a42ac0433190d1e9ccbf144 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Mon, 11 May 2020 14:21:25 +0200
Subject: [PATCH 01/16] buildifier -r .

---
 WORKSPACE                                 |  5 ++---
 nixpkgs/BUILD.bazel                       |  6 +++++-
 nixpkgs/constraints/BUILD.bazel           |  2 +-
 nixpkgs/nixpkgs.bzl                       |  5 +++--
 nixpkgs/toolchains/go.bzl                 | 18 ++++++++----------
 tests/BUILD.bazel                         |  4 ++--
 tests/invalid_nixpkgs_package/BUILD.bazel |  6 +++++-
 7 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/WORKSPACE b/WORKSPACE
index 1f25ff149..2f67a471f 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -202,12 +202,11 @@ http_archive(
 
 load(
     "//nixpkgs:toolchains/go.bzl",
-    "nixpkgs_go_configure"
+    "nixpkgs_go_configure",
 )
 
 nixpkgs_go_configure(repository = "@nixpkgs")
 
-load("@io_bazel_rules_go//go:deps.bzl", "go_rules_dependencies", "go_register_toolchains")
+load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")
 
 go_rules_dependencies()
-
diff --git a/nixpkgs/BUILD.bazel b/nixpkgs/BUILD.bazel
index 09fa6aa52..f055a773a 100644
--- a/nixpkgs/BUILD.bazel
+++ b/nixpkgs/BUILD.bazel
@@ -6,7 +6,11 @@ exports_files([
     "nixpkgs.bzl",
 ])
 
-filegroup(name = "srcs", srcs = glob(["**"]), visibility = ["//visibility:public"])
+filegroup(
+    name = "srcs",
+    srcs = glob(["**"]),
+    visibility = ["//visibility:public"],
+)
 
 # @bazel_tools//tools does not define a bzl_library itself, instead we are
 # supposed to define our own using the @bazel_tools//tools:bzl_srcs filegroup.
diff --git a/nixpkgs/constraints/BUILD.bazel b/nixpkgs/constraints/BUILD.bazel
index 66625309b..94678bda9 100644
--- a/nixpkgs/constraints/BUILD.bazel
+++ b/nixpkgs/constraints/BUILD.bazel
@@ -3,7 +3,7 @@ package(default_visibility = ["//visibility:public"])
 constraint_setting(name = "nix")
 
 constraint_value(
-    name =  "support_nix",
+    name = "support_nix",
     constraint_setting = ":nix",
 )
 
diff --git a/nixpkgs/nixpkgs.bzl b/nixpkgs/nixpkgs.bzl
index 3e6b0b5e6..f0e995520 100644
--- a/nixpkgs/nixpkgs.bzl
+++ b/nixpkgs/nixpkgs.bzl
@@ -6,7 +6,8 @@ load("@bazel_tools//tools/cpp:lib_cc_configure.bzl", "get_cpu_value")
 def _nixpkgs_git_repository_impl(repository_ctx):
     repository_ctx.file(
         "BUILD",
-        content = 'filegroup(name = "srcs", srcs = glob(["**"]), visibility = ["//visibility:public"])')
+        content = 'filegroup(name = "srcs", srcs = glob(["**"]), visibility = ["//visibility:public"])',
+    )
 
     # Make "@nixpkgs" (syntactic sugar for "@nixpkgs//:nixpkgs") a valid
     # label for default.nix.
@@ -208,7 +209,7 @@ def _nixpkgs_package_impl(repository_ctx):
         if create_build_file_if_needed:
             p = repository_ctx.path("BUILD")
             if not p.exists:
-               repository_ctx.template("BUILD", Label("@io_tweag_rules_nixpkgs//nixpkgs:BUILD.pkg"))
+                repository_ctx.template("BUILD", Label("@io_tweag_rules_nixpkgs//nixpkgs:BUILD.pkg"))
 
 _nixpkgs_package = repository_rule(
     implementation = _nixpkgs_package_impl,
diff --git a/nixpkgs/toolchains/go.bzl b/nixpkgs/toolchains/go.bzl
index 79cbb998c..d8c6366e2 100644
--- a/nixpkgs/toolchains/go.bzl
+++ b/nixpkgs/toolchains/go.bzl
@@ -2,20 +2,19 @@ load(
     "@io_bazel_rules_go//go:deps.bzl",
     "go_wrap_sdk",
 )
-
 load(
     "//nixpkgs:nixpkgs.bzl",
-    "nixpkgs_package"
+    "nixpkgs_package",
 )
 
 def nixpkgs_go_configure(
-    sdk_name = "go_sdk",
-    repository = None,
-    repositories = {},
-    nix_file = None,
-    nix_file_deps = None,
-    nix_file_content = None,
-    nixopts = []):
+        sdk_name = "go_sdk",
+        repository = None,
+        repositories = {},
+        nix_file = None,
+        nix_file_deps = None,
+        nix_file_content = None,
+        nixopts = []):
     """
     Use go toolchain from Nixpkgs. Will fail if not a nix-based platform.
 
@@ -41,7 +40,6 @@ def nixpkgs_go_configure(
             }
         """
 
-
     nixpkgs_package(
         name = "nixpkgs_go_toolchain",
         repository = repository,
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
index 16866a370..93228f458 100644
--- a/tests/BUILD.bazel
+++ b/tests/BUILD.bazel
@@ -86,7 +86,7 @@ sh_test(
 # Test nixpkgs_go_configure()
 go_binary(
     name = "go-test",
-    srcs = ["go-test.go"]
+    srcs = ["go-test.go"],
 )
 
 sh_test(
@@ -97,6 +97,6 @@ sh_test(
         "//nixpkgs:srcs",
         "//tests/invalid_nixpkgs_package:srcs",
         "@busybox_static//:bin",
-        "@nix-unstable//:bin"
+        "@nix-unstable//:bin",
     ],
 )
diff --git a/tests/invalid_nixpkgs_package/BUILD.bazel b/tests/invalid_nixpkgs_package/BUILD.bazel
index 00cbfeaa5..70ff958ed 100644
--- a/tests/invalid_nixpkgs_package/BUILD.bazel
+++ b/tests/invalid_nixpkgs_package/BUILD.bazel
@@ -1 +1,5 @@
-filegroup(name = "srcs", srcs = glob(["**"]), visibility = ["//visibility:public"])
+filegroup(
+    name = "srcs",
+    srcs = glob(["**"]),
+    visibility = ["//visibility:public"],
+)

From 985f13dc03bf05930d6af88fb71a3d5cf6202fb6 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Wed, 13 May 2020 17:22:25 +0200
Subject: [PATCH 02/16] implement _expand_location

---
 nixpkgs/nixpkgs.bzl | 48 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/nixpkgs/nixpkgs.bzl b/nixpkgs/nixpkgs.bzl
index f0e995520..4cf6f53dd 100644
--- a/nixpkgs/nixpkgs.bzl
+++ b/nixpkgs/nixpkgs.bzl
@@ -73,6 +73,54 @@ nixpkgs_local_repository = repository_rule(
 def _is_supported_platform(repository_ctx):
     return repository_ctx.which("nix-build") != None
 
+def _expand_location(repository_ctx, string, labels, attr = None):
+    """Expand `$(location label)` to a path.
+
+    Attrs:
+      repository_ctx: The repository rule context.
+      string: string, Replace instances of `$(location )` in this string.
+      labels: dict from label to path: Known label to path mappings.
+      attr: string, The rule attribute to use for error reporting.
+
+    Returns:
+      The string with all instances of `$(location )` replaced by paths.
+    """
+    num = string.count("$(location ")
+    result = ""
+    offset = 0
+    for i in range(num):
+        start = string.find("$(location ", offset)
+        label_start = start + len("$(location ")
+        label_end = string.find(")", label_start)
+        if label_end == -1:
+            fail("Unbalanced parentheses in location expansion for '{}'.".format(string[start:]), attr)
+        end = label_end + 1
+        label_str = string[label_start:label_end]
+        label_candidates = [
+            (lbl, path)
+            for (lbl, path) in labels.items()
+            if lbl.relative(label_str) == lbl
+        ]
+        if len(label_candidates) == 0:
+            fail("Unknown label '{}' in location expansion for '{}'.".format(label_str, string), attr)
+        elif len(label_candidates) > 1:
+            fail(
+                "Ambiguous label '{}' in location expansion for '{}'. Candidates: {}".format(
+                    label_str,
+                    string,
+                    ", ".join([str(lbl) for lbl in label_candidates]),
+                ),
+                attr,
+            )
+        location = paths.join(".", paths.relativize(
+            str(repository_ctx.path(label_candidates[0][1])),
+            str(repository_ctx.path(".")),
+        ))
+        result += string[offset:start] + location
+        offset = end
+    result += string[offset:]
+    return result
+
 def _nixpkgs_package_impl(repository_ctx):
     repository = repository_ctx.attr.repository
     repositories = repository_ctx.attr.repositories

From f3d30ee836b3fc0adea246da52fc8666fe4555c6 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Wed, 13 May 2020 17:24:16 +0200
Subject: [PATCH 03/16] use _expand_location in nixopts

---
 README.md           | 14 +++++++++++++-
 nixpkgs/nixpkgs.bzl | 12 ++++++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/README.md b/README.md
index 721f64dbb..346216033 100644
--- a/README.md
+++ b/README.md
@@ -176,7 +176,7 @@ Make the content of a Nixpkgs package available in the Bazel workspace.
 nixpkgs_package(
     name, attribute_path, nix_file, nix_file_deps, nix_file_content,
     repository, repositories, build_file, build_file_content, nixopts,
-    fail_not_supported,
+    expand_location, fail_not_supported,
 )
 ```
 
@@ -306,6 +306,18 @@ filegroup(
         <p>Extra flags to pass when calling Nix.</p>
       </td>
     </tr>
+    <tr>
+      <td><code>nixopts</code></td>
+      <td>
+        <p><code>Bool; optional</code></p>
+        <p>
+            If set to <code>True</code> any instance of
+            <code>$(location LABEL)</code> in <code>nixopts</code>
+            will be replaced by the path to the file referenced by
+            <code>LABEL</code> relative to the workspace root.
+        </p>
+      </td>
+    </tr>
     <tr>
       <td><code>fail_not_supported</code></td>
       <td>
diff --git a/nixpkgs/nixpkgs.bzl b/nixpkgs/nixpkgs.bzl
index 4cf6f53dd..9a2eed455 100644
--- a/nixpkgs/nixpkgs.bzl
+++ b/nixpkgs/nixpkgs.bzl
@@ -168,8 +168,9 @@ def _nixpkgs_package_impl(repository_ctx):
     else:
         expr_args = ["-E", "import <nixpkgs> { config = {}; overlays = []; }"]
 
+    nix_file_deps = {}
     for dep in repository_ctx.attr.nix_file_deps:
-        _cp(repository_ctx, dep)
+        nix_file_deps[dep] = _cp(repository_ctx, dep)
 
     expr_args.extend([
         "-A",
@@ -184,7 +185,13 @@ def _nixpkgs_package_impl(repository_ctx):
         "bazel-support/nix-out-link",
     ])
 
-    expr_args.extend(repository_ctx.attr.nixopts)
+    if repository_ctx.attr.expand_location:
+        expr_args.extend([
+            _expand_location(repository_ctx, opt, nix_file_deps, "nixopts")
+            for opt in repository_ctx.attr.nixopts
+        ])
+    else:
+        expr_args.extend(repository_ctx.attr.nixopts)
 
     for repo in repositories.keys():
         path = str(repository_ctx.path(repo).dirname) + "/nix-file-deps"
@@ -271,6 +278,7 @@ _nixpkgs_package = repository_rule(
         "build_file": attr.label(),
         "build_file_content": attr.string(),
         "nixopts": attr.string_list(),
+        "expand_location": attr.bool(default = False),
         "quiet": attr.bool(),
         "fail_not_supported": attr.bool(default = True, doc = """
             If set to True (default) this rule will fail on platforms which do not support Nix (e.g. Windows). If set to False calling this rule will succeed but no output will be generated.

From 548d0e0626cb0bf11045b943408917e3f5abc3d9 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Wed, 13 May 2020 18:09:44 +0200
Subject: [PATCH 04/16] Add test case for nixopts location expansion

---
 BUILD.bazel                  |  4 ++++
 WORKSPACE                    | 21 +++++++++++++++++++++
 tests/BUILD.bazel            | 24 ++++++++++++++++++++++++
 tests/location_expansion.nix | 17 +++++++++++++++++
 tests/location_expansion.sh  |  5 +++++
 5 files changed, 71 insertions(+)
 create mode 100644 tests/location_expansion.nix
 create mode 100755 tests/location_expansion.sh

diff --git a/BUILD.bazel b/BUILD.bazel
index e69de29bb..efcf274c9 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -0,0 +1,4 @@
+exports_files([
+    "nixpkgs.json",
+    "nixpkgs.nix",
+])
diff --git a/WORKSPACE b/WORKSPACE
index 2f67a471f..e2aa0869e 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -170,6 +170,27 @@ nixpkgs_package(
     repository = "@remote_nixpkgs",
 )
 
+nixpkgs_package(
+    name = "nixpkgs_location_expansion_test",
+    build_file_content = "exports_files(glob(['out/**']))",
+    expand_location = True,
+    nix_file = "//tests:location_expansion.nix",
+    nix_file_deps = [
+        "//:nixpkgs.json",
+        "//:nixpkgs.nix",
+        "@io_tweag_rules_nixpkgs//tests:relative_imports/nixpkgs.nix",
+    ],
+    nixopts = [
+        "--arg",
+        "attrs",
+        "{ nixpkgs_json = $(location //:nixpkgs.json); nixpkgs_nix = $(location //:nixpkgs.nix); }",
+        "--arg",
+        "relative_imports",
+        "$(location @io_tweag_rules_nixpkgs//tests:relative_imports/nixpkgs.nix)",
+    ],
+    repository = "@remote_nixpkgs",
+)
+
 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
 
 http_archive(
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
index 93228f458..4c5977676 100644
--- a/tests/BUILD.bazel
+++ b/tests/BUILD.bazel
@@ -54,6 +54,30 @@ load("@io_bazel_rules_go//go:def.bzl", "go_binary")
     ),
 ]
 
+# Test nixopts location expansion
+sh_test(
+    name = "location-expansion-test",
+    srcs = ["location_expansion.sh"],
+    args = [
+        "$(POSIX_DIFF)",
+        "$(rootpath //:nixpkgs.json)",
+        "$(rootpath //:nixpkgs.nix)",
+        "$(rootpath //tests:relative_imports/nixpkgs.nix)",
+        "$(rootpath @nixpkgs_location_expansion_test//:out/nixpkgs.json)",
+        "$(rootpath @nixpkgs_location_expansion_test//:out/nixpkgs.nix)",
+        "$(rootpath @nixpkgs_location_expansion_test//:out/relative_imports.nix)",
+    ],
+    data = [
+        "//:nixpkgs.json",
+        "//:nixpkgs.nix",
+        "//tests:relative_imports/nixpkgs.nix",
+        "@nixpkgs_location_expansion_test//:out/nixpkgs.json",
+        "@nixpkgs_location_expansion_test//:out/nixpkgs.nix",
+        "@nixpkgs_location_expansion_test//:out/relative_imports.nix",
+    ],
+    toolchains = ["@rules_sh//sh/posix:make_variables"],
+)
+
 # Test nixpkgs_cc_configure() by building some CC code.
 cc_binary(
     name = "cc-test",
diff --git a/tests/location_expansion.nix b/tests/location_expansion.nix
new file mode 100644
index 000000000..8023925b6
--- /dev/null
+++ b/tests/location_expansion.nix
@@ -0,0 +1,17 @@
+with import <nixpkgs> { config = {}; overlays = []; };
+
+{ attrs, relative_imports }:
+let
+  inherit (attrs) nixpkgs_json nixpkgs_nix;
+in
+  runCommand "location-expansion"
+    {
+      preferLocalBuild = true;
+      allowSubstitutes = false;
+    }
+    ''
+      mkdir -p $out/out
+      cp ${nixpkgs_json} $out/out/nixpkgs.json
+      cp ${nixpkgs_nix} $out/out/nixpkgs.nix
+      cp ${relative_imports} $out/out/relative_imports.nix
+    ''
diff --git a/tests/location_expansion.sh b/tests/location_expansion.sh
new file mode 100755
index 000000000..9f4df14b1
--- /dev/null
+++ b/tests/location_expansion.sh
@@ -0,0 +1,5 @@
+DIFF="$1"
+
+diff "$2" "$5"
+diff "$3" "$6"
+diff "$4" "$7"

From db4fbb111c4bf8d7d27bc7f88eb328e7a00a736d Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Thu, 14 May 2020 09:56:29 +0200
Subject: [PATCH 05/16] changelog entry about expand_location

---
 CHANGELOG.md | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 859663396..a12013bec 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,17 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
+## [Unreleased]
+
+[Unreleased]: https://github.com/tweag/rules_nixpkgs/compare/v0.7.0...HEAD
+
+### Added
+
+- Add `expand_location` attribute to `nixpkgs_package`. When enabled instances
+  of `$(location LABEL)` in the `nixopts` attribute will be expanded to the
+  file path of the file referenced by `LABEL`.
+  See [#128][#128].
+
 ## [0.7.0] - 2020-04-20
 
 [0.7.0]: https://github.com/tweag/rules_nixpkgs/compare/v0.6.0...v0.7.0

From 290c735264cadfba71e9bd176c1fd52806ec6222 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Fri, 17 Jul 2020 16:56:48 +0200
Subject: [PATCH 06/16] Location expansion escaping

---
 nixpkgs/nixpkgs.bzl | 84 ++++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 32 deletions(-)

diff --git a/nixpkgs/nixpkgs.bzl b/nixpkgs/nixpkgs.bzl
index 9a2eed455..2b4ecd82a 100644
--- a/nixpkgs/nixpkgs.bzl
+++ b/nixpkgs/nixpkgs.bzl
@@ -1,5 +1,6 @@
 """Rules for importing Nixpkgs packages."""
 
+load("@bazel_skylib//lib:paths.bzl", "paths")
 load("@bazel_tools//tools/cpp:cc_configure.bzl", "cc_autoconf_impl")
 load("@bazel_tools//tools/cpp:lib_cc_configure.bzl", "get_cpu_value")
 
@@ -76,6 +77,9 @@ def _is_supported_platform(repository_ctx):
 def _expand_location(repository_ctx, string, labels, attr = None):
     """Expand `$(location label)` to a path.
 
+    Raises an error on unexpected occurrences of `$`.
+    Use `$$` to insert a verbatim `$`.
+
     Attrs:
       repository_ctx: The repository rule context.
       string: string, Replace instances of `$(location )` in this string.
@@ -85,40 +89,56 @@ def _expand_location(repository_ctx, string, labels, attr = None):
     Returns:
       The string with all instances of `$(location )` replaced by paths.
     """
-    num = string.count("$(location ")
     result = ""
     offset = 0
-    for i in range(num):
-        start = string.find("$(location ", offset)
-        label_start = start + len("$(location ")
-        label_end = string.find(")", label_start)
-        if label_end == -1:
-            fail("Unbalanced parentheses in location expansion for '{}'.".format(string[start:]), attr)
-        end = label_end + 1
-        label_str = string[label_start:label_end]
-        label_candidates = [
-            (lbl, path)
-            for (lbl, path) in labels.items()
-            if lbl.relative(label_str) == lbl
-        ]
-        if len(label_candidates) == 0:
-            fail("Unknown label '{}' in location expansion for '{}'.".format(label_str, string), attr)
-        elif len(label_candidates) > 1:
-            fail(
-                "Ambiguous label '{}' in location expansion for '{}'. Candidates: {}".format(
-                    label_str,
-                    string,
-                    ", ".join([str(lbl) for lbl in label_candidates]),
-                ),
-                attr,
-            )
-        location = paths.join(".", paths.relativize(
-            str(repository_ctx.path(label_candidates[0][1])),
-            str(repository_ctx.path(".")),
-        ))
-        result += string[offset:start] + location
-        offset = end
-    result += string[offset:]
+    # Step through occurrences of `$`. This is bounded by the length of the string.
+    for _ in range(len(string)):
+        start = string.find("$", offset)
+        if start == -1:
+            result += string[offset:]
+            break
+        else:
+            result += string[offset:start]
+        if start + 1 == len(string):
+            fail("Unescaped '$' in location expansion at end of input", attr)
+        elif string[start + 1] == "$":
+            # Insert verbatim '$'.
+            result += "$"
+            offset = start + 2
+        elif string[start + 1] == "(":
+            group_start = start + 2
+            group_end = string.find(")", group_start)
+            if group_end == -1:
+                fail("Unbalanced parentheses in location expansion for '{}'.".format(string[start:]), attr)
+            group = string[group_start:group_end]
+            if group.startswith("location "):
+                label_str = group[len("location "):]
+                label_candidates = [
+                    (lbl, path)
+                    for (lbl, path) in labels.items()
+                    if lbl.relative(label_str) == lbl
+                ]
+                if len(label_candidates) == 0:
+                    fail("Unknown label '{}' in location expansion for '{}'.".format(label_str, string), attr)
+                elif len(label_candidates) > 1:
+                    fail(
+                        "Ambiguous label '{}' in location expansion for '{}'. Candidates: {}".format(
+                            label_str,
+                            string,
+                            ", ".join([str(lbl) for lbl in label_candidates]),
+                        ),
+                        attr,
+                    )
+                location = paths.join(".", paths.relativize(
+                    str(repository_ctx.path(label_candidates[0][1])),
+                    str(repository_ctx.path(".")),
+                ))
+                result += location
+            else:
+                fail("Unrecognized location expansion '$({})'.".format(group), attr)
+            offset = group_end + 1
+        else:
+            fail("Unescaped '$' in location expansion at position {} of input.".format(start), attr)
     return result
 
 def _nixpkgs_package_impl(repository_ctx):

From a307fe7811490786ccf3c99cedaa4a75de41ab0b Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Fri, 17 Jul 2020 17:21:32 +0200
Subject: [PATCH 07/16] Extend location-expansion test case for escaping

---
 WORKSPACE                               | 3 +++
 tests/BUILD.bazel                       | 4 ++++
 tests/location_expansion.nix            | 3 ++-
 tests/location_expansion.sh             | 7 ++++---
 tests/location_expansion/escaped_string | 1 +
 5 files changed, 14 insertions(+), 4 deletions(-)
 create mode 100644 tests/location_expansion/escaped_string

diff --git a/WORKSPACE b/WORKSPACE
index e2aa0869e..509f036a7 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -187,6 +187,9 @@ nixpkgs_package(
         "--arg",
         "relative_imports",
         "$(location @io_tweag_rules_nixpkgs//tests:relative_imports/nixpkgs.nix)",
+        "--argstr",
+        "escaped_string",
+        "$$ $$(location //:does-not-exist) )(",
     ],
     repository = "@remote_nixpkgs",
 )
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
index 4c5977676..a3f49ac58 100644
--- a/tests/BUILD.bazel
+++ b/tests/BUILD.bazel
@@ -63,17 +63,21 @@ sh_test(
         "$(rootpath //:nixpkgs.json)",
         "$(rootpath //:nixpkgs.nix)",
         "$(rootpath //tests:relative_imports/nixpkgs.nix)",
+        "$(rootpath //tests:location_expansion/escaped_string)",
         "$(rootpath @nixpkgs_location_expansion_test//:out/nixpkgs.json)",
         "$(rootpath @nixpkgs_location_expansion_test//:out/nixpkgs.nix)",
         "$(rootpath @nixpkgs_location_expansion_test//:out/relative_imports.nix)",
+        "$(rootpath @nixpkgs_location_expansion_test//:out/escaped_string)",
     ],
     data = [
         "//:nixpkgs.json",
         "//:nixpkgs.nix",
         "//tests:relative_imports/nixpkgs.nix",
+        "//tests:location_expansion/escaped_string",
         "@nixpkgs_location_expansion_test//:out/nixpkgs.json",
         "@nixpkgs_location_expansion_test//:out/nixpkgs.nix",
         "@nixpkgs_location_expansion_test//:out/relative_imports.nix",
+        "@nixpkgs_location_expansion_test//:out/escaped_string",
     ],
     toolchains = ["@rules_sh//sh/posix:make_variables"],
 )
diff --git a/tests/location_expansion.nix b/tests/location_expansion.nix
index 8023925b6..f5715c6f8 100644
--- a/tests/location_expansion.nix
+++ b/tests/location_expansion.nix
@@ -1,6 +1,6 @@
 with import <nixpkgs> { config = {}; overlays = []; };
 
-{ attrs, relative_imports }:
+{ attrs, relative_imports, escaped_string }:
 let
   inherit (attrs) nixpkgs_json nixpkgs_nix;
 in
@@ -14,4 +14,5 @@ in
       cp ${nixpkgs_json} $out/out/nixpkgs.json
       cp ${nixpkgs_nix} $out/out/nixpkgs.nix
       cp ${relative_imports} $out/out/relative_imports.nix
+      echo '${escaped_string}' >$out/out/escaped_string
     ''
diff --git a/tests/location_expansion.sh b/tests/location_expansion.sh
index 9f4df14b1..ff62292dd 100755
--- a/tests/location_expansion.sh
+++ b/tests/location_expansion.sh
@@ -1,5 +1,6 @@
 DIFF="$1"
 
-diff "$2" "$5"
-diff "$3" "$6"
-diff "$4" "$7"
+diff "$2" "$6"
+diff "$3" "$7"
+diff "$4" "$8"
+diff "$5" "$9"
diff --git a/tests/location_expansion/escaped_string b/tests/location_expansion/escaped_string
new file mode 100644
index 000000000..20c7f0655
--- /dev/null
+++ b/tests/location_expansion/escaped_string
@@ -0,0 +1 @@
+$ $(location //:does-not-exist) )(

From b9865964061f155518c7816ff321d3e3c2bd728a Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Fri, 17 Jul 2020 17:35:39 +0200
Subject: [PATCH 08/16] Update changelog entry

---
 CHANGELOG.md | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a12013bec..cc8bf78d0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,7 +13,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 - Add `expand_location` attribute to `nixpkgs_package`. When enabled instances
   of `$(location LABEL)` in the `nixopts` attribute will be expanded to the
   file path of the file referenced by `LABEL`.
-  See [#128][#128].
+  See [#132][#132].
+
+[#132]: https://github.com/tweag/rules_nixpkgs/pull/132
 
 ## [0.7.0] - 2020-04-20
 

From 23f252f545a14730f9b9207bd4f919131f5f9b7d Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Fri, 17 Jul 2020 17:43:47 +0200
Subject: [PATCH 09/16] load nixpkgs dependencies in test-case

---
 tests/invalid_nixpkgs_package/workspace.bazel | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/invalid_nixpkgs_package/workspace.bazel b/tests/invalid_nixpkgs_package/workspace.bazel
index c43464952..2113a7733 100644
--- a/tests/invalid_nixpkgs_package/workspace.bazel
+++ b/tests/invalid_nixpkgs_package/workspace.bazel
@@ -1,5 +1,9 @@
 workspace(name = "io_tweag_rules_nixpkgs")
 
+load("//nixpkgs:repositories.bzl", "rules_nixpkgs_dependencies")
+
+rules_nixpkgs_dependencies()
+
 load(
     "//nixpkgs:nixpkgs.bzl",
     "nixpkgs_local_repository",

From e639830500e2c0f022b4ab9f7da71a2ef7e85e3c Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Fri, 17 Jul 2020 17:44:07 +0200
Subject: [PATCH 10/16] define skylib dependency

---
 nixpkgs/BUILD.bazel | 1 +
 nixpkgs/nixpkgs.bzl | 1 +
 tests/BUILD.bazel   | 4 ++--
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/nixpkgs/BUILD.bazel b/nixpkgs/BUILD.bazel
index f055a773a..04c7c1723 100644
--- a/nixpkgs/BUILD.bazel
+++ b/nixpkgs/BUILD.bazel
@@ -41,5 +41,6 @@ bzl_library(
     visibility = ["//visibility:public"],
     deps = [
         ":bazel_tools",
+        "@bazel_skylib//lib:paths",
     ],
 )
diff --git a/nixpkgs/nixpkgs.bzl b/nixpkgs/nixpkgs.bzl
index 2b4ecd82a..91cb59ba3 100644
--- a/nixpkgs/nixpkgs.bzl
+++ b/nixpkgs/nixpkgs.bzl
@@ -91,6 +91,7 @@ def _expand_location(repository_ctx, string, labels, attr = None):
     """
     result = ""
     offset = 0
+
     # Step through occurrences of `$`. This is bounded by the length of the string.
     for _ in range(len(string)):
         start = string.find("$", offset)
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
index a3f49ac58..4d765cb69 100644
--- a/tests/BUILD.bazel
+++ b/tests/BUILD.bazel
@@ -72,12 +72,12 @@ sh_test(
     data = [
         "//:nixpkgs.json",
         "//:nixpkgs.nix",
-        "//tests:relative_imports/nixpkgs.nix",
         "//tests:location_expansion/escaped_string",
+        "//tests:relative_imports/nixpkgs.nix",
+        "@nixpkgs_location_expansion_test//:out/escaped_string",
         "@nixpkgs_location_expansion_test//:out/nixpkgs.json",
         "@nixpkgs_location_expansion_test//:out/nixpkgs.nix",
         "@nixpkgs_location_expansion_test//:out/relative_imports.nix",
-        "@nixpkgs_location_expansion_test//:out/escaped_string",
     ],
     toolchains = ["@rules_sh//sh/posix:make_variables"],
 )

From 7fa30506006591bda8fd330dac8bc8f673b7c828 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Wed, 29 Jul 2020 12:06:47 +0200
Subject: [PATCH 11/16] Don't gate nixopts location expansion

Addressing review comment
https://github.com/tweag/rules_nixpkgs/pull/132#issuecomment-665563883
---
 CHANGELOG.md        |  9 +++++----
 README.md           | 17 +++++------------
 WORKSPACE           |  1 -
 nixpkgs/nixpkgs.bzl | 12 ++++--------
 4 files changed, 14 insertions(+), 25 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cc8bf78d0..a1c6ddab2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,11 +8,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 [Unreleased]: https://github.com/tweag/rules_nixpkgs/compare/v0.7.0...HEAD
 
-### Added
+### Changed
 
-- Add `expand_location` attribute to `nixpkgs_package`. When enabled instances
-  of `$(location LABEL)` in the `nixopts` attribute will be expanded to the
-  file path of the file referenced by `LABEL`.
+- The values in the `nixopts` attribute to `nixpkgs_package` are now subject to
+  location expansion. Any instance of `$(location LABEL)` in the `nixopts`
+  attribute will be expanded to the file path of the file referenced by
+  `LABEL`. To pass a plain `$` to Nix it must be escaped as `$$`.
   See [#132][#132].
 
 [#132]: https://github.com/tweag/rules_nixpkgs/pull/132
diff --git a/README.md b/README.md
index 346216033..de7f269e1 100644
--- a/README.md
+++ b/README.md
@@ -176,7 +176,7 @@ Make the content of a Nixpkgs package available in the Bazel workspace.
 nixpkgs_package(
     name, attribute_path, nix_file, nix_file_deps, nix_file_content,
     repository, repositories, build_file, build_file_content, nixopts,
-    expand_location, fail_not_supported,
+    fail_not_supported,
 )
 ```
 
@@ -303,18 +303,11 @@ filegroup(
       <td><code>nixopts</code></td>
       <td>
         <p><code>String list; optional</code></p>
-        <p>Extra flags to pass when calling Nix.</p>
-      </td>
-    </tr>
-    <tr>
-      <td><code>nixopts</code></td>
-      <td>
-        <p><code>Bool; optional</code></p>
         <p>
-            If set to <code>True</code> any instance of
-            <code>$(location LABEL)</code> in <code>nixopts</code>
-            will be replaced by the path to the file referenced by
-            <code>LABEL</code> relative to the workspace root.
+            Extra flags to pass when calling Nix. Subject to location
+            expansion, any instance of <code>$(location LABEL)</code> will be
+            replaced by the path to the file ferenced by <code>LABEL</code>
+            relative to the workspace root.
         </p>
       </td>
     </tr>
diff --git a/WORKSPACE b/WORKSPACE
index 509f036a7..20337ae1e 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -173,7 +173,6 @@ nixpkgs_package(
 nixpkgs_package(
     name = "nixpkgs_location_expansion_test",
     build_file_content = "exports_files(glob(['out/**']))",
-    expand_location = True,
     nix_file = "//tests:location_expansion.nix",
     nix_file_deps = [
         "//:nixpkgs.json",
diff --git a/nixpkgs/nixpkgs.bzl b/nixpkgs/nixpkgs.bzl
index 91cb59ba3..c57a16b82 100644
--- a/nixpkgs/nixpkgs.bzl
+++ b/nixpkgs/nixpkgs.bzl
@@ -206,13 +206,10 @@ def _nixpkgs_package_impl(repository_ctx):
         "bazel-support/nix-out-link",
     ])
 
-    if repository_ctx.attr.expand_location:
-        expr_args.extend([
-            _expand_location(repository_ctx, opt, nix_file_deps, "nixopts")
-            for opt in repository_ctx.attr.nixopts
-        ])
-    else:
-        expr_args.extend(repository_ctx.attr.nixopts)
+    expr_args.extend([
+        _expand_location(repository_ctx, opt, nix_file_deps, "nixopts")
+        for opt in repository_ctx.attr.nixopts
+    ])
 
     for repo in repositories.keys():
         path = str(repository_ctx.path(repo).dirname) + "/nix-file-deps"
@@ -299,7 +296,6 @@ _nixpkgs_package = repository_rule(
         "build_file": attr.label(),
         "build_file_content": attr.string(),
         "nixopts": attr.string_list(),
-        "expand_location": attr.bool(default = False),
         "quiet": attr.bool(),
         "fail_not_supported": attr.bool(default = True, doc = """
             If set to True (default) this rule will fail on platforms which do not support Nix (e.g. Windows). If set to False calling this rule will succeed but no output will be generated.

From 4ee940f7f9ccdc66843717e8188c376b43ddcd73 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Wed, 29 Jul 2020 12:10:10 +0200
Subject: [PATCH 12/16] Factor out location expansion

Addressing review comment
https://github.com/tweag/rules_nixpkgs/pull/132#issuecomment-665563883
---
 nixpkgs/BUILD.bazel                    |  1 +
 nixpkgs/nixpkgs.bzl                    | 72 +-------------------------
 nixpkgs/private/location_expansion.bzl | 69 ++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 70 deletions(-)
 create mode 100644 nixpkgs/private/location_expansion.bzl

diff --git a/nixpkgs/BUILD.bazel b/nixpkgs/BUILD.bazel
index 04c7c1723..18f32ded3 100644
--- a/nixpkgs/BUILD.bazel
+++ b/nixpkgs/BUILD.bazel
@@ -37,6 +37,7 @@ bzl_library(
     name = "nixpkgs",
     srcs = [
         "nixpkgs.bzl",
+        "private/location_expansion.bzl",
     ],
     visibility = ["//visibility:public"],
     deps = [
diff --git a/nixpkgs/nixpkgs.bzl b/nixpkgs/nixpkgs.bzl
index c57a16b82..dbda3774b 100644
--- a/nixpkgs/nixpkgs.bzl
+++ b/nixpkgs/nixpkgs.bzl
@@ -1,8 +1,8 @@
 """Rules for importing Nixpkgs packages."""
 
-load("@bazel_skylib//lib:paths.bzl", "paths")
 load("@bazel_tools//tools/cpp:cc_configure.bzl", "cc_autoconf_impl")
 load("@bazel_tools//tools/cpp:lib_cc_configure.bzl", "get_cpu_value")
+load(":private/location_expansion.bzl", "expand_location")
 
 def _nixpkgs_git_repository_impl(repository_ctx):
     repository_ctx.file(
@@ -74,74 +74,6 @@ nixpkgs_local_repository = repository_rule(
 def _is_supported_platform(repository_ctx):
     return repository_ctx.which("nix-build") != None
 
-def _expand_location(repository_ctx, string, labels, attr = None):
-    """Expand `$(location label)` to a path.
-
-    Raises an error on unexpected occurrences of `$`.
-    Use `$$` to insert a verbatim `$`.
-
-    Attrs:
-      repository_ctx: The repository rule context.
-      string: string, Replace instances of `$(location )` in this string.
-      labels: dict from label to path: Known label to path mappings.
-      attr: string, The rule attribute to use for error reporting.
-
-    Returns:
-      The string with all instances of `$(location )` replaced by paths.
-    """
-    result = ""
-    offset = 0
-
-    # Step through occurrences of `$`. This is bounded by the length of the string.
-    for _ in range(len(string)):
-        start = string.find("$", offset)
-        if start == -1:
-            result += string[offset:]
-            break
-        else:
-            result += string[offset:start]
-        if start + 1 == len(string):
-            fail("Unescaped '$' in location expansion at end of input", attr)
-        elif string[start + 1] == "$":
-            # Insert verbatim '$'.
-            result += "$"
-            offset = start + 2
-        elif string[start + 1] == "(":
-            group_start = start + 2
-            group_end = string.find(")", group_start)
-            if group_end == -1:
-                fail("Unbalanced parentheses in location expansion for '{}'.".format(string[start:]), attr)
-            group = string[group_start:group_end]
-            if group.startswith("location "):
-                label_str = group[len("location "):]
-                label_candidates = [
-                    (lbl, path)
-                    for (lbl, path) in labels.items()
-                    if lbl.relative(label_str) == lbl
-                ]
-                if len(label_candidates) == 0:
-                    fail("Unknown label '{}' in location expansion for '{}'.".format(label_str, string), attr)
-                elif len(label_candidates) > 1:
-                    fail(
-                        "Ambiguous label '{}' in location expansion for '{}'. Candidates: {}".format(
-                            label_str,
-                            string,
-                            ", ".join([str(lbl) for lbl in label_candidates]),
-                        ),
-                        attr,
-                    )
-                location = paths.join(".", paths.relativize(
-                    str(repository_ctx.path(label_candidates[0][1])),
-                    str(repository_ctx.path(".")),
-                ))
-                result += location
-            else:
-                fail("Unrecognized location expansion '$({})'.".format(group), attr)
-            offset = group_end + 1
-        else:
-            fail("Unescaped '$' in location expansion at position {} of input.".format(start), attr)
-    return result
-
 def _nixpkgs_package_impl(repository_ctx):
     repository = repository_ctx.attr.repository
     repositories = repository_ctx.attr.repositories
@@ -207,7 +139,7 @@ def _nixpkgs_package_impl(repository_ctx):
     ])
 
     expr_args.extend([
-        _expand_location(repository_ctx, opt, nix_file_deps, "nixopts")
+        expand_location(repository_ctx, opt, nix_file_deps, "nixopts")
         for opt in repository_ctx.attr.nixopts
     ])
 
diff --git a/nixpkgs/private/location_expansion.bzl b/nixpkgs/private/location_expansion.bzl
new file mode 100644
index 000000000..20b23700c
--- /dev/null
+++ b/nixpkgs/private/location_expansion.bzl
@@ -0,0 +1,69 @@
+load("@bazel_skylib//lib:paths.bzl", "paths")
+
+def expand_location(repository_ctx, string, labels, attr = None):
+    """Expand `$(location label)` to a path.
+
+    Raises an error on unexpected occurrences of `$`.
+    Use `$$` to insert a verbatim `$`.
+
+    Attrs:
+      repository_ctx: The repository rule context.
+      string: string, Replace instances of `$(location )` in this string.
+      labels: dict from label to path: Known label to path mappings.
+      attr: string, The rule attribute to use for error reporting.
+
+    Returns:
+      The string with all instances of `$(location )` replaced by paths.
+    """
+    result = ""
+    offset = 0
+
+    # Step through occurrences of `$`. This is bounded by the length of the string.
+    for _ in range(len(string)):
+        start = string.find("$", offset)
+        if start == -1:
+            result += string[offset:]
+            break
+        else:
+            result += string[offset:start]
+        if start + 1 == len(string):
+            fail("Unescaped '$' in location expansion at end of input", attr)
+        elif string[start + 1] == "$":
+            # Insert verbatim '$'.
+            result += "$"
+            offset = start + 2
+        elif string[start + 1] == "(":
+            group_start = start + 2
+            group_end = string.find(")", group_start)
+            if group_end == -1:
+                fail("Unbalanced parentheses in location expansion for '{}'.".format(string[start:]), attr)
+            group = string[group_start:group_end]
+            if group.startswith("location "):
+                label_str = group[len("location "):]
+                label_candidates = [
+                    (lbl, path)
+                    for (lbl, path) in labels.items()
+                    if lbl.relative(label_str) == lbl
+                ]
+                if len(label_candidates) == 0:
+                    fail("Unknown label '{}' in location expansion for '{}'.".format(label_str, string), attr)
+                elif len(label_candidates) > 1:
+                    fail(
+                        "Ambiguous label '{}' in location expansion for '{}'. Candidates: {}".format(
+                            label_str,
+                            string,
+                            ", ".join([str(lbl) for lbl in label_candidates]),
+                        ),
+                        attr,
+                    )
+                location = paths.join(".", paths.relativize(
+                    str(repository_ctx.path(label_candidates[0][1])),
+                    str(repository_ctx.path(".")),
+                ))
+                result += location
+            else:
+                fail("Unrecognized location expansion '$({})'.".format(group), attr)
+            offset = group_end + 1
+        else:
+            fail("Unescaped '$' in location expansion at position {} of input.".format(start), attr)
+    return result

From b89539c0e76f44a77ebc844857690f45fb4d8d55 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Tue, 10 Nov 2020 12:16:20 +0100
Subject: [PATCH 13/16] Use named parameters

Addressing review comment
https://github.com/tweag/rules_nixpkgs/pull/132#discussion_r520401312
---
 nixpkgs/nixpkgs.bzl | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/nixpkgs/nixpkgs.bzl b/nixpkgs/nixpkgs.bzl
index dbda3774b..40308ed98 100644
--- a/nixpkgs/nixpkgs.bzl
+++ b/nixpkgs/nixpkgs.bzl
@@ -139,7 +139,12 @@ def _nixpkgs_package_impl(repository_ctx):
     ])
 
     expr_args.extend([
-        expand_location(repository_ctx, opt, nix_file_deps, "nixopts")
+        expand_location(
+            repository_ctx = repository_ctx,
+            string = opt,
+            labels = nix_file_deps,
+            attr = "nixopts",
+        )
         for opt in repository_ctx.attr.nixopts
     ])
 

From 3053009f378d44ae42086e510bcde8fe9a5b9e94 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Tue, 10 Nov 2020 13:41:42 +0100
Subject: [PATCH 14/16] Location expansion using pure functions

Addressing review comment
https://github.com/tweag/rules_nixpkgs/pull/132#discussion_r520431428

Implement parsing of location expansion commands and label resolution as
pure functions that do not depend on the repository context. This allows
to test these functions in unit tests.
---
 nixpkgs/private/location_expansion.bzl | 153 +++++++++++++++++--------
 1 file changed, 106 insertions(+), 47 deletions(-)

diff --git a/nixpkgs/private/location_expansion.bzl b/nixpkgs/private/location_expansion.bzl
index 20b23700c..069058643 100644
--- a/nixpkgs/private/location_expansion.bzl
+++ b/nixpkgs/private/location_expansion.bzl
@@ -1,69 +1,128 @@
 load("@bazel_skylib//lib:paths.bzl", "paths")
 
-def expand_location(repository_ctx, string, labels, attr = None):
-    """Expand `$(location label)` to a path.
+def parse_expand_location(string):
+    """Parse a string that might contain location expansion commands.
 
-    Raises an error on unexpected occurrences of `$`.
-    Use `$$` to insert a verbatim `$`.
+    Generates a list of pairs of command and argument.
+    The command can have the following values:
+    - `string`: argument is a string, append it to the result.
+    - `location`: argument is a label, append its location to the result.
 
     Attrs:
-      repository_ctx: The repository rule context.
-      string: string, Replace instances of `$(location )` in this string.
-      labels: dict from label to path: Known label to path mappings.
-      attr: string, The rule attribute to use for error reporting.
+      string: string, The string to parse.
 
     Returns:
-      The string with all instances of `$(location )` replaced by paths.
+      (result, error):
+        result: The generated list of pairs of command and argument.
+        error: string or None, This is set if an error occurred.
     """
-    result = ""
+    result = []
     offset = 0
+    len_string = len(string)
 
     # Step through occurrences of `$`. This is bounded by the length of the string.
-    for _ in range(len(string)):
-        start = string.find("$", offset)
-        if start == -1:
-            result += string[offset:]
+    for _ in range(len_string):
+        # Find the position of the next `$`.
+        position = string.find("$", offset)
+        if position == -1:
+            position = len_string
+
+        # Append the in-between literal string.
+        if offset < position:
+            result.append(("string", string[offset:position]))
+
+        # Terminate at the end of the string.
+        if position == len_string:
             break
-        else:
-            result += string[offset:start]
-        if start + 1 == len(string):
-            fail("Unescaped '$' in location expansion at end of input", attr)
-        elif string[start + 1] == "$":
+
+        # Parse the `$` command.
+        if string[position:].startswith("$$"):
             # Insert verbatim '$'.
-            result += "$"
-            offset = start + 2
-        elif string[start + 1] == "(":
-            group_start = start + 2
+            result.append(("string", "$"))
+            offset = position + 2
+        elif string[position:].startswith("$("):
+            # Expand a location command.
+            group_start = position + 2
             group_end = string.find(")", group_start)
             if group_end == -1:
-                fail("Unbalanced parentheses in location expansion for '{}'.".format(string[start:]), attr)
+                return (None, "Unbalanced parentheses in location expansion for '{}'.".format(string[position:]))
+
             group = string[group_start:group_end]
+            command = None
             if group.startswith("location "):
                 label_str = group[len("location "):]
-                label_candidates = [
-                    (lbl, path)
-                    for (lbl, path) in labels.items()
-                    if lbl.relative(label_str) == lbl
-                ]
-                if len(label_candidates) == 0:
-                    fail("Unknown label '{}' in location expansion for '{}'.".format(label_str, string), attr)
-                elif len(label_candidates) > 1:
-                    fail(
-                        "Ambiguous label '{}' in location expansion for '{}'. Candidates: {}".format(
-                            label_str,
-                            string,
-                            ", ".join([str(lbl) for lbl in label_candidates]),
-                        ),
-                        attr,
-                    )
-                location = paths.join(".", paths.relativize(
-                    str(repository_ctx.path(label_candidates[0][1])),
-                    str(repository_ctx.path(".")),
-                ))
-                result += location
+                command = ("location", label_str)
             else:
-                fail("Unrecognized location expansion '$({})'.".format(group), attr)
+                return (None, "Unrecognized location expansion '$({})'.".format(group))
+
+            result.append(command)
             offset = group_end + 1
         else:
-            fail("Unescaped '$' in location expansion at position {} of input.".format(start), attr)
+            return (None, "Unescaped '$' in location expansion at position {} of input.".format(position))
+
+    return (result, None)
+
+def resolve_label(label_str, labels):
+    """Find the label that corresponds to the given string.
+
+    Attr:
+      label_str: string, String representation of a label.
+      labels: dict from Label to path: Known label to path mappings.
+
+    Returns:
+      (path, error):
+        path: path, The path to the resolved label
+        error: string or None, This is set if an error occurred.
+    """
+    label_candidates = [
+        (lbl, path)
+        for (lbl, path) in labels.items()
+        if lbl.relative(label_str) == lbl
+    ]
+
+    if len(label_candidates) == 0:
+        return (None, "Unknown label '{}' in location expansion.".format(label_str))
+    elif len(label_candidates) > 1:
+        return (None, "Ambiguous label '{}' in location expansion. Candidates: {}".format(
+            label_str,
+            ", ".join([str(lbl) for (lbl, _) in label_candidates]),
+        ))
+
+    return (label_candidates[0][1], None)
+
+def expand_location(repository_ctx, string, labels, attr = None):
+    """Expand `$(location label)` to a path.
+
+    Raises an error on unexpected occurrences of `$`.
+    Use `$$` to insert a verbatim `$`.
+
+    Attrs:
+      repository_ctx: The repository rule context.
+      string: string, Replace instances of `$(location )` in this string.
+      labels: dict from label to path: Known label to path mappings.
+      attr: string, The rule attribute to use for error reporting.
+
+    Returns:
+      The string with all instances of `$(location )` replaced by paths.
+    """
+    (parsed, error) = parse_expand_location(string)
+    if error != None:
+        fail(error, attr)
+
+    result = ""
+    for (command, argument) in parsed:
+        if command == "string":
+            result += argument
+        elif command == "location":
+            (label, error) = resolve_label(argument, labels)
+            if error != None:
+                fail(error, attr)
+
+            result += paths.join(".", paths.relativize(
+                str(repository_ctx.path(label)),
+                str(repository_ctx.path(".")),
+            ))
+        else:
+            fail("Internal error: Unknown location expansion command '{}'.".format(command), attr)
+
     return result

From d8e21baf3604b11a6c3d298587b4c396277010d1 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Tue, 10 Nov 2020 18:19:29 +0100
Subject: [PATCH 15/16] Define unit tests for location expansion

Addressing review comment
https://github.com/tweag/rules_nixpkgs/pull/132#discussion_r520405151
---
 WORKSPACE                              |   4 +
 tests/BUILD.bazel                      |   3 +
 tests/location_expansion_unit_test.bzl | 145 +++++++++++++++++++++++++
 3 files changed, 152 insertions(+)
 create mode 100644 tests/location_expansion_unit_test.bzl

diff --git a/WORKSPACE b/WORKSPACE
index 20337ae1e..2eb37704c 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -16,6 +16,10 @@ load(
 
 # For tests
 
+load("@bazel_skylib//lib:unittest.bzl", "register_unittest_toolchains")
+
+register_unittest_toolchains()
+
 nixpkgs_git_repository(
     name = "remote_nixpkgs",
     remote = "https://github.com/NixOS/nixpkgs",
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
index 4d765cb69..8f0662abb 100644
--- a/tests/BUILD.bazel
+++ b/tests/BUILD.bazel
@@ -1,6 +1,9 @@
 package(default_testonly = 1)
 
 load("@io_bazel_rules_go//go:def.bzl", "go_binary")
+load(":location_expansion_unit_test.bzl", "expand_location_unit_test_suite")
+
+expand_location_unit_test_suite()
 
 [
     # All of these tests use the "hello" binary to see
diff --git a/tests/location_expansion_unit_test.bzl b/tests/location_expansion_unit_test.bzl
new file mode 100644
index 000000000..a924fe57c
--- /dev/null
+++ b/tests/location_expansion_unit_test.bzl
@@ -0,0 +1,145 @@
+load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
+load(
+    "//nixpkgs:private/location_expansion.bzl",
+    "parse_expand_location",
+    "resolve_label",
+)
+
+def _parse_expand_location_test(ctx):
+    env = unittest.begin(ctx)
+
+    asserts.equals(
+        env,
+        expected = ([], None),
+        actual = parse_expand_location(""),
+        msg = "Parses the empty string",
+    )
+
+    asserts.equals(
+        env,
+        expected = ([("string", "plain string")], None),
+        actual = parse_expand_location("plain string"),
+        msg = "Parses a plain string",
+    )
+
+    asserts.equals(
+        env,
+        expected = ([("string", "$")], None),
+        actual = parse_expand_location("$$"),
+        msg = "Parses an escaped dollar sign",
+    )
+
+    asserts.equals(
+        env,
+        expected = ([("location", "@workspace//package:target")], None),
+        actual = parse_expand_location("$(location @workspace//package:target)"),
+        msg = "Parses a location command",
+    )
+
+    asserts.equals(
+        env,
+        expected = ([
+            ("string", "before "),
+            ("location", "//label:1"),
+            ("string", " "),
+            ("string", "$"),
+            ("string", " "),
+            ("location", "//label:2"),
+            ("string", " after"),
+        ], None),
+        actual = parse_expand_location(
+            "before $(location //label:1) $$ $(location //label:2) after",
+        ),
+        msg = "Parses a complex location expansion string",
+    )
+
+    asserts.equals(
+        env,
+        expected = (None, "Unescaped '$' in location expansion at position 0 of input."),
+        actual = parse_expand_location("$"),
+        msg = "Fails on unescaped dollar sign",
+    )
+
+    asserts.equals(
+        env,
+        expected = (None, "Unbalanced parentheses in location expansion for '$(location //label:1'."),
+        actual = parse_expand_location("$(location //label:1"),
+        msg = "Fails on unbalanced parentheses",
+    )
+
+    asserts.equals(
+        env,
+        expected = (None, "Unrecognized location expansion '$(misspelled)'."),
+        actual = parse_expand_location("$(misspelled)"),
+        msg = "Fails on unknown location expansion command",
+    )
+
+    return unittest.end(env)
+
+parse_expand_location_test = unittest.make(_parse_expand_location_test)
+
+def _resolve_label_test(ctx):
+    env = unittest.begin(ctx)
+
+    asserts.equals(
+        env,
+        expected = ("correct/path", None),
+        actual = resolve_label(
+            "@workspace//package:target",
+            {
+                Label("@workspace//package:target"): "correct/path",
+                Label("@another//package:target"): "wrong/path",
+            },
+        ),
+        msg = "Finds an absolute label",
+    )
+
+    asserts.equals(
+        env,
+        expected = ("correct/path", None),
+        actual = resolve_label(
+            "//package:target",
+            {
+                Label("@workspace//package:target"): "correct/path",
+                Label("@another//different:target"): "wrong/path",
+            },
+        ),
+        msg = "Finds an unambiguous relative label",
+    )
+
+    asserts.equals(
+        env,
+        expected = (None, "Unknown label '@unknown//package:target' in location expansion."),
+        actual = resolve_label(
+            "@unknown//package:target",
+            {
+                Label("@workspace//package:target"): "wrong/path",
+                Label("@another//package:target"): "another/wrong/path",
+            },
+        ),
+        msg = "Fails on an unknown label",
+    )
+
+    asserts.equals(
+        env,
+        expected = (None, "Ambiguous label '//package:target' in location expansion. Candidates: @workspace//package:target, @another//package:target"),
+        actual = resolve_label(
+            "//package:target",
+            {
+                Label("@workspace//package:target"): "wrong/path",
+                Label("@another//package:target"): "another/wrong/path",
+            },
+        ),
+        msg = "Fails on an ambiguous relative label",
+    )
+
+    return unittest.end(env)
+
+resolve_label_test = unittest.make(_resolve_label_test)
+
+def expand_location_unit_test_suite():
+    unittest.suite(
+        "expand_location_unit_test_suite",
+        parse_expand_location_test,
+        resolve_label_test,
+    )

From 057fa29c2c1a316882d8b792d1db95d6ed22d541 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <andreas.herrmann@tweag.io>
Date: Wed, 11 Nov 2020 15:49:14 +0100
Subject: [PATCH 16/16] Simplify the location expansion integration test

It now only tests the correct mapping of paths in the local workspace
and in an external workspace. The parsing of more complicated location
strings is tested in the unit tests.

Addressing review comment
https://github.com/tweag/rules_nixpkgs/pull/132#discussion_r520405151
---
 WORKSPACE                                     | 21 +++++++++---------
 tests/BUILD.bazel                             | 22 +++++--------------
 tests/location_expansion.nix                  |  8 +++----
 tests/location_expansion.sh                   | 15 +++++++++----
 tests/location_expansion/escaped_string       |  1 -
 tests/location_expansion/test_file            | 11 ++++++++++
 .../location_expansion/test_repo/BUILD.bazel  |  1 +
 tests/location_expansion/test_repo/WORKSPACE  |  0
 tests/location_expansion/test_repo/test_file  |  1 +
 9 files changed, 44 insertions(+), 36 deletions(-)
 delete mode 100644 tests/location_expansion/escaped_string
 create mode 100644 tests/location_expansion/test_file
 create mode 100644 tests/location_expansion/test_repo/BUILD.bazel
 create mode 100644 tests/location_expansion/test_repo/WORKSPACE
 create mode 120000 tests/location_expansion/test_repo/test_file

diff --git a/WORKSPACE b/WORKSPACE
index 2eb37704c..85143ecc4 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -174,25 +174,26 @@ nixpkgs_package(
     repository = "@remote_nixpkgs",
 )
 
+local_repository(
+    name = "nixpkgs_location_expansion_test_file",
+    path = "tests/location_expansion/test_repo",
+)
+
 nixpkgs_package(
     name = "nixpkgs_location_expansion_test",
     build_file_content = "exports_files(glob(['out/**']))",
     nix_file = "//tests:location_expansion.nix",
     nix_file_deps = [
-        "//:nixpkgs.json",
-        "//:nixpkgs.nix",
-        "@io_tweag_rules_nixpkgs//tests:relative_imports/nixpkgs.nix",
+        "//tests:location_expansion/test_file",
+        "@nixpkgs_location_expansion_test_file//:test_file",
     ],
     nixopts = [
         "--arg",
-        "attrs",
-        "{ nixpkgs_json = $(location //:nixpkgs.json); nixpkgs_nix = $(location //:nixpkgs.nix); }",
+        "local_file",
+        "$(location //tests:location_expansion/test_file)",
         "--arg",
-        "relative_imports",
-        "$(location @io_tweag_rules_nixpkgs//tests:relative_imports/nixpkgs.nix)",
-        "--argstr",
-        "escaped_string",
-        "$$ $$(location //:does-not-exist) )(",
+        "external_file",
+        "$(location @nixpkgs_location_expansion_test_file//:test_file)",
     ],
     repository = "@remote_nixpkgs",
 )
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
index 8f0662abb..e782f747e 100644
--- a/tests/BUILD.bazel
+++ b/tests/BUILD.bazel
@@ -63,24 +63,14 @@ sh_test(
     srcs = ["location_expansion.sh"],
     args = [
         "$(POSIX_DIFF)",
-        "$(rootpath //:nixpkgs.json)",
-        "$(rootpath //:nixpkgs.nix)",
-        "$(rootpath //tests:relative_imports/nixpkgs.nix)",
-        "$(rootpath //tests:location_expansion/escaped_string)",
-        "$(rootpath @nixpkgs_location_expansion_test//:out/nixpkgs.json)",
-        "$(rootpath @nixpkgs_location_expansion_test//:out/nixpkgs.nix)",
-        "$(rootpath @nixpkgs_location_expansion_test//:out/relative_imports.nix)",
-        "$(rootpath @nixpkgs_location_expansion_test//:out/escaped_string)",
+        "$(rootpath //tests:location_expansion/test_file)",
+        "$(rootpath @nixpkgs_location_expansion_test//:out/local_file)",
+        "$(rootpath @nixpkgs_location_expansion_test//:out/external_file)",
     ],
     data = [
-        "//:nixpkgs.json",
-        "//:nixpkgs.nix",
-        "//tests:location_expansion/escaped_string",
-        "//tests:relative_imports/nixpkgs.nix",
-        "@nixpkgs_location_expansion_test//:out/escaped_string",
-        "@nixpkgs_location_expansion_test//:out/nixpkgs.json",
-        "@nixpkgs_location_expansion_test//:out/nixpkgs.nix",
-        "@nixpkgs_location_expansion_test//:out/relative_imports.nix",
+        "//tests:location_expansion/test_file",
+        "@nixpkgs_location_expansion_test//:out/external_file",
+        "@nixpkgs_location_expansion_test//:out/local_file",
     ],
     toolchains = ["@rules_sh//sh/posix:make_variables"],
 )
diff --git a/tests/location_expansion.nix b/tests/location_expansion.nix
index f5715c6f8..d7639ba7f 100644
--- a/tests/location_expansion.nix
+++ b/tests/location_expansion.nix
@@ -1,6 +1,6 @@
 with import <nixpkgs> { config = {}; overlays = []; };
 
-{ attrs, relative_imports, escaped_string }:
+{ local_file, external_file }:
 let
   inherit (attrs) nixpkgs_json nixpkgs_nix;
 in
@@ -11,8 +11,6 @@ in
     }
     ''
       mkdir -p $out/out
-      cp ${nixpkgs_json} $out/out/nixpkgs.json
-      cp ${nixpkgs_nix} $out/out/nixpkgs.nix
-      cp ${relative_imports} $out/out/relative_imports.nix
-      echo '${escaped_string}' >$out/out/escaped_string
+      cp ${local_file} $out/out/local_file
+      cp ${external_file} $out/out/external_file
     ''
diff --git a/tests/location_expansion.sh b/tests/location_expansion.sh
index ff62292dd..5f60e2ae8 100755
--- a/tests/location_expansion.sh
+++ b/tests/location_expansion.sh
@@ -1,6 +1,13 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+# USAGE:
+# location_expansion.sh DIFF REFERENCE FILE...
+#
+# Compares the given files to the reference file and fails if there is a difference.
 DIFF="$1"
+REFERENCE="$2"
 
-diff "$2" "$6"
-diff "$3" "$7"
-diff "$4" "$8"
-diff "$5" "$9"
+for file in "${@:3}"; do
+  "$DIFF" "$file" "$REFERENCE"
+done
diff --git a/tests/location_expansion/escaped_string b/tests/location_expansion/escaped_string
deleted file mode 100644
index 20c7f0655..000000000
--- a/tests/location_expansion/escaped_string
+++ /dev/null
@@ -1 +0,0 @@
-$ $(location //:does-not-exist) )(
diff --git a/tests/location_expansion/test_file b/tests/location_expansion/test_file
new file mode 100644
index 000000000..b954d82d1
--- /dev/null
+++ b/tests/location_expansion/test_file
@@ -0,0 +1,11 @@
+# A few random numbers
+3030205
+68380721
+93196393
+70110283
+92132420
+98602836
+37655207
+12502743
+73421895
+75878115
diff --git a/tests/location_expansion/test_repo/BUILD.bazel b/tests/location_expansion/test_repo/BUILD.bazel
new file mode 100644
index 000000000..af49d1ebb
--- /dev/null
+++ b/tests/location_expansion/test_repo/BUILD.bazel
@@ -0,0 +1 @@
+exports_files(glob(["*"]))
diff --git a/tests/location_expansion/test_repo/WORKSPACE b/tests/location_expansion/test_repo/WORKSPACE
new file mode 100644
index 000000000..e69de29bb
diff --git a/tests/location_expansion/test_repo/test_file b/tests/location_expansion/test_repo/test_file
new file mode 120000
index 000000000..a525bb2c1
--- /dev/null
+++ b/tests/location_expansion/test_repo/test_file
@@ -0,0 +1 @@
+../test_file
\ No newline at end of file