Future of --experimental_sibling_repository_layout?

[lberki]

We have an experimental flag that costs quite a bit to maintain: --experimental_sibling_repository_layout.

It makes external repositories appear under ../$REPO instead of external/$REPO in the execroo and runfiles trees, which has two benefits:

1. It frees up the name external/. Otherwise, Bazel doesn't support projects that have this directory (which is kind of embarrassing)
2. It puts external repositories and the main repository on equal footing, which is more sound in principle.

As it stands, "it's a forever experimental" flag: we have it implemented, we pay the maintenance price, but we have not committed to migrating to it, which is the worst of all worlds: our users in general don't derive the above benefits (since they presumably don't opt in) and we pay the maintenance price.

I think the best plan is to flip it in Bazel 8, but this is mostly based on feelings ((1) is quite painful). However, it may be a costly migration, so I could be convinced to delete the flag and give up on the whole things.

@Wyverald @meisterT @comius do you have any thoughts?

[tpudlik]

Putting external repositories and the main repository on an equal footing is not necessarily a benefit. For example, in Pigweed we use the fact that external repos are under external/ to silence compiler warnings from external code using --per_file_copt=external/.*@-w in our bazelrc. This is essential if you want your toolchain to apply warnings of any strictness, since without this feature you'll be seeing warnings from your dependencies.

Incidentally, this should ideally be done through toolchain configuration rather than .bazelrc, but there's no support for that at present (bazelbuild/rules_cc#209).

[tpudlik]

The documentation is not very clear. At the very top is says, "--host_per_file_copt=<a comma-separated list of regex expressions with prefix '-' specifying excluded paths followed by an @ and a comma separated list of options>" (emphasis added). Since it explicitly refers to paths, it sounds like a path regex. But later, the example provided is --host_per_file_copt=//foo/.*\.cc,-//foo/bar\.cc@-O0, which looks like a label regex. Actually, if this is a label regex, I'm surprised --per_file_copt=external/.*@-w works at all (the labels don't include the external, do they?).

If --per_file_copt=^@.+//.*@-w did work, that would address my concern! (But is it certain that files in my own repository are never referred to using a label starting with their canonical repository name?)

[Wyverald]

I've only ever seen it used with execroot paths. Don't think labels would work.

[Wyverald]

Ah, looking at the code, it seems that the regex is used against both the label and the artifact path. The problem is (IIUC) that some artifacts might not have labels at all (like certain generated sources?)

But obviously the other problem is the @ being used as the separator between regex and options.

[Wyverald]

But is it certain that files in my own repository are never referred to using a label starting with their canonical repository name?

This is true as of today -- some file in your own repo would appear as //pkg:file.cc, with no starting @s at all. This is arguably a quirk of the Label.toString() behavior.

[illicitonion]

Reference to issue about this: #19665

[tjgq]

I agree that having two code paths is very unfortunate from a testing surface standpoint. I share the sentiment around the sibling layout being cleaner, but pragmatically speaking, if we can't find the time/energy to do the migration soon™, we should just cut our losses and accept the ugly/embarrassing API.

On that note, it would help to know how many people are actively using the sibling layout and haven't found any bugs in Bazel around it. (Otherwise, this might not be "just" a matter of flipping the flag and fixing downstream projects.)

[jin]

+1. This is a nearly decade-old design decision that's caused delays in Bazel adoptions (most notably in Android, when we had to implement this). The workaround is ugly (Bazel doesn't work in your source tree, so change your source tree), so IMO there's value in getting it over the hump, leveraging on major version changes.

[fmeum]

@tjgq #20500 (comment) is the one example of a bug in the sibling layout I am aware of.

[Wyverald]

It makes external repositories appear under ../$REPO instead of external/$REPO in the execroo and runfiles trees, which has two benefits:

Does this actually affect the runfiles tree? I thought runfiles trees were already organized as $runfilesRoot/$repoName without any nesting under $runfilesRoot/_main/external/$repoName.

do you have any thoughts?

I'm mostly worried about @tpudlik's comment: IMO we definitely need a solution to the --per_file_copts problem before we can even think about flipping this.

[lberki]

When the flag flipped, the paths of files in external repositories starts with ../ instead of external/ so the regex could be changed accordingly (or even to accommodate both alternatives).

So it looks like --per_file_copt isn't the thing this decision should hinge upon.

[fmeum]

I would also welcome the flag flip. Another benefit that hasn't been mentioned yet is that it aligns the directory structure of input files with that of runfiles, which would have helped me a lot when I initially got into rules authoring.

That said, if we want to flip this in Bazel 8, I think that we need to resolve the following issues soon (i.e., in time for Bazel 7.1 so that ruleset authors have ample time to prepare the migration):

1. https://groups.google.com/g/bazel-discuss/c/eyjCTJl3OHA: Even with --experimental_sibling_repository_layout, bazel-out for an external repo is still rooted under the main repo (e.g. __main__/bazel-out/other_repo/k8-fastbuild/bin/pkg/gen.txt). This voids most of the gains in conceptual simplicity and can also break builds if other_repo happens to be called k8-fastbuild.
2. Feature Request: local_includes for cc_library, making an easy path for good cross-platform hygiene #16472: Without a local_includes attribute on cc_library, users currently tend to hardcode external/... in BUILD files. There are unofficial workarounds (see bzlmod external repo name and copts #15553), so an alternative could be to standardize on one and make it available in rules_cc. This would be less discoverable though.

With these issues addressed and with buy-in from the Rules Authors SIG to address ruleset incompatibilities, this migration should be doable.

[lberki]

re (1): it was an explicit decision to put the bazel-out of external repositories under the cwd. If I recall correctly, one of the arguments for it was that it turned out that a single "output directory" was much easier to handle in the code than one for each external repository and it's also convenient that all output files are accessible under a single convenience symlink. It does open us up to name clashes like the way you mention and I'm still of the opinion that if we want to flip this flag, we might revisit this decision.

re (2), I think it's indicative of the long-tail issues we'd be facing if we committed to this flag flip.

[lberki]

I sent out #21785 to disable the flag :(

[lberki]

Update: as a last-ditch effort to rescue this flag flip, I checked the downstream project pipeline and it looked like there was only a very manageable number of breakages. In light of this, @comius @meteorcloudy and @meisterT kindly agreed to push the migration forward, so instead of removing this flag, we are going to flip it. Yay!

[fmeum]

Sounds great! Happy to help with the migration as well. What's the plan for dealing with #20500 (reply in thread) though?

[lberki]

I'm afraid the plan I have in my mind is "deal with it later". It would require a quite serious reshuffling of both the output directory layout and the code that creates it and I'd much rather do it separately, even if that means two changes to the output directory structure instead of one.

I have another theoretically annoying issue, which is that with sibling repository layout, the cwd really should not be execroot/<main repo>, but simply execroot so that all repositories are equal, but that's another big change and I have no idea if its benefits would outweigh its costs or not.

[fmeum]

I'm worried about the cost of doing all these migrations separately, as well as the risk of getting stuck somewhere in the middle. For example, some runfiles libraries would need to support every single format (they derive the canonical repo name of a file from its execpath) and this can get out of hand if we keep changing it. Happy to look into more compatible alternatives, but that again would require a better understanding of future migrations.

[lberki]

A few months later, it seems that we are switching back to "this flag should be condemned":

#23576 (comment)

Please speak up if you think that's a bad idea -- given that we haven't made progress in the legwork to clean up the flag, that it has a large potential for harm, that no one benefits from it too much and that it's arguably the wrong layout anyway, it's hard to imagine an argument for it...

[AlexK-BD]

--legacy_external_runfiles=false produces the sibling layout for tests and such -- wouldn't it be good if the layout was consistent for execroot and runfiles?

[meteorcloudy]

See #23576 (comment)

And the fact flipping --legacy_external_runfiles to false didn't cause much breakages indicates most users are already depending on the new layout.

[AlexK-BD]

I'm working on adopting --nolegacy_external_runfiles at the moment! One example consistency problem I've run into in the process:

• We have a license_files_list rule that uses an aspect to collect the file.path for license files in its transitive dependencies. file.path produces paths with external/whatever for files that are in other workspaces. This produces a generated file, license_list.txt for example.
• There are things that want to read license_list.txt at runtime and then access the licenses listed therein. But, the licenses are no longer found under external, they're in the "sibling" layout.

Is there a better thing to use than file.path to get the location that a file will have in a runfiles directory?