Can one of the admins verify this patch?

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
• In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

Two concerns that I still have with this change:

1. This definitely has the potential to break existing builds. Not sure what the rules are around breaking changes.
2. This relies on pkg_resources.evaluate_marker() which comes from setuptools. I assume it's only available from some setuptools version onwards. I wonder if it's a good idea to put a lower boundary setuptools version in python/requirements.txt. Update: evaluate_marker() has been part of the API since setuptools version 1.0, so not a real concern.

CLAs look good, thanks!

@mattmoor rebased

Test this please

(For Bazel CI)

About the CI failure (https://ci.bazel.io/blue/organizations/jenkins/PR%2Frules_python/detail/rules_python/67/pipeline): It seems the linux-x86_64 environment on Jenkins has a somewhat old setuptools version which does not yet support some common syntax in version markers. It's failing on '<='. This appears to be supported from setuptools 17.1 onwards.

I added another commit which pins the version of setuptools in piptool to the latest (38.2.4). I think this is a good idea in any case as it should make builds more predictable in general. It does mean that the piptool.par executable is now 1.8M instead of 1.3M as setuptools is embedded in it.

Test this please

Ugh, this is painful. piptool does use the provided setuptools now, but the whl_library rules generated by piptools do not. Unclear to me if it's possible to pull in the correct setuptools in the whl_library rules. I'll need more time to investigate, any ideas appreciated in the meantime.

@nikhaldi This is because we are actually using two different scripts to do this, piptool.par is one, which we bundle as a PAR to avoid needing the host to have pip installed, and whl.py, which we invoke directly.

So why are these (checked in .par or .py) our only options?

During BUILD steps, you can use py_binary targets as "tools", but you cannot do this during WORKSPACE. So whatever you invoke must be a standalone executable.

So how do we get past this?

Given a dependency on a particular setuptools, I think the option that makes the most sense would be to do with whl.py what we do for piptool.par and have update_tools.sh check a PAR file into the repo (yuck, but oh well).

LMK if that [doesn't] make sense, and I'm happy to elaborate.

@mattmoor thanks for the explanation. It makes somewhat sense, though it feels like there is a lot of duct tape holding this thing together.

I can see how to do a separate whl.par. An alternative might be to vendor (copy) the packaging module. That's where the environment marker parsing is actually implemented. I only realized recently that setuptools is doing the same thing, it's just vendoring packaging. The existing uses in piptool of pkg_resources.Requirement could probably be replaced with packaging as well which would remove setuptools from piptool.par. Thoughts?

I won't have time to look at this further until late December/early January but I can spend some more time on it then. I'm not unhappy either if somebody else takes over in the meantime.

@nikhaldi You cannot vendor the packaging module because whl.py has to be a self-contained script to work properly. This is what I meant when I said it couldn't be a py_binary.

FWIW, I agree with your characterization (re: duct tape), but I think it's largely working around some limitations of Bazel that aren't easily fixed (though I've been trying!). For things that aren't, I'm certainly open to discussing the trade-offs and making improvements.

The existing uses in piptool of pkg_resources.Requirement could probably be replaced with packaging as well which would remove setuptools from piptool.par. Thoughts?

Defer to @duggelz wisdom, but superficially this SGTM.

I won't have time to look at this further until late December/early January but I can spend some more time on it then. I'm not unhappy either if somebody else takes over in the meantime.

No worries, everything slows down this time of year :)

I really appreciate you taking the time to send us this PR.

Test this please

This appears to be working now with a separate whltool.par. Also refactored the tests to not use ugly branching based on Python versions.

I opted to leave the setuptools package in over packaging because piptool uses some pkg_resources features which I'm not sure can easily be rewritten with packaging. This could be a separate refactoring if the size of the par files is a real concern.

Test this please