Make the java and python setup more consistent #71
Conversation
consistency with python files.
…test the python rules.
{java,py}_appengine.bzl use it.
|
If you can break it up great, if not also ok. I am backlogged on a few things so it will take me a day before I can get to this. |
|
|
||
| load("//appengine:appengine.bzl", "appengine_repositories") | ||
| appengine_repositories() | ||
| load("//appengine:java_appengine.bzl", "java_appengine_repositories") |
There was a problem hiding this comment.
prefer to group loads first
There was a problem hiding this comment.
Done. It would be nice if buildifier did that for me.
| "${JVM_FLAGS_CMDLINE[@]}" | ||
| "-Dappengine.sdk.root=${APP_ENGINE_ROOT}" | ||
| ${main_class} | ||
| "--disable_update_check" |
There was a problem hiding this comment.
It's the java counterpart to this change to the python file.
https://github.com/bazelbuild/rules_appengine/pull/56/files#diff-b042c7a50d680fc8c3189481231c70e5R134
| @@ -0,0 +1,289 @@ | |||
| # Copyright 2015 The Bazel Authors. All rights reserved. | |||
There was a problem hiding this comment.
is there no way to use 'git mv' to preserve the previous history?
There was a problem hiding this comment.
This is the main reason I did this as several changes in my branch. It works for the first change which was just a rename.
3061013#diff-3b8b4a89deffca2ab1dabfb178ff7012
but every time I made changes to the moved file, git seems to forget the history. At least during the diff.
39266ae#diff-3b8b4a89deffca2ab1dabfb178ff7012
Running this in my client does show the full history back to 2015.
git log --follow appengine/java_appengine.bzl
|
LGTM |
rogerhub
left a comment
rogerhub
left a comment
There was a problem hiding this comment.
I suppose there's not much point sending these a week after this was merged, but here are some issues I noticed.
| fi | ||
|
|
||
| ROOT=$PWD | ||
| tmp_dir=$(mktemp -d ${{TMPDIR:-/tmp}}/war.XXXXXXXX) |
There was a problem hiding this comment.
These double curly braces are invalid. They were originally Python string escape sequences and should be unescaped here (below as well).
| ROOT=$PWD | ||
| tmp_dir=$(mktemp -d ${{TMPDIR:-/tmp}}/war.XXXXXXXX) | ||
| cp -R $ROOT $tmp_dir | ||
| trap "{{ cd ${{root_path}}; rm -rf $tmp_dir; }}" EXIT |
There was a problem hiding this comment.
What's root_path? Doesn't look like it's defined anywhere.
There was a problem hiding this comment.
Preexisting issue. I'll have to research what it should have been. Is it even needed?
There was a problem hiding this comment.
It looks like this section was copied from the java counterpart and ROOT was originally called root_path.
|
|
||
| ROOT=$PWD | ||
| tmp_dir=$(mktemp -d ${{TMPDIR:-/tmp}}/war.XXXXXXXX) | ||
| cp -R $ROOT $tmp_dir |
There was a problem hiding this comment.
Preexisting issue, but these variables should be quoted (below as well).
| fi | ||
|
|
||
| else | ||
| echo "\033[1;31mERROR:\033[0m Application ID must be provided as first argument |
There was a problem hiding this comment.
It appears it does. Preexisting issue.
| fi | ||
|
|
||
| rm -Rf $tmp_dir | ||
| trap - EXIT |
There was a problem hiding this comment.
Why not just let the exit handler delete tmp_dir?
There was a problem hiding this comment.
I didn't write this, so I don't know.
There was a problem hiding this comment.
The trap - EXIT deletes the trap above so the rm defined there doesn't run on exit. This was also copied from the java counterpart, but I don't know why it would replicate the trap. I've removed it in my change and we can discuss it there.
|
I'll create another PR to address these. |
- Quote bash variable when possible.
- Fix an undefined $root_dir
- Remove a redundant rm and let the defined trap handler do it.
- echo -e is needed when trying to display escape codes.
- Remove incorrect {{foo}} double curly braces.
- Quote bash variable when possible.
- Fix an undefined $root_dir
- Remove a redundant rm and let the defined trap handler do it.
- echo -e is needed when trying to display escape codes.
- Remove incorrect {{foo}} double curly braces.
4 participants
Related to #10
@pmbethe09 offered to do the review. If you want, I can try to break this up into smaller PRs.