Closed lyao-77 closed 1 year ago
Do not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed.
Trivial Change
Code
Architecture
@alexjpwalker Could you take a look at this PR?
Thanks for the PR!
The intent looks clear and reasonable. I'll find time to review it in the coming days.
Also, it would be nice to have the option to not generate a wheel and only sdist.
Also, it would be nice to have the option to not generate a wheel and only sdist.
Hi @dmitrii-ubskii, I have addressed all of your comments by pushing new commits. Specifically for this comment, I find it kind of difficult to make the wheel build optional because the outputs
field of assemble_pip
seems static and it is not supposed to be dynamic in bazel. I think it doesn't hurt to build a .whl
file in this case.
I did push a commit to make deployment optional. So although it will build both .whl
and tar.gz
, the user could choose if they would like to deploy either one of them or both.
Looks good! @alexjpwalker, can you take a look?
@alexjpwalker @dmitrii-ubskii Hi I have addressed all of the new comments. Can you take a look again? Thanks!
@lyao-77 Hi, please note that we've made some changes on top of your changes to bring the rule back in line with the standard architecture we've chosen for this repository: https://github.com/vaticle/bazel-distribution/pull/373
You'll still be able to use exactly the functionality you require as implemented in this PR, just with a different API.
I'm actually also going to remove the universal
part of the python wheel distribution - if this is something you would like, you should add it as an argument to assemble_pip
. Most Python libraries these days are not Python 2 and 3 compatible, which is one of the requirements for Universal = 1: https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/#wheels
I'm actually also going to remove the universal
part of the python wheel distribution - if this is something you would like, you should add it as an argument to assemble_pip
. Most Python libraries these days are not Python 2 and 3 compatible, which is one of the requirements for Universal = 1: https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/#wheels
This also means we're going to fix the assemble and deploy rules to be only compatible with Python 3.x, which is implemented in #375
Currently there are some issues with these two rules: 'assemble_pip' and 'deploy_pip'.
assemble_pip: (1) data files are not included in "tarball" build; (2) it cannot produce "wheel" file which is a common needs for many use cases.
deploy_pip: (1) currently the deployment relies on "repository_registry" and username&password. However, we use "~/.pypirc" to config twine and we believe many other users have the same config. "pypi_profile" attr is added to enable this use case; (2) this rule will run into an error due to missing deps in requirements.txt; (3) this rule will run into an error due to that the "repo_type_key" arg in deploy.py is missing when running the script. Practically, only one of "release" and "snapshot" can be used for deployment; (4) this rule can only deploy single package ("tar.gz") which is incompatible with what we did for "assemble_pip".
This PR is to address all of the issues above.