Add support for hermetic Python

[quval]

Allow python_configure to use a Python interpreter that is provided by a Bazel target, similarly to other Python rules (e.g. pip_install).

[SomeoneSerge]

This doesn't bridge it with toolchains though, does it?

[quval]

This doesn't bridge it with toolchains though, does it?

That's right, it doesn't - but the rules in rules_python don't do it either. I don't know if that sort of thing would be possible at all, given that toolchain resolution happens at a later stage (and depends on the target).

[SomeoneSerge]

Ended up with this in WORKSPACE and confirm it works:

nixpkgs_package(
        name = "python3",
        repositories = {"nixpkgs": "@nixpkgs//:default.nix"}
)
python_configure(
        name = "local_config_python",
        python_interpreter_target = "@python3//:bin/python3",
)

I don't think the "toolchains support" issue should be closed though. I don't see why supporting toolchains wouldn't be possible, but it does seem like supporting them would take writing pybind rules almost from scratch.

@TheButlah anything preventing the merge?

[TheButlah]

@TheButlah anything preventing the merge?

I'm not a maintainer of the repo

[TheButlah]

This doesn't bridge it with toolchains though, does it?

That's right, it doesn't - but the rules in rules_python don't do it either. I don't know if that sort of thing would be possible at all, given that toolchain resolution happens at a later stage (and depends on the target).

I thought that rules_python do support them though - isn't that what py_runtime_pair does?

[quval]

The pip_install rule supports either a path or a target for the Python interpreter - it doesn't support a py_runtime_pair. Or did I misunderstand you?

[TheButlah]

Oh I think I understand what you are saying - for a toolchain, you register the bazel target for the python interpreter as a toolchain and then all python rules should be able to use it automatically without specifying it as a dependency. But in this merge request, instead of relying on the registered toolchain, the user still has to explicitly provide the python interpreter target to the rule. Therefore, even though its hermetic, it still doesn't truly interoperate with the toolchain system.

Did I understand that correctly @quval?

[quval]

That's pretty much my understanding as well. And since toolchain resolution depends on the target, making configure_python actually integrated with toolchains is a major overhaul of the whole rule set (since we may need multiple configurations for multiple toolchains). I suppose it might be possible to get the Python interpreter from a toolchain rule, but I don't know how hard it would be to support multiple configurations here, e.g. for both Python 2 and Python 3 with a runtime pair. This change does seem to me to add the same sort of support for hermetic Python as rules_python provides, and it makes sense to assume that if a workspace uses hermetic Python then all Python targets will use one and the same configuration, but this actually isn't full support of toolchains -- I may have been a wee bit rash in marking this as resolving the issue.

[TheButlah]

So for whoever is inevitably going to look to review this, its mergeable right? It just adds hermetic python support, rather than toolchain support

[jordanzliu]

I'm getting the following error from Bazel when I try to build a pybind11 library with the --spawn_strategy=standalone Bazel flag:

this rule is missing dependency declarations for the following files included by 'software/ai/passing/cost_function_python.cpp':
  'bazel-out/k8-fastbuild/bin/external/local_config_python/python_include/pyconfig.h'

WIth the sandbox enabled, the compiler is unable to find the header.

If I add @local_config_python//:python_headers to the deps for my library, Bazel gives the following error:

Label '@local_config_python//:python_headers' is duplicated in the 'deps' attribute of rule 'cost_function_python_lib'

[quval]

@jordanzliu I could find a handful of results referencing what looks like the same problem you're having, so just to make sure: are you seeing this error only with this patch? What version of Python are you using?

[jordanzliu]

I'm on Python 3.7.9. I did actually end up finding out that pyconfig.h was placed in the root directory of the Python source tree instead of in the directory at sysconfig.get_python_inc(), and I made this patch to fix it.

[quval]

Hmm. I haven't had to do this with Python 3.8 or Python 3.9.

[jordanzliu]

In any case, this patch shouldn't fail if pyconfig.h is in the actual include directory, though i haven't tested with newer python versions.

[dmadisetti]

_get_python_lib needs to change

def _get_python_lib(repository_ctx, python_bin, python_lib, local):
    """Gets the python lib path."""
    if not (local or python_lib):
        python_lib = repository_ctx.os.environ.get(_PYTHON_LIB_PATH)
    if python_lib:
        return python_lib
    # .. default to running python to find lib

[dmadisetti]

would suggest library target for completeness

[quval]

Do you mean to substitute the check at _get_python_lib? What would this target look like? As far as I understand, we should be getting the correct value by querying the interpreter.

[dmadisetti]

Ahh nice, I was just about to start a PR.

As is, this looks like it will still use the python system libraries- left some suggestions

[quval]

@dmadisetti Thanks for reviewing! What system libraries are you thinking will be used? I've successfully used this for executing actions on an image that has no Python installation.

[dmadisetti]

@quval No problem! I was dealing with use-case of standardizing python across my platforms (vs. using completely vanilla containers). If my base image already has PYTHON_LIB_PATH set, then this will break. Example: building a 3.8 project on a ubuntu18 image (that defaults to 3.6)

• We need to specify a hook for the library target so we can pass in the expected library install location e.g. @python//Include (the way my install is set up).
• And/or use the fallback python for determining library_path in case the interpreter target is passed in (if the env is set, it'll default to that which is not ideal)

I'm happy to make / test these patches, but I don't know who we need to ping to get this merged in

[quval]

@dmadisetti Ah, if that environment variable is set, things will indeed break. I was assuming it wouldn't be there when using hermetic Python. Specifying a target for the include path could be very nice if it can replace the genrule.

I have no idea who is maintaining this repository, but I'll be happy to merge your patch into this PR, in hope that helps people. (I'll also add in @jordanzliu's patch from above when I get around to do it.)

[dmadisetti]

Locally tested and pr'ed quval#1

[dstripelis]

Can you please merge this PR to the master branch? We need it for our hermetic python env. Thank you.

[quval]

@Raschild This PR is basically the patch we used on top of pybind11_bazel - perhaps that can help you until somebody can get around to reviewing or merging this.

[QuantamHD]

Would love to get this merged as this unblocks using a custom sysroot C++ toolchain