Add support for isolated extension usages to the lockfile

[fmeum]

Previously, Bazel would crash if a module declares an isolated extension usage and the lockfile is used.

[fmeum]

@bazel-io flag

[meteorcloudy]

@bazel-io fork 6.3.0

[Wyverald]

this is quite a lot of code. could we simplify this by folding everything into a string, something like //abc.bzl%ext_name[%isolation_key]?

[fmeum]

I was thinking about this as well, but then found this to be confusing in its own way as we would essentially concatenate a variable number of up to five distinct pieces of information into a single string. If you prefer it that way I can certainly implement it, I just slightly leaned towards the current solution.

[Wyverald]

actually, we may have to concatenate everything here, since a ModuleExtensionId is used as a JSON object key for the extension map. I just remembered that this was why we had a concatenated string for ModuleExtensionId (instead of the normal JSON object representation) in the first place. But then again, your test passed... so I'm not quite sure what's happening here. I wonder what the lockfile actually looks like in that test case.

[fmeum]

Instead of a map, it has a list of pairs, but that was the case already before my change.

[fmeum]

Should we explore going back to the regular object representation for everything?

[Wyverald]

Are you sure? With a simple local test case, this is what I see in the lockfile with Bazel HEAD:

"moduleExtensions": {
    "//:ext.bzl%ext": {
      "bzlTransitiveDigest": "X8hz9J00k/ncSFM8d3lFoFATXO4NhBFr5xuqfhWiRs0=",
      "envVariables": {},
      "generatedRepoSpecs": {
        "kek": {
          "bzlFile": "@@//:ext.bzl",
          "ruleClassName": "rr",
          "attributes": {"name":"--_main~ext~kek"}
        }
      }
    }
  }

EDIT: same with 6.3.0rc1

[fmeum]

You're right. What's your preference? Strings, list of pairs or the current solution that's a max of both? Probably not the latter, I suppose.

[Wyverald]

I actually prefer the string version. It's terser, and IMO more human-readable, and human readability is a desirable property in the lockfile despite its main "audience" being Bazel itself.

[Wyverald]

In fact, I wanted to go a bit further and squash things like Starlark's Location into a string (so "abc.bzl:5:6" instead of {"file":"abc.bzl","line":5,"column":6}. But that would require bumping the lockfile version so I thought to do it later.

[fmeum]

Makes sense, I can change this tomorrow.

Regarding compressing location: Wouldn't now be a good time given that lockfiles haven't been enabled by default in a release yet?

[Wyverald]

nice! I like this very much :)

[SalmaSamy]

I guess there is no need for tags, environ and this printing code here

[SalmaSamy]

Maybe another test to check changing "isolate" causes re-evaluation as it should be treated as a new different extension? Also, make sure that the previously saved one is removed?

[SalmaSamy]

I think we should test here that if we have two usages one isolated & one not this should result in two extensions in the lockfile?

[fmeum]

@SalmaSamy The PR has already been merged, but I will submit another one with the changes and additions to the tests you proposed.

[SalmaSamy]

The PR has already been merged, but I will submit another one with the changes and additions to the tests you proposed.

@fmeum Thanks for that!