incompatible_disallow_empty_glob: fail if a glob doesn't match anything

[laurentlb]

The glob() function tends to be error-prone, because any typo in a path will silently return an empty list. This has been a source of confusion and bugs for many users.

glob has an optional Boolean argument allow_empty. For example, let's say your code looks like:

glob([pattern_a, pattern_b], exclude = [pattern_c], allow_empty = False)

This code will fail if pattern_a or pattern_b doesn't match anything. It also fails if the whole function (after excluding pattern_c) doesn't match anything. We believe this behavior is safer in general.

In rare cases, this is intentional that a pattern doesn't match anything, e.g. when a source file is optional. When it happens, users should tell their intent explicitly by setting allow_empty = True.

• What's going to change? The default value of allow_empty used to be True. It will change to False.

• How to update your code? If a glob fails, use allow_empty = True to keep the old behavior.

In Bazel 8.0, the flag --incompatible_disallow_empty_glob will default to true. This means that the allow_empty argument defaults to False on all glob calls.

[alandonovan]

Status: still awaiting cleanup and flag flip.

[martis42]

This is labeled with breaking-change-5.0, which I interpret as the intention to flip this with Bazel 5.0.0. The flag is however still inactive by default: https://github.com/bazelbuild/bazel/blob/5.0.0/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java#L355

Please update the issue status.

[meteorcloudy]

According to https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1253

This flag is breaking almost all downstream projects, I believe it's the same for the wider community. If no active plan on migrating the ecosystem, I'll remove "migration-ready" label for now.

[martis42]

@meteorcloudy I believe migrating this flag is a chicken and egg problem. Enabling this does not really solve a problem for users. It merely establishes a sane default for globbing. Imho donwstream projects will most likely not adapt until there is an incentive.

What I am going for is, imho we should flip this flag soon (maybe Bazel 6.0?). This might be the gentle push for downstream projects to adapt to the flag. If they don't want to, setting the old behavior via the incompatibility flag is easy enough.

[meteorcloudy]

What I am going for is, imho we should flip this flag soon (maybe Bazel 6.0?). This might be the gentle push for downstream projects to adapt to the flag. If they don't want to, setting the old behavior via the incompatibility flag is easy enough.

No, this doesn't really work. You can see in the link I posted, flipping this flag will break almost all the downstream projects. Many of them are common dependencies in the ecosystem instead of just leaf projects. If your dependency doesn't build with this flag, you will be forced to unflip the flag in your bazelrc file even if you have migrated your own code.

So the correct way is to file issues to downstream projects, make sure common dependencies in the ecosystem are migrated first, then flip the flag.

See our updated incompatible change process: https://bazel.build/contribute/breaking-changes?hl=en#update-repos

[limdor]

@bazel-io flag

[limdor]

I would request considering this for Bazel 6.0. Even though it is considered a P4, there are people in the community that would like to see it flipped (see for example https://github.com/bazel-contrib/SIG-rules-authors/issues/37) and most of the work is done (CI is green for the flip in https://github.com/bazelbuild/bazel/pull/15327)

Regarding the downstream projects, I did not see any failure on the latest builds: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1279 https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1280 https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1281 https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1282

[meteorcloudy]

@limdor The flag wasn't tested in the pipeline because the "migration-ready" was removed. I added it again let's see https://github.com/bazelbuild/bazel/pull/15374 will fix the failures.

[meteorcloudy]

From https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1253#01833aea-8848-4562-976c-916e87f33b6b, I can see at least TensorFlow is broken by this:

(08:59:35) ERROR: Traceback (most recent call last):
    File "C:/b/bk-windows-gclq/bazel-downstream-projects/tensorflow/tensorflow/tools/docs/BUILD", line 154, column 16, in <toplevel>
        data = glob(["**/create_model.md"]),
Error in glob: glob pattern '**/create_model.md' didn't match anything, but allow_empty is set to False (the default value of allow_empty can be set with --incompatible_disallow_empty_glob).

Also protobuf: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1253#01833aea-77d8-4da4-ae2d-7e06130a3fc3

I think many downstream failures are actually not caused by the android rules, so I don't think we can migrate the downstream projects in time for 6.0

[limdor]

@limdor The flag wasn't tested in the pipeline because the "migration-ready" was removed. I added it again let's see #15374 will fix the failures.

Ok understood, I will take a look to the failures then. In general the fixes are quite trivial, at least to get the default behavior. The problem is getting reviews in timely manner. The problem that I might be facing in downstream projects is that setting the allow_empty = True might not be well received and some projects might want to investigate why that is the case. However my rationale here is that first it should be set allow_empty to True and then investigate, with this way we can flip the default behavior and stop introducing cases where the glob is empty but the developer did not expect that.

I understand that the downstream projects are important but at some point there is the whole Bazel comunity that is suffering from not flipping this flag.

My proposal would be to try to get the PRs for the downstream projects where it sets allow_empty to true for the affected globs, and in the hypothetical case where they don't want to merge it, consider flipping it anyway. But somehow I'm sure we should be able to convince the maintainers of the downstream projects that setting the default explicitly is better than doing nothing.

[meteorcloudy]

My proposal would be to try to get the PRs for the downstream projects where it sets allow_empty to true for the affected globs, and in the hypothetical case where they don't want to merge it, consider flipping it anyway.

Agree, however, there is no clear owner of this flag from the Bazel team right now, so it would be great if the community can help send those PRs. It will be useful to have a tracking list like https://github.com/bazelbuild/continuous-integration/issues/1404#issue-1341836598

[limdor]

My proposal would be to try to get the PRs for the downstream projects where it sets allow_empty to true for the affected globs, and in the hypothetical case where they don't want to merge it, consider flipping it anyway.

Agree, however, there is no clear owner of this flag from the Bazel team right now, so it would be great if the community can help send those PRs. It will be useful to have a tracking list like bazelbuild/continuous-integration#1404 (comment)

For the moment the tracking list is here https://github.com/bazelbuild/bazel/pull/15327#issue-1213646746

[meteorcloudy]

@limdor The list seems to track the progress of fixing Bazel itself? There are many downstream projects also broken, which should also be migrated first before flipping the flag: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1253

[limdor]

@limdor The list seems to track the progress of fixing Bazel itself? There are many downstream projects also broken, which should also be migrated first before flipping the flag: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1253

Initially was, but I'm planning to extend it. Any chance to leave the migration ready flag? If not it's quite hard to track and I have to ask every time.

[meteorcloudy]

@limdor Sure, as long as someone is actively working on this, I'm happy to enable the test!

[limdor]

@limdor Sure, as long as someone is actively working on this, I'm happy to enable the test!

Thanks! As active as can be doing it in my free time.

[meteorcloudy]

@limdor Thank you so much!

[limdor]

Update: All PRs in Bazel have been merged except the flag flip itself. The remaining part is the downstream projects that are tracked here https://github.com/bazelbuild/bazel/pull/15327

[restingbull]

Worth noting this will cause a lot of churn in macros, especially in private repositories, that will not be exposed in downstream rule sets.

[keertk]

Hi all, wanted to check if we plan to flip this flag in 7.0? Let me know when this is ready for migration so we can check for downstream fixes needed. Thanks.

[github-actions[bot]]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

[brentleyjones]

allow_empty = False causing errors when any pattern is empty can result in unsafe globs by forcing people to use allow_empty =True. Take the following glob as an example:

glob([
   allow_empty = False,
    "*.strings",
    "*.stringsdict",
])

I want an error if we don't have any .strings or .stringsdict files. But I'm fine with having either. Currently this will error out if I don't have both. So I need to set allow_empty = True, but then I won't get errors when its empty.

[fmeum]

@brentleyjones Would this work for you?

glob([
   allow_empty = True,
    "*.strings",
    "*.stringsdict",
]) or fail("glob is empty")

[brentleyjones]

Yes. Thank you.