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] Add microgenerator support to synthtool. #252

Merged
merged 7 commits into from
Jul 22, 2019
Merged

Conversation

lukesneeringer
Copy link
Contributor

This commit adds microgenerator support to synthtool, and defines shortcut methods for the three languages where the ACT team currently publishes Docker images.

A couple of things were not entirely clear to me when I wrote this, so I am looking for some guidance:

  • What is the appropriate testing regimen? I did not see any existing unit tests that seemed to exercise the GAPICGenerator or Artman classes, so I did not attempt to write any for mine either.
  • It is not clear to me how you want to handle output directories. This might be because artman largely took the option out of your hands? It seems to me that the right thing to do is to make a temporary directory, generate code there, and copy out of it, but currently to do that correctly, it probably requires that the individual synth.py files handle the temporary directory context manager logic.

@lukesneeringer lukesneeringer requested a review from crwilcox May 25, 2019 22:11
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 25, 2019
# Ensure the desired output directory exists.
# If none was provided, create a temporary directory.
if not output_dir:
output_dir = tempfile.mkdtemp()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crwilcox I think it is pretty likely that this is the wrong thing to do, but it is not obvious to me what a better option is.

Here are a couple potential possibilities:

  • Return a TemporaryDirectory object instead of a Path.
    • Pros: Easy for synth.py files; they just use the context manager interface to copy out of the temporary directory and cleanup magically happens.
    • Cons: Violates / changes the interface observed by GAPICGenerator.
  • Require an output directory.
    • Pros: Unambiguous. If you create a temporary directory to handle this, you clean it up.
    • Cons: Slightly more code than the option above (~1-2 more lines in each synth.py file). Slightly violates the interface by requiring output_dir.
  • What is here now.
    • Pros: Holds to the existing interface.
    • Cons: Very easy to leave temporary directories lying around.
  • Try to return a frankenstein object that subclasses both Path and TemporaryDirectory. (In this case, I would remove output_dir entirely and only write to temp directories.)
    • Pros: Holds to the existing interface.
    • Cons: Seems dirty. Edge cases intuitively seem likely.

Copy link
Contributor

@busunkim96 busunkim96 May 31, 2019

Choose a reason for hiding this comment

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

I am OK with the approach here. Synthtool already leaves behind generated libraries in ~/.cache/synthtool/googleapis/artman-genfiles/.

In fact, we have a note in the README that recommends looking at that directory when you're doing a generation.

SynthTool will run Artman which will create generated code that can be found at ~/.cache/synthtool/googleapis<-private>/artman_genfiles. This is useful for figuring out what it is you need to copy for your specific library.

Would it be reasonable to have the microgenerator default to a similar location for predictability?

For reference:
Artman's output_dir is root_dir / "artman-genfiles"

def run(self, image, root_dir, config, *args):
"""Executes artman command in the artman container.
Args:
root_dir: The input directory that will be mounted to artman docker
container as local googleapis directory.
Returns:
The output directory with artman-generated files.
"""
container_name = "artman-docker"
output_dir = root_dir / "artman-genfiles"

root_dir is the path for googleapis.

output_root = artman.Artman().run(
f"googleapis/artman:{artman.ARTMAN_VERSION}",
googleapis,
config_path,
gapic_language_arg,

And googleapis is cloned to a cache dir, by default.

def clone(
url: str,
dest: pathlib.Path = None,
committish: str = "master",
force: bool = False,
depth: int = None,
) -> pathlib.Path:
if dest is None:
dest = cache.get_cache_dir()

cache_dir = pathlib.Path.home() / ".cache" / "synthtool"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @busunkim96 for looking at this!

The new generators are protoc plugins, which require a specified directory to output to (that is the fundamental protoc plugin interface), so I can not have the generator itself default to any location.

However, I can have synthtool pick a default location (e.g. python-gapic-genfiles) and behave this way if that is your preference. That seems odd to me, but you are the end user, and I defer to your preference. :)

Should I make that change? Happy to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lukesneeringer Yes please! I think it would be the least surprising for the microgenerator to also put files in the synthtool cache dir. Thank-you. 😄

@busunkim96 busunkim96 self-requested a review May 30, 2019 22:01
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 4, 2019
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Jul 3, 2019
@busunkim96
Copy link
Contributor

If folks want the library to be in some other directory, that can be changed in a future PR.

@busunkim96 busunkim96 merged commit f4aae59 into master Jul 22, 2019
@busunkim96 busunkim96 deleted the microgenerators branch September 9, 2019 23:26
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.

4 participants