chore: [hermetic-build] simplify usage of synthtool #2772
Conversation
This PR eliminates the need for: * `library_generation/owlbot/src/apply_repo_templates.py` * Passing the path to the config yaml to the postprocessing scripts ### How is it achieved? We set the environment variable `SYNTHTOOL_TEMPLATES` to point to a temp dir where we copy the contents of `owlbot/templates` plus a modification of the template using `sed` (instead of `synthtool.common_templates`). In this way, we avoid the usage of `apply_repo_templates.py`. generation_config.template_excludes. #### Why don't we use common_templates to pass libraries_bom_version? Because `owlbot.py` is a legacy script that was written when `java.common_templates` didn't take `libraries_bom_version` as a parameter. If we don't like this approach of having two template replacement workflows we could 1. Modify all owlbot.py files 2. Pass libraries_bom_version as an environment variable (not bad) #### What about template_excludes in the config yaml? We still need it to generate new libraries. This is done by rendering a new `owlbot.py` via templates.
diegomarquezp
added
hermetic-build
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
| export SYNTHTOOL_TEMPLATES=$(mktemp -d) | ||
| cp -r ${scripts_root}/owlbot/templates/* "${SYNTHTOOL_TEMPLATES}" | ||
| sed -i 's/\$\$__libraries_bom_version__\$\$/'"${libraries_bom_version}"'/g' "${SYNTHTOOL_TEMPLATES}/java_library/README.md" |
There was a problem hiding this comment.
What if we migrate java.py to sdk-platform-java? Do you think of another solution?
There was a problem hiding this comment.
It would be ideal. A couple considerations:
owlbot.pyimportssynthtool, meaning that we would need to not only transferjava.pybut other scripts such ascommon.py(or we could merge all the logic in a single script)- If we migrate synthtool/java.py, we would still need a way to pass
libraries_bom_versiontoowlbot.py. We can either modifyowlbot.pyor we can rely on an environment variable consumed in java.py
There was a problem hiding this comment.
Another more hacky approach would be to pass the version in repo-metadata
There was a problem hiding this comment.
The logic here seems not easy to maintain, as it is tightly coupled with a magic string __libraries_bom_version__.
I'm leaning towards the environment variable approach you mentioned. If we go with that approach, I guess we should not need any changes in this file or the template, we just need to load the libraries bom version from environment variable and add it to the kwargs in synthtool?
There was a problem hiding this comment.
I added googleapis/synthtool#1967 to infer the bom version from the environment. Once that's merged, we can enable the env var approach.
There was a problem hiding this comment.
What was this file used for? I think it was not only for libraries_bom_version? Or maybe it was a mistake that we rendered the templates twice?
There was a problem hiding this comment.
It allowed us to run common_templates with the list of template_excludes listed in the config yaml
| export SYNTHTOOL_TEMPLATES=$(mktemp -d) | ||
| cp -r ${scripts_root}/owlbot/templates/* "${SYNTHTOOL_TEMPLATES}" | ||
| sed -i 's/\$\$__libraries_bom_version__\$\$/'"${libraries_bom_version}"'/g' "${SYNTHTOOL_TEMPLATES}/java_library/README.md" |
There was a problem hiding this comment.
The logic here seems not easy to maintain, as it is tightly coupled with a magic string __libraries_bom_version__.
I'm leaning towards the environment variable approach you mentioned. If we go with that approach, I guess we should not need any changes in this file or the template, we just need to load the libraries bom version from environment variable and add it to the kwargs in synthtool?
|
|
|
cloud build failed after merging this PR: |
This PR eliminates the need for: * `library_generation/owlbot/src/apply_repo_templates.py` * Passing the config yaml path to the postprocessing scripts ### How is it achieved? We set the environment variable `SYNTHTOOL_TEMPLATES` to point to a temp dir where we copy the contents of `owlbot/templates` plus a modification of the template using `sed` (instead of `synthtool.common_templates`). In this way, we avoid the usage of `apply_repo_templates.py`. generation_config.template_excludes. #### Why don't we use common_templates to pass libraries_bom_version? Because `owlbot.py` is a legacy script that was written when `java.common_templates` didn't take `libraries_bom_version` as a parameter. If we don't like this approach of having two template replacement workflows we could 1. Modify all owlbot.py files 2. Pass libraries_bom_version as an environment variable (not bad) #### What about template_excludes in the config yaml? We still need it to generate new libraries. This is done by rendering a new `owlbot.py` via templates.
This PR eliminates the need for:
library_generation/owlbot/src/apply_repo_templates.pyHow is it achieved?
We set the environment variable
SYNTHTOOL_TEMPLATESto point to atemp dir where we copy the contents of
owlbot/templatesplus amodification of the template using
sed(instead ofsynthtool.common_templates). In this way, we avoid the usage ofapply_repo_templates.py.generation_config.template_excludes.
Why don't we use common_templates to pass libraries_bom_version?
Because
owlbot.pyis a legacy script that was written whenjava.common_templatesdidn't takelibraries_bom_versionas aparameter. If we don't like this approach of having two template
replacement workflows we could
What about template_excludes in the config yaml?
We still need it to generate new libraries. This is done by rendering a
new
owlbot.pyvia templates.