improve the deploy jar creation #85
Conversation
|
Can one of the admins verify this patch? |
| jar=_get_jar_path(ctx.files._jar), | ||
| java=ctx.file._java.path, | ||
| manifest=ctx.outputs.manifest.path) | ||
| args = ["-m", ctx.outputs.manifest.path, ctx.outputs.deploy_jar.path] |
There was a problem hiding this comment.
what does -m stand for? Obviously a noob question but maybe should be a constant? a bit of a magic symbol?
|
@johnynek are there any tests around this feature? If not I'd add one or two. If there are then this does look like refactoring so it's not needed |
|
@ittaiz there are existing tests that exercise this (the java consumption of scala deploy jars) and on top of that, this fixes the issue seen on linux (the contents of some jars were too long, so we could not make the fat jar). |
|
@ittaiz I am not sure I follow. That flag is how we signal to the jar program that we are passing a manifest file. I think your suggestion is confusing, I don't understand it. In the interest of you saying "ship it" I am tempted to take the suggestion, but it seems to mislead the reader if we say "USE_EXISTING_MANIFEST_FLAG". What is the existing one? The ones in the inner jars? By the way, we generally have not documented flags in other scripts. So this is a bit of a break to concern ourselves so deeply with this one flag. I'll try one more time and see if you like it. |
|
@ittaiz I updated the code. If you have any suggestions on how to improve the wording, please make a suggestion. Otherwise, I can merge and you can make a PR that improves it if you like. I appreciate you taking the time to look at the change. |
|
LGTM |
* upstream/master: use strings rather than HOST/DATA_CFG Add cfg attribute on executable labels Switch to using shorter stack traces. Catch throwables from type errors make thrift targets quieter (bazelbuild#94) Fix jvm_flag support (bazelbuild#93) Make compile timing optional, with default false Update README.md Persistent/worker scala compiler (bazelbuild#91) fix typo in README.md (bazelbuild#89) Fix for thrift rule issue that manifests with remote repos (bazelbuild#83) improve the deploy jar creation (bazelbuild#85) Use bind's for twitter scrooge so local repo's can override scrooge and thrift versions (bazelbuild#84)
This does not pass the zip contents through the file system and makes deploy jar creation part a executable action. It should be both faster, but also safer for creation of deploy jars on linux systems (such as Docker or with encrypted home directories) that often have limited file lengths that might be violated by jars.
review? @ianoc @ittaiz ?