Fix for thrift rule issue that manifests with remote repos #83
Conversation
|
Can one of the admins verify this patch? |
|
@ianoc review? |
| pass | ||
| else: | ||
| tmp_pref = pref | ||
| for end in range(0, len(pref) + 1): |
There was a problem hiding this comment.
no while in skylark. :(
There was a problem hiding this comment.
(skylark may be a total language... No recursion, only for loops on collections, I think everything terminates).
If you consider any method error to be a type error (which a better compiler would have caught, I think it may be total....)
|
looks reasonable to me. Is it possible to add a test for the issue so it doesn't re-occur? |
|
@ianoc we can add a test. It is not super easy to do so. We need to set up some kind of remote repo (maybe a nested local repository will do it), then verify that the build of a target that depends on the remote thrift target has all the thrift files we expect. |
|
I'd be inclined to merge it since its strictly better, but maybe we should just file an issue for future clean up? |
|
Just as general inquiry:
|
|
@ittaiz the suggestion is for the tests to be run with Did you see the movie Inception? Like that. |
|
:) Got you.
|
* 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 (bazel-contrib#94) Fix jvm_flag support (bazel-contrib#93) Make compile timing optional, with default false Update README.md Persistent/worker scala compiler (bazel-contrib#91) fix typo in README.md (bazel-contrib#89) Fix for thrift rule issue that manifests with remote repos (bazel-contrib#83) improve the deploy jar creation (bazel-contrib#85) Use bind's for twitter scrooge so local repo's can override scrooge and thrift versions (bazel-contrib#84)
The problem is that the path is not the same when remote as local. The current fix:
.*prefix(.*)$.The above two fixes make this rule usable in both local and remote contexts for me.