Inconsistent import behaviour between py_binary and par_binary

thundergolfer commented 5 years ago

Description

I encountered some unexpected incompatibility between py_binary and par_binary, that belies this line "par_binary() is a drop-in replacement for py_binary() in your BUILD file".

How to Reproduce

Subpar version:

# Import the rules to build Python subpar packages.
git_repository(
    name = "subpar",
    remote = "https://github.com/google/subpar.git",
    tag = "1.3.0",
)

Python Rules Version:

# TODO: switch back to upstream once https://github.com/bazelbuild/rules_python/pull/82
# and https://github.com/bazelbuild/rules_python/pull/136 are merged.
http_archive(
    name = "io_bazel_rules_python",
    sha256 = "9bd76f014ae4239f0d3ed0c29900a7e3d7307e5fa00506c287dc658cdfe3840a",
    strip_prefix = "rules_python-be39b7906da9c924b09ca78ce3fa152a10f782ae",
    url = "https://github.com/uri-canva/rules_python/archive/be39b7906da9c924b09ca78ce3fa152a10f782ae.tar.gz",
)

Given a WORKSPACE root of /example, create a buggy directory inside that, with the following:

A.py

import B

if __name__ == '__main__':
    b = B.Thing()
    print(b.speak())

B.py

class Thing:
    def speak(self):
        return "Hi I'm B.Thing!"

BUILD.bazel

load("@io_bazel_rules_python//python:python.bzl", "py_binary", "py_test")
load("@subpar//:subpar.bzl", "par_binary")

SRCS = [
    "A.py",
    "B.py",
]

# Simple py_binary
py_binary(
    name = "main",
    srcs = SRCS,
    default_python_version = "PY3",
    main = "A.py",
    srcs_version = "PY3",
    deps = [],
)

# Google subpar self-contained monoarchive executable
par_binary(
    name = "buggy",
    srcs = SRCS,
    default_python_version = "PY3",
#    imports = [""], <-- Uncommenting this changes our fortunes
    main = "A.py",
    srcs_version = "PY3",
    deps = [],
)

Executing bazel run --force_python=PY3 --python_path=$(which python3) //buggy:main gives the following:

INFO: Invocation ID: fa858959-917a-4240-b2ba-bd6e099c928f
INFO: Build options --force_python and --python_path have changed, discarding analysis cache.
INFO: Analysed target //buggy:main (1 packages loaded, 116 targets configured).
INFO: Found 1 target...
Target //buggy:main up-to-date:
  bazel-bin/buggy/main
INFO: Elapsed time: 0.224s, Critical Path: 0.04s
INFO: 0 processes.
INFO: Build completed successfully, 4 total actions
INFO: Build completed successfully, 4 total actions
Hi I'm B.Thing!

But executing bazel run --force_python=PY3 --python_path=$(which python3) //buggy:buggy.par gives this:

INFO: Invocation ID: 2f0582ac-c075-49b7-be27-76ad4e20a455
INFO: Analysed target //buggy:buggy.par (0 packages loaded, 16 targets configured).
INFO: Found 1 target...
Target //buggy:buggy.par up-to-date:
  bazel-bin/buggy/buggy.par
INFO: Elapsed time: 0.387s, Critical Path: 0.28s
INFO: 2 processes: 2 processwrapper-sandbox.
INFO: Build completed successfully, 10 total actions
INFO: Build completed successfully, 10 total actions
Traceback (most recent call last):
  File "/nix/store/d74rl714a32wbv5i5j5qblcl169k1vs4-python3-3.7.2/lib/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/nix/store/d74rl714a32wbv5i5j5qblcl169k1vs4-python3-3.7.2/lib/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/private/var/tmp/_bazel_jbelotti/6d46c1a8d79be3c21fd51c25cfdc932c/execroot/data_eng/bazel-out/darwin-py3-fastbuild/bin/buggy/buggy.par/__main__.py", line 6, in <module>
ModuleNotFoundError: No module named 'B'

The Solution

The solution is to add imports = [""], to the par_binary rule, which is fine, but in my opinion this should be unnecessary as subpar should by default behave the same as py_binary.

Interested to hear your thoughts, and open to submitting a patch for this if you think it appropriate.

thundergolfer commented 5 years ago

I just ran into py_test having this same issue. Adding imports = [""] allowed for correct importing.

aaliddell commented 5 years ago

This may be related to https://github.com/bazelbuild/bazel/issues/7067

Subpar appears to be enforcing the requirement for using fully qualified imports, whereas in py* rules you can currently get away without the prefix. In the future there appears to be an intention to enforce similar stricter rules in the py* rules, but it is not specified quite how that'd look or when.

Setting imports = [""] causes subpar to behave the same as the py* rules do by adding an import path that py* is adding by default.

google / subpar