aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Sirois <jsir...@apache.org>
Subject Re: Review Request 53836: Get pants using the same thrift binary as gradle.
Date Thu, 17 Nov 2016 23:32:00 GMT


> On Nov. 17, 2016, 2:48 p.m., Zameer Manji wrote:
> > Seems like a lot of patches to enable this behaviour. I'm not opposed but it seems
risky.
> 
> John Sirois wrote:
>     You find everything risky!
> 
> Zameer Manji wrote:
>     I'm just afraid that the more patches we have, the harder it will be to upgrade to
the next version of thrift.
>     
>     I'll let others weigh in here.

Aha - so you meant thrift patches in particular, not all the pants-related code.  Well, these
patches merely _fix_ thrift in general, they are not specific to aurora.  Hopefully 10.0.0
wil be better, but Jake would know better than I.


> On Nov. 17, 2016, 2:48 p.m., Zameer Manji wrote:
> > api/src/main/thrift/org/apache/aurora/gen/BUILD, line 34
> > <https://reviews.apache.org/r/53836/diff/1/?file=1565790#file1565790line34>
> >
> >     Can't we have our own 3rdparty thrift target that includes the thrift lib and
this prepare binary? Then we can put the dep in one place.
> 
> John Sirois wrote:
>     I can, but this requires a custom pants plugin (housed as a loose python sourcefile
in the repo) to expose, say, an `aurora_py_thrift_lib` target.  Your comments here expressed
reservations about that: https://reviews.apache.org/r/40219/
>     If you re-ack you're in-fact OK with a custom plugin for this, I'll whip up diff
2.
> 
> Zameer Manji wrote:
>     I think in this case, a custom pants plugin will be better, assuming the API in this
version is stable until pants 2.x

Yes, the API gaurantees are strong with pants since 1.0.0 and the plugin APIs needed for this
will be stable until 2.x.


- John


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53836/#review156248
-----------------------------------------------------------


On Nov. 16, 2016, 9:46 p.m., John Sirois wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53836/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2016, 9:46 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This required an upgrade to the latest pants dev release to correct
> an issue with the setup.py binary packager we use to generate sdists.
> 
> This is for sanity sake, and, once the TODO in
> `build-support/thrift/prepare_binary.sh` is addressed, it will also
> allow pants-patch-free addition of new platforms (thinking ARM).
> 
>  api/src/main/thrift/org/apache/aurora/gen/BUILD                        |  3 +++
>  api/src/main/thrift/org/apache/thermos/BUILD                           |  1 +
>  build-support/thrift/.gitignore                                        |  1 +
>  build-support/thrift/AURORA-1727.compiler.cpp.Makefile.in.patch        | 42 +++++++++++++------------------
>  build-support/thrift/AURORA-1727.lib.py.setup.py.patch                 | 45 +++++++++++++++++++++++++++++++++
>  {api/src/main/thrift/org/apache/thermos => build-support/thrift}/BUILD | 18 ++++---------
>  build-support/thrift/Makefile                                          |  2 +-
>  build-support/thrift/prepare_binary.sh                                 | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  build-support/thrift/thriftw                                           | 12 +++++++--
>  pants.ini                                                              |  7 +++++-
>  10 files changed, 158 insertions(+), 41 deletions(-)
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/BUILD f4004a3ca36078c6d24991db4beb68903a05652c

>   api/src/main/thrift/org/apache/thermos/BUILD cb655a2fd35d22f7d7e80c4311742bad763d8614

>   build-support/thrift/.gitignore 9a3adb69210ba3dbf3c1f408895561da37e8f4c3 
>   build-support/thrift/AURORA-1727.compiler.cpp.Makefile.in.patch b69e3fef137cd73c6f2b73201463a0705ef8082a

>   build-support/thrift/AURORA-1727.lib.py.setup.py.patch PRE-CREATION 
>   build-support/thrift/BUILD PRE-CREATION 
>   build-support/thrift/Makefile 48b174ad622288d2738a5fa37bbb72385fcc3a27 
>   build-support/thrift/prepare_binary.sh PRE-CREATION 
>   build-support/thrift/thriftw 50d6dfdeb16ca8bf14aaff7aa826e3d69c5e13f0 
>   pants.ini cecdb277f327f77b2652f76a30fc8d4ffd9ff1db 
> 
> Diff: https://reviews.apache.org/r/53836/diff/
> 
> 
> Testing
> -------
> 
> Locally green:
> ```
> rm -rf ~/.cache/pants .cache/
> git clean -fdx build-support/thrift
> ./pants clean-all
> ./build-support/jenkins/build.sh
> 
> vagrant ssh --command "rm -rf ~/.cache/pants && cd aurora && rm -rf .cache/
&& ./pants clean-all"
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> ./build-support/python/make-pycharm-virtualenv
> ```
> 
> 
> Thanks,
> 
> John Sirois
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message