Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make hermetic interpreters compatible to disallow_empty_glob #761

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Make hermetic interpreters compatible to disallow_empty_glob #761

merged 1 commit into from
Jul 26, 2022

Conversation

martis42
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Using Bazel CLI option incompatible_disallow_empty_glob is not compatible with using a hermetic interpreter offered by rules_python.

It causes the error (exemplary for a Linux host):

ERROR: Traceback (most recent call last):
	File "<path_to_bazel_out>/external/python3_10_x86_64-unknown-linux-gnu/BUILD.bazel", line 9, column 16, in <toplevel>
		srcs = glob(
Error in glob: glob pattern '*.exe' 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).

Issue Number: N/A

What is the underlying problem

A single filegroup is used to aggregate the interpreter files. Its platform specific regex patterns are not compatible to the flag --incompatible_disallow_empty_glob which forbids any pattern to return an empty list. For example, on a Linux host the "*.exe", pattern will be empty.

Reproducing

Minimal workspace for reproducing the problem.

WORKSPACE

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
    name = "rules_python",
    sha256 = "a3a6e99f497be089f81ec082882e40246bfd435f52f4e82f37e89449b04573f6",
    strip_prefix = "rules_python-0.10.2",
    url = "https://github.com/bazelbuild/rules_python/archive/refs/tags/0.10.2.tar.gz",
)

load("@rules_python//python:repositories.bzl", "python_register_toolchains")
python_register_toolchains(
    name = "python3_10",
    python_version = "3.10",
)

BUILD

load("@rules_python//python:defs.bzl", "py_binary")

py_binary(
    name = "foo",
    srcs = ["foo.py"],
)

foo.py

import sys
print(sys.version)

Command to generate the error:
bazel run --incompatible_disallow_empty_glob //:foo

What is the new behavior?

One can utilize flag incompatible_disallow_empty_glob while using a hermetic Python interpreter offered by rules_python.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

What alternatives have been considered.

We could also introduce separate filegroups for the different platforms and use an alias with a platforms based select statement to provide the correct one to the user.
However, given how unlikely it is that an interpreter without files is referenced by rules_python and this is not discovered in any test, we should use the straight forward solution offered in this PR instead of introducing this additional complexity.

Copy link
Collaborator

@thundergolfer thundergolfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with this simple approach. Seems there's not really a clean way to partition the patterns into required and maybe like so:

filegroup(
    name = "files",
    srcs = glob(
        include = [
            "bin/**",
            ...
        ],
        exclude = [
            "**/* *", # Bazel does not support spaces in file names.
        ],
        allow_empty = False,
    ) + glob(
        # Platform-specific files.
        include = [
            ...
        ],
        allow_empty = True,
    ),
)

A single filegroup is used to aggregate the interpreter files.
Its platform specific regex patterns are not compatible to the flag
`--incompatible_disallow_empty_glob` which forbids any pattern to return
an empty list. For example, on a Linux host the `"*.exe",` will be empty.
@martis42
Copy link
Contributor Author

@thundergolfer
I incorporated the comment suggested by you and rebased the PR. Anything else required from my side for this to merge?

@mattem mattem merged commit 4b3c2b3 into bazelbuild:main Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants