Overhaul workspace file to use only a single setup function #129
Conversation
…o workspace-overhaul
|
Never mind for now. This is very, very, broken. Apparently my verification was insufficient. |
|
OK. PR looks good to go. Review at your leisure :) |
setup.bzl
Outdated
| # Protobuf expects an //external:python_headers label which would contain the | ||
| # Python headers if fast Python protos is enabled. Since we are not using fast | ||
| # Python protos, bind python_headers to a dummy target. | ||
| native.bind( |
There was a problem hiding this comment.
This wasn't in the WORKSPACE file before, what changed?
There was a problem hiding this comment.
It was part of skydoc_repositories(), which I've essentially moved into setup().
setup.bzl
Outdated
| actual = "//:dummy", | ||
| ) | ||
|
|
||
| http_archive( |
There was a problem hiding this comment.
I think you should add these repositories conditionally as well. While you're at it, may as well let your users overwrite them
| strip_prefix = "MarkupSafe-0.23", | ||
| ) | ||
|
|
||
| native.bind( |
There was a problem hiding this comment.
I know it's not new to this CL, but these binds are kind of unnecessary (this comment is not blocking but I don't think that's an option on github)
There was a problem hiding this comment.
They are? I'm not sure what you mean they are unnecessary (though admittedly I haven't tried removing them)
There was a problem hiding this comment.
Ok, unnecessary is an exaggeration, but it is just a convenience rename and I believe that there is a movement to get rid of bind (and in this case I think alias would work just as well)
There was a problem hiding this comment.
As discussed offline, it's pretty tricky removing these or replacing them with alias as-is :) Maybe another time
| name = "six", | ||
| actual = "@six_archive//:six", | ||
| ) | ||
| skydoc_repositories = _skydoc_repositories |
There was a problem hiding this comment.
Why import it as _skydoc_repositories if you are renaming it its default name?
There was a problem hiding this comment.
Because
- Loading non-explicitly-exported symbols is disallowed (Without redefining skydoc_repositories in this file, a bzl file depending on this file would not be able to load skydoc_repositories)
- Overriding an imported global symbol is disallowed
(I'm not allowed to do skydoc_repositories = skydoc_repositories)
Thus, I have to load it in as a different symbol name then re-export it under the desired name.
I would indeed like to do this, but I'd like to do this as a followup separately.
|
Fix skydoc_repositories() so it does not require dependencies to be previously defined in the WORKSPACE file.