Add load() statements for Python rules in third_party

[brandjon]

This PR is necessary to unblock #9006, which blocks the next Bazel release. However, I'm running into some trouble getting the load paths to work. Note that this PR depends on cl/260716838 which is as of this writing in-flight.

The trouble with load paths is that this makes third_party packages load a bzl underneath //tools, This works for some third_party packages, but six and protobuf get turned into local external repos by Bazel's own WORKSPACE file, which causes the labels to resolve relative to @six and @com_google_protobuf respectively.

I can sort of fix this by qualifying the label with @io_bazel, except that this breaks six's use inside of @bazel_tools, which cannot access @io_bazel.

So my ideas are

• Add some kind of repository renaming to the generation of @bazel_tools, to rewrite @io_bazel as @bazel_tools or the main repo name. This seems ugly and like it will confuse future debugging.

• Use labels that are not qualified by repo name and therefore work both within @io_bazel and @bazel_tools. Add some kind of rewriting step to the repo rules that produce @six and @com_google_protobuf within Bazel's WORKSPACE.

• Don't add a dependency on the //tools/... bzl at all, but instead inline that file into each third_party package that needs it.

Finally, I want to say I've never modified third_party before so I don't know what our practices are regarding hand-crafted changes to vendored sources. I'm happy to tweak things by adding explanatory comments, but this change is somewhat time-sensitive (cut date is supposed to be August 1).

[brandjon]

Discussed with @laszlocsomor. Resolution is:

1. We add load() statements as needed across third_party/
2. We add a copy of defs.bzl to those third_party/ packages that form their own repos (six and protobuf)
3. We document these changes in the updating-protobuf readme (and whatever other applicable readme there is). This is an ongoing maintenance cost whenever new versions of protobuf and others are imported.
4. Eventually, protobuf upstream migrates to depend on @rules_python, and we change our own loads across Bazel to also use @rules_python instead of defs.bzl (except in @bazel_tools, which will continue to use this hack). At that point the third_party manual edits go away.

[laszlocsomor]

SGTM.

I think in the long term we'd benefit from purging six and protobuf (and whatever else we can) from bazel_tools to avoid needing such hacks, and require users to import in their WORKSPACE all language rules and tools they need.

[katre]

Note that db063a8 (internally cl/260716838 from the PR description) is now merged.

[katre]

LGTM. I can help you merge this once tests pass.

[brandjon]

Gah, six is problematic because

• It is mentioned in Bazel's own WORKSPACE file
• It goes into @bazel_tools as well
• It implicitly is included in protobuf's tree as well, but as a downloadable archive that I can't hand-edit

But from what I can tell, the use of six in Bazel's WORKSPACE is just a bind rule, not a repo rule, so IIUC there's no separate @six repository visible within Bazel's workspace. (I may finally be understanding the use case for //external...) So maybe loads in six are relative to Bazel's workspace?

And if protobuf gets its six from //external via this bind, then it's probably not using its own internal six.BUILD / http_achive rule, so I don't need to worry about editing it.

[brandjon]

Starlark files may not be loaded from the //external package

flips table

[brandjon]

Ok, I have the framework of a solution.

Protobuf, which is used both in-tree as-is, and as a (local) external repo, is hand-modified to reference @rules_python. Bazel's own WORKSPACE file gets a new @rules_python repo entry that is in reality a local stub (#9029 tracks possibly replacing it with the real thing). No changes are made to its own WORKSPACE or six.BUILD files, because these are byproducts from vendoring it in but are not actually used by Bazel (since workspaces aren't recursive). This solution has the advantage that whatever extra maintenance burden there is going forward will eventually disappear once upstream protobuf migrates for the #9006 flag flip.

As for six, which is used in-workspace and in @bazel_tools, it uses the //tools/python:private/defs.bzl label which resolves fine in either case. (I.e., the difference between six and protobuf is that six doesn't get its own @six repo in the Bazel workspace.) Protobuf does depend on six via a bind rule, but I don't think this change to the BUILD file should interfere with that.

Here is a Buildkite run (in progress) of a proof of concept of these third_party/ changes plus my incompatible flag introduction. If this all goes well I'll clean up this PR and look to merge tomorrow (the scheduled day of the release cut).

[brandjon]

@katre this should be good now.

Currently running a CI build of this change plus the unsubmitted CL adding my incompatible flag...

[brandjon]

Well great, I broke bootstrapping (see CI results for this PR). You don't happen to have any ideas John, do you?

[brandjon]

Turns out it helps if, when creating a new third_party/ subdir, I also add it to //third_party:srcs.

To allow //third_party:srcs to see //third_party/rules_python:srcs, I had to rename third_party/rules_python/WORKSPACE to not be a real WORKSPACE file. I wonder why that isn't a problem in the case of protobuf, which also has a WORKSPACE file.

[brandjon]

Detail: src/test/shell/integration/modify_execution_info_test.sh requires a fix to its test setup to add @rules_python to its mock workspace. Since third_party changes must be in a separate PR from non-third_party, it looks like I'll have to break this up into four changes:

1. add rules_python in third_party
2. fix this test to use it
3. make third_party use the bzl files / @rules_python
4. add my incompatible change flag

Setting up one more combined CI run tonight and I'll handle the rest tomorrow.

[laszlocsomor]

@brandjon that's some heroic effort you do here.

But from what I can tell, the use of six in Bazel's WORKSPACE is just a bind rule, not a repo rule, so IIUC there's no separate @six repository visible within Bazel's workspace. (I may finally be understanding the use case for //external...) So maybe loads in six are relative to Bazel's workspace?

This part I don't follow. Could you explain the deal with bind?

And if protobuf gets its six from //external via this bind, then it's probably not using its own internal six.BUILD / http_achive rule, so I don't need to worry about editing it.

Sounds like you could verify this by adding garbage to six.BUILD?

[brandjon]

@brandjon that's some heroic effort you do here.

Thanks, but don't mistake exasperated frequent updates for heroism ;)

This part I don't follow. Could you explain the deal with bind?

Six is used in a bind rule in Bazel's WORKSPACE, but not a repository rule (third_party/py/six has no WORKSPACE file). This contrasts with protobuf, which in addition to being accessed as //third_party/protobuf/... is also accessed as @com_google_protobuf. Within the latter, the protobuf sources can't resolve a Bazel-relative label like //tools/python:private/defs.bzl. But six doesn't have this problem because bind doesn't create a @six repo or anything like that, it just creates a label //external:six for use by other things (protobuf as it turns out). So six can use a bazel-relative label, and indeed it has to because six also appears in @bazel_tools (and the layout of //tools/... in @io_bazel and @bazel_tools coincide).

Sounds like you could verify this by adding garbage to six.BUILD?

Gah, with garbage added I get a test failure in //src/test/shell/bazel:bazel_bootstrap_distfile_test with no clear error message, but the logs suggest that was in fact while building protobuf. Yet it looks like protobuf's WORKSPACE file is the only user. Wonder what happens if I garbage that instead...

[brandjon]

Yep, garbage in the WORKSPACE kills it too. Which makes sense, if the workspace is being evaluated that explains why six.BUILD is. Failing target within the test's call to compile.sh is

src/BUILD:331:2: Executing genrule //src:package-zip_nojdk failed (Exit 1)

[brandjon]

In any case I'll just fix the six.BUILD file to be compatible with the flag just in case we run into some kind of bootstrapping problem when we try to flip the flag.

[brandjon]

Please see PR #9044, the first step of merging this.

[laszlocsomor]

Thanks for the explanation!

[brandjon]

Manual presubmit run with current commits (which need to be rebased but content-wise I believe are ok) is here.

[brandjon]

Status update: Waiting on this PR's auto-initiated CI run, which includes #9048.

#9048 is blocked on this run, because the issue in the test it fixes is only exercised by this PR.

#9048's own CI is proceeding as we speak and should easily pass. So I propose that if both of these PRs CI are green, we rebase and merge without waiting for more runs.

[katre]

I am fine with merging once the combined test run passes.

[laszlocsomor]

This seems to have broken https://buildkite.com/bazel/starlark/builds/83#482da2c7-8415-4381-9645-173a945749c3

[laszlocsomor]

I verified it locally:

1. cloned bazelbuild/starlark@4efed8d
2. bazel build //... failed
3. edited io_bazel in WORKSPACE, replaced with local_repository
4. bisected failure in Bazel source tree

[brandjon]

So IIUC, this breaks projects that import @io_bazel (i.e. that depend directly on bazelbuild/bazel's source tree). This seems acceptable/allowed to me, and not subject to our incompatible change policy. If it breaks the project, they can version the dependency and work around the change on their own timetable, just like any other repo dependency. It's not an issue with the host bazel binary. Or am I misunderstanding?

[brandjon]

Note that any recent (last week or so) version of bazelbuild/rules_python will give you a @rules_python in your workspace that is compatible with bazel's source tree's use of @rules_python, which is only a stub.

[brandjon]

FYI, bazelbuild/starlark doesn't appear to run in the downstream projects CI. Last night's run, which I believe includes this PR, is green (modulo unrelated nodejs failures).

[brandjon]

Sent bazelbuild/starlark#79 to migrate them.