Skip to content
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

feat: Initial draft of GAPIC Bazel Extensions gapic-generator-python #342

Merged
merged 4 commits into from
Mar 16, 2020

Conversation

vam-google
Copy link
Contributor

@vam-google vam-google commented Mar 13, 2020

This is a "minimal viable product" version of the rule, which simply call the python microgenerator to generate a client. No post and/or preprocessing is implemented, but the actual GAPIC code is produced successfully.

Successfully tested on googleapis/google/cloud/lanugage/v1 API.

To use it from googleapis, add the following to the # Python in WORKSPACE file in googleapis:

load("@rules_python//python:repositories.bzl", "py_repositories")
py_repositories()

load("@rules_python//python:pip.bzl", "pip_repositories")
pip_repositories()

# Change upstream repository once PR is merged
http_archive(
    name = "gapic_generator_python",
    urls = ["https://github.com/vam-google/gapic-generator-python/archive/77637d9bfdbfcb67b02b3d6bbbc6938de44214ce.zip"],
    strip_prefix = "gapic-generator-python-77637d9bfdbfcb67b02b3d6bbbc6938de44214ce",
)

load("@gapic_generator_python//:repositories.bzl", "gapic_generator_python")

gapic_generator_python()

load("@gapic_generator_python_pip_deps//:requirements.bzl", "pip_install")
pip_install()

and the following at the top of the file (needs to go to the top, because rules_python of older version is a transitive dependency of grpc rules, we need a newer version to be used in workspace, so specify it first to override the older one).

http_archive(
    name = "rules_python",
    url = "https://github.com/bazelbuild/rules_python/archive/748aa53d7701e71101dfd15d800e100f6ff8e5d1.zip",
    strip_prefix = "rules_python-748aa53d7701e71101dfd15d800e100f6ff8e5d1"
)

The in google/cloud/langugae/v1/BUILD.bazel file in Python section add

load("@gapic_generator_python//rules_python_gapic:py_gapic.bzl", py_gapic_library2 = "py_gapic_library")

py_gapic_library2(
    name = "language_py_gapic2",
    srcs = [":language_proto"],
)

and build it with:

bazel build //google/cloud/language/v1:language_py_gapic2

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 13, 2020
@codecov
Copy link

codecov bot commented Mar 13, 2020

Codecov Report

Merging #342 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #342   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          25     25           
  Lines        1397   1397           
  Branches      289    289           
=====================================
  Hits         1397   1397

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3c178c...7308273. Read the comment docs.

Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, just a few questions.

requirement("pypandoc"),
requirement("PyYAML"),
],
python_version = "PY3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we can be more specific about the python version requirement? Some of the code in the generator requires features from >=3.6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are ways to be more specific, but they have their own issues (i.e. make the build less hermetic and less portable). Practically speaking, it should be picking the latest installed version of python on the host machine, and if there is 3.6+ bazel will pick it up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Good to know.

@@ -0,0 +1,18 @@
load("@com_google_api_codegen//rules_gapic:gapic.bzl", "proto_custom_library")

def py_gapic_library2(name, srcs, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pass custom options through **kwargs? Is it straightforward?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we can, we can also always add those custom options explicitly as args in this rule. It is straightforward to do (I did not add any because of my lack of knowledge and context here).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. We will try using kwargs first and see if explicit args are necessary.

@vam-google
Copy link
Contributor Author

@software-dov I cleaned up this PR, and added instructions how it can be used from googleapis to test stuff, PTAL.

Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One question: I don't see a hash anywhere that corresponds to gapic-generator-python; other things yes, but not that one. Does the rule automatically use the remote repository master? Do I need to update anything if I cut a new release?

@vam-google
Copy link
Contributor Author

@software-dov Maybe there is some misunderstanding how this things (bazel rules and bazel workspaces) work together.

There is a workspace which defines a rule (this one, gapic-generator-python defines the py_gapic_library rule), and a workspace which uses the rule (it often can be same repo, but for our case it is the googleapis repository). To wire these two things together, googleapis in its WORKSPACE file defines the actual version of gapic-generator-python repository (i.e. the one, which defines the rule) to pull. In the instructions provided in this PR description it corresponds to the following lines:

http_archive(
    name = "gapic_generator_python",
    urls = ["https://github.com/vam-google/gapic-generator-python/archive/77637d9bfdbfcb67b02b3d6bbbc6938de44214ce.zip"],
    strip_prefix = "gapic-generator-python-77637d9bfdbfcb67b02b3d6bbbc6938de44214ce",
)

Where 77637d9bfdbfcb67b02b3d6bbbc6938de44214ce is literally the github commit ID. It does not have to be a github commit id, it can be a versioned release tag, for examle (if there is any). That is it, you do not need to cut any releases of anything (but still can, if you want). When you update something in gapic-generator-python and want to use it in googleapis, you just need to update commit id, or, even simly use local_repository rule to pull it (so your local build will simply pull stuff from your harddrive).

@vam-google
Copy link
Contributor Author

@software-dov Pleas push this PR if it looks good to you (i don't have rights).

@software-dov
Copy link
Contributor

Okay, so if I do cut new releases (or make new commits on top of master) I need to change the hash in googleapis' workspace file. Got it.

@software-dov software-dov merged commit cc7ab0b into googleapis:master Mar 16, 2020
@vam-google vam-google changed the title [WIP] feat: Initial draft of GAPIC Bazel Extensions gapic-generator-python feat: Initial draft of GAPIC Bazel Extensions gapic-generator-python Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants