Switch to new http repo rules - #135
Conversation
This upgrades our dependencies to newer versions that are compatible with the new repo rules, and handles the fallout. Also add some comments to WORKSPACE. rules_python now builds under Bazel 0.19.1 with `--incompatible_remove_native_git_repository` and `incompatible_remove_native_http_archive`. Fixes #105.
|
Rupert, sending this to you "by default" since you did a couple trivial ones earlier (thanks!), but feel free to send it back for me to find someone else. |
|
|
||
| git_repository( | ||
| name = "io_bazel_rules_sass", | ||
| commit = "8b61ad6953fde55031658e1731c335220f881369", # Taken from skydoc's WORKSPACE |
There was a problem hiding this comment.
Change the comment, let us know what this commit means. Is it a specific release? A tag? Opaque hashes are the worst thing to sync on because it's impossible to tell when it's out of date.
There was a problem hiding this comment.
Well skydoc didn't explain why they use that version. Updated the comment to add a date.
| url = ("https://pypi.python.org/packages/c6/28/" + | ||
| "67651b4eabe616b27472c5518f9b2aa3f63beab8f62100b26f05ac428639/" + | ||
| "grpcio-1.6.0-cp27-cp27m-manylinux1_i686.whl"), | ||
| urls = [("https://pypi.python.org/packages/c6/28/" + |
There was a problem hiding this comment.
Why is this constructed using string concatenation? I'd prefer just one long string, because that would make it clearer that there's only a single URL in the urls attribute.
There was a problem hiding this comment.
The multiline form has the advantage of being able to easily read the distinct pieces of the string, for instance that the wheel part of the URL matches the file path attr above. I'm not sure the value of having an unbroken URL is worth it in this case.
| "google_cloud_language-0.29.0-py2.py3-none-any.whl"), | ||
| urls = [("https://pypi.python.org/packages/6e/86/" + | ||
| "cae57e4802e72d9e626ee5828ed5a646cf4016b473a4a022f1038dba3460/" + | ||
| "google_cloud_language-0.29.0-py2.py3-none-any.whl")], |
There was a problem hiding this comment.
You need a hygenic macro to create these.
There was a problem hiding this comment.
You mean to factor out the whl name from the url and the downloaded_file_path? I'm not sure what value the macro adds besides code golfing -- What would the macro's arguments be, and what would the URL-minus-the-wheel-name be? Or would you suggest parsing the URL to determine downloaded_file_path?
I'm not even sure I like the downloaded file path being the wheel name as opposed to the default string, <repo name>/file/downloaded, but I kept it this way for compatibility with the existing tests. If I create a macro, that risks users loading it and adopting that convention without us having really thought it out.
There was a problem hiding this comment.
Okay, that makes sense. Leave it as-is.
|
Thanks for the review! |
This upgrades our dependencies to newer versions that are compatible with the new repo rules, and handles the fallout. Also add some comments to WORKSPACE.
rules_python now builds under Bazel 0.19.1 with
--incompatible_remove_native_git_repositoryandincompatible_remove_native_http_archive.Fixes #105.