-
Notifications
You must be signed in to change notification settings - Fork 534
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
PIP dependency support #1
Conversation
This is broken off of my prototype repository, and there are a handful of TODOs left to resolve.
Elaborate on why the TODO to rewrite {pip,whl}.sh as Python is harder than it looks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No big remarks, except adding the bazel ci configuration.
@@ -0,0 +1,39 @@ | |||
# Compiled Object files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have all those parts? emacs exclude belongs to the ~/.gitignore, all the bin files should not be generated by bazel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just my standard .gitignore
file I copy from repo to repo. I never touch it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repo .gitignore are supposed to be repo specific, so not includes things like *~...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I advice I got, didn't like, but eventually learned to appreciate the wisdom of, is to create a ~/.gitignore_global with all the editor autosave filenames, etc (things that are particular to me, not to the repo I'm working on), and do git config --global core.excludesfile ~/.gitignore_global
Otherwise, you're like "What part of this Python program creates Fortran executables?!"
@@ -0,0 +1,27 @@ | |||
sudo: required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add .ci/rules_python.json for CI. see https://github.com/bazelbuild/continuous-integration/blob/master/docs/owner.md#customizing-a-project
Seeing the build, you probably just want:
[
{
"variation": "",
"configurations": [
{"node": "linux-x86_64"},
{"node": "ubuntu_16.04-x86_64"},
{"node": "darwin-x86_64"}
]
}
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
which can be used directly in `deps=[]` to reference an imported `py_library`. | ||
|
||
```python | ||
load("@my_deps//:requirements.txt", "packages") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably meant requirements.bzl
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not updated? Maybe you just want to delete that part all in profit of the skydoc generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, I think this bug was in two places. thanks for catching it again :)
python/pip.sh
Outdated
@@ -0,0 +1,84 @@ | |||
#!/bin/bash -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment for this file: the file already complex, why not in python instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See line 17. I was halfway through rewriting one of these scripts in Python before I realized: I can't invoke a py_binary
during WORKSPACE
, I'd have to publish a PAR file. I immediately git stash
ed and rewrote my TODO instead. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:( I actually do things without py_binary for simple python_script (I know that is a shame) using simply python <script.py>. Anyway up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that is true. I want to be able to factor libraries and have a clean separation of unit tests etc. I'll experiment with my stashed changes with this in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it's ok to use shell here, not Python. But you actually need Python inside the script to parse json, so yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For historical reasons that are still sometimes relevant, don't put the -e in the shebang line, instead add set -euo pipefail
around line 16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only remaining shell script now does this (I copied it from you :D)
python/whl.sh
Outdated
@@ -0,0 +1,82 @@ | |||
#!/bin/bash -e | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, bash can become quickly hard to read...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
@duggelz Do you have any suggestions on the best way to download and invoke |
@sebgoa You'd asked for this at one point, if you are still interested I'd love your feedback. |
README.md
Outdated
See Bazel core [documentation](https://docs.bazel.build/versions/master/be/python.html#py_test). | ||
|
||
<a name="pip_import"></a> | ||
## pip_import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a thing called skydoc that will turn structured docstrings into Bazel documentation. However, it's kind of basic and a bit, well, not beautiful. https://google.github.io/subpar/subpar.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll TAL at skydoc for this repo, but in rules_docker
and rules_k8s
I'm blocked on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've switched over to skydoc for the docs here (based on how google/subpar does it). Feedback welcome :)
76a4bcb
to
91f1f9b
Compare
I moved I have not moved
What if we build / publish a I still haven't found a clean process for doing this from a single repo ( |
e2267eb
to
c1dbfa0
Compare
This migrates the logic from pip.sh into piptool.py, which should improve portability by removing the bash dependency. This also has the beginnings of wrapping piptool as a closed redistributable that doesn't rely on a system-installed copy of PIP, but instead uses these rules to pull pip into a PAR bundle. Besides needing to work out the details of releasing and redistributing the PAR, we have two unresolved issues: * When bundled as a PAR (vs. py_binary), piptool seems to pick up the system-installed version of pip. * When bundled as a PAR, piptool sometimes sees cert issues resolving requirements (similar to what we see with httplib2).
examples/version/version_test.py
Outdated
|
||
def test_version(self): | ||
parts = pip.__version__.split('.') | ||
self.assertEqual(3, len(parts)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertEqual(pip.__version__, '9.0.1')
? You might be overthinking this :) There's no standard format for __version__
, as you discovered, sometimes it's a tuple, sometimes a string, sometimes it doesn't exist.
python/piptool.py
Outdated
# being invoked as a raw .py file. Once bundled, we should be able | ||
# to remove this fallback on a stub implementation of Wheel. | ||
try: | ||
from python.whl import Wheel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like you're really playing with fire calling your package python
.
move python tools under a top-level rules_python package simplify version_test
@duggelz I think that's a very good question, and I think it breaks down where you have:
As you are clearly driving at, we have 3 sets of requirements that are being merged after the constraints have been solved, which I think is a losing proposition. I think that my expectation is that instead of This story is only half-baked because:
I think the crux of this issue is that
The analogs to this outside of Bazel are things like I think the guidance should be for users not to mix independently resolved external dependencies, but I'm not 100% sure if/how we can enforce it. Perhaps we can do this through Bazel's constraint system, but I have not looked any further at how we might express this. Is what I'm describing clear? Does my logic make sense? |
@mattmoor I agree with what you wrote. Some additional points:
I don't think we need to make a tool that tries to merge A and B automatically or anything. I think it's reasonable to say that C must provide a After reading some discussions about Java/Maven, I do think that we should put the names in a canonical format that we document and agree on. Specifically, I think we should have the literal prefix |
`whl_library` rules generated by `pip_import` are now named as: ``` pypi__{distribution}_{version} ``` Substituting illegal characters (e.g. `-`, `.`) with underscores.
@duggelz Given that things are hidden behind I've gone with |
af2c63f
to
182554f
Compare
Add a script for updating the tools and document this and the docs script.
@duggelz Hooray, green CI :) |
Update whl.bzl
This looks like it's left over from before the switch to buildkite. The Travis tests are not run in precommit now, and I can't even tell if they were run in PR bazelbuild#1. This causes pre-commit errors in forks if folks have Travis enabled in other projects.
This looks like it's left over from before the switch to buildkite. The Travis tests are not run in precommit now, and I can't even tell if they were run in PR #1. This causes pre-commit errors in forks if folks have Travis enabled in other projects.
Add extra_index_urls parameter to pip_parse
This is a proposal for some PIP rules that are heavily based on a prototype I wrote a month or two back. Very open to suggestions here, but it seems to work across a number of examples I've tried so far.