Add optional location expansion for the nixopts attribute

[aherrmann]

This factors out the addition of location expansion for the nixopts attribute from #128.

This adds an additional attribute expand_location to nixpkgs_package. When set to True location expansion will be performed on the value of the nixopts attribute. This performs location expansion on the value of the nixopts attribute to nixpkgs_packge. I.e. any occurrence of $(location LABEL) will be expanded to the path to the file referenced by LABEL. This feature is motivated by #128 where we need to predict the file path of a dependency from within a macro invoking repository rules.

This also adds a test-case for location expansion.

[support-dpulls]

Hey there! Walid from Dpulls here.
We recently noticed a bug that happens when the Dpulls status check is set as required (link of the issue: dpulls/backlog#10) blocking some PRs from being merged. This PR seems to be concerned by this issue.
We suggest you to temporarily uncheck Dpulls from your branch protection rule until we fix the issue. We will let you know once it is done.
Thanks.
cc @aherrmann

[mboes]

@aherrmann why not do this always? i.e. not gate the location expansion by a flag?

[aherrmann]

@aherrmann why not do this always? i.e. not gate the location expansion by a flag?

The motivation was to not make this a breaking change. Prior uses of nixopts could be using $, e.g. "${foo}/bar", which would now be illegal and should instead be "$${foo}/bar".

[mboes]

I'd go for making this a breaking change. We're otherwise stuck with a more complicated API that breaks precedent with the way location expansion in other rules work (where i've never seen this be gated by a flag). We could introduce expand_location for now and emit a warning if we see $ in nixopts, thus offering a deprecation period to users. But that hardly seems worth it, since this is small corner case that will affect very few users if any.

Alternatively, maybe it's possible to steal less syntax? Allow ${foo}/bar but also $(location ...). It appears that in Bazel Make variable expansion, $ needs to be escaped only because they allow shorthands. But we only need $(location ...). In fact it's possible that the parsing code might end up simpler: look for a $(location , then a ) somewhere later. Anything in between is deemed to be a label (which we can optionally further sanity check). Essentially, we'd be implementing only the equivalent of ctx.expand_location(), not ctx.expand_make_variables().

Nit: I'd put the expand_location() thing in its own source file. It's a complex enough function that is completely generic and that others may want to use by copying the source file.

[aherrmann]

I'd go for making this a breaking change. We're otherwise stuck with a more complicated API that breaks precedent with the way location expansion in other rules work (where i've never seen this be gated by a flag). We could introduce expand_location for now and emit a warning if we see $ in nixopts, thus offering a deprecation period to users. But that hardly seems worth it, since this is small corner case that will affect very few users if any.

Fair enough, I'll remove the gating.

It appears that in Bazel Make variable expansion, $ needs to be escaped only because they allow shorthands. But we only need $(location ...). In fact it's possible that the parsing code might end up simpler: look for a $(location , then a ) somewhere later. Anything in between is deemed to be a label (which we can optionally further sanity check).

That's what the initial implementation did, however based on feedback I moved it to be consistent with Bazel's behavior.

[aherrmann]

I've removed the gating and factored out the location expansion into its own module.

[Profpatsch]

The feature makes sense to me, but the tests and implementation are a bit involved and rather hard to understand (I couldn’t fit everything into my head during review without spending undue amounts of time on it). More separation into smaller tests/subroutines would help.

[Profpatsch]

use attr keys for easier reading

[aherrmann]

Done

[Profpatsch]

This test is extremely convoluted, it has so many moving parts that it’s hard to understand what it does or why. Needs either a more detailed description of what is done and why, or be split up into more tests that each do a lot less.

[Profpatsch]

For example, the location expansion stuff is just a string replacement, and the algo could be tested as a unit-test entirely; which would mean the integration test needs to test less.

[aherrmann]

With the factored out pure parsing and label resolution functions it is now possible to define unit tests for these, which I've done. I've simplified this integration test to only test that the mapping to paths works correctly for a file in the main repository and for one in an external repository.

[Profpatsch]

This function is a bit too unwieldy for my taste.

I would have said we should split it up into generators, but skylark does not have them. However, the location strings should be short enough for it not to matter.

So e.g. have one function which returns all “$ to end” strings for $ that are not escaped, one function tries to parse the location syntax, and then one function which replaces the location syntax with its actual content.

[aherrmann]

I've factored out the parsing and label resolution into separate pure functions. With this change it becomes possible to test them in unit tests. I've also changed the parsing logic a bit to make it clearer.

[Profpatsch]

This looks great now! I’m confident that it does what we want and is well-tested.