Fix incompatibility with Python Toolchains

[aaliddell]

Fixes #98 by detecting the default toolchain wrappers and replacing with env versions. I'm perhaps not totally happy with how it's done, as any change in the path to these wrappers would break things again, so open to suggestions. Also, for Python 2 should we use /usr/bin/python or /usr/bin/python2, I prefer the latter but don't know how this behaves on all systems?

Also, now that we have PyInfo provider, the import paths are passed directly from Starlark to the compiler, removing a bit of fragility in parsing the stub file for those. However, I can't see a way to pass the interpreter directly, as this doesn't appear to be available to Starlark. This removed a TODO with reference b/29227737, if that means anything internally?

Tested with both the default toolchain and a custom one to ensure the correct shebang ends up at the top of the par file. It might be worth adding a test that excercises this, but until the toolchains flag is flipped in CI the test will always fail.

[aaliddell]

Regarding the python vs python2 for Python 2 question: the concern was that the latter may not exist everywhere, but PEP-394 specifies that it must exist and is a well supported convention to depend upon.

[tmc]

This diff could be pared down a bit to just the relevant parts.

[brandjon]

I'm perhaps not totally happy with how it's done, as any change in the path to these wrappers would break things again, so open to suggestions.

I wouldn't worry about it. This is a bespoke hack to make subpar work with a particular toolchain, so I think it's ok to assume some implementation details about that toolchain.

Also, for Python 2 should we use /usr/bin/python or /usr/bin/python2, I prefer the latter but don't know how this behaves on all systems?

python2 please. I'd rather break users who fail to specify an appropriate toolchain for their platform, than choose a default that ends up invoking the wrong version interpreter.

Also, now that we have PyInfo provider, the import paths are passed directly from Starlark to the compiler, removing a bit of fragility in parsing the stub file for those.

Cool!

However, I can't see a way to pass the interpreter directly, as this doesn't appear to be available to Starlark.

bazelbuild/bazel#7805

This removed a TODO with reference b/29227737, if that means anything internally?

It was an obsolete bug asking for imports to be accessible from PyInfo, which it has been for some time now.

Tested with both the default toolchain and a custom one to ensure the correct shebang ends up at the top of the par file. It might be worth adding a test that excercises this, but until the toolchains flag is flipped in CI the test will always fail.

I won't block the PR on this, but for reference the buildkite pipeline is defined here, and it looks like it can take custom flags.

[aaliddell]

python2 please. I'd rather break users who fail to specify an appropriate toolchain for their platform, than choose a default that ends up invoking the wrong version interpreter.

Alright, that's the way it is now.

bazelbuild/bazel#7805

We can have a separate issue to track the desire to change the way the interpreter is found, which can resolve if PyRuntimeInfo becomes available.

I won't block the PR on this, but for reference the buildkite pipeline is defined here, and it looks like it can take custom flags.

Ok, I'll take a look

[tmc]

The postsubmit is probably a better place but I changed the travisci config to run with the toolchains flag here: f0c9ec7
some of the tests are going to fail as they explicitly check which files end up in the archives (

subpar/tests/test_harness.sh

Line 42 in 07ff5fe

if [ "$PAR" -eq 1 ]; then

). Personally I'm a fan of getting this in and expanding test flags as a follow-on.

I had started a minimal diff here with the flag in each state but I'll close in favor of this diff: #100

[aaliddell]

Thanks, changes made 👍

[brandjon]

LGTM. Will try to merge tomorrow and see what else breaks due to the toolchains flag. Thanks for the PR!