You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by "Timothy St. Clair" <ts...@redhat.com> on 2014/05/01 23:15:27 UTC
Review Request 20983: Update system check (http-parser)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20983/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1175
https://issues.apache.org/jira/browse/MESOS-1175
Repository: mesos-git
Description
-------
Updates libprocess configure check for http-parser to build against system dependencies outlined in MESOS-1071
+ fixes libprocess local 'make check' which was broken
Diffs
-----
3rdparty/libprocess/3rdparty/Makefile.am 8ccf0ef
3rdparty/libprocess/Makefile.am 8990f38
3rdparty/libprocess/configure.ac fc5e2ad
Diff: https://reviews.apache.org/r/20983/diff/
Testing
-------
make check
Thanks,
Timothy St. Clair
Re: Review Request 20983: Update system check (http-parser)
Posted by "Timothy St. Clair" <ts...@redhat.com>.
> On Aug. 15, 2014, 1:45 a.m., Vinod Kone wrote:
> >
Stuff is flying all over atm, lets rally around https://reviews.apache.org/r/24734/ for final cleanup.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20983/#review50690
-----------------------------------------------------------
On May 5, 2014, 7:13 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20983/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 7:13 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1175
> https://issues.apache.org/jira/browse/MESOS-1175
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Updates libprocess configure check for http-parser to build against system dependencies outlined in MESOS-1071
> + fixes libprocess local 'make check' which was broken
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 980146b
> 3rdparty/libprocess/Makefile.am 8990f38
> 3rdparty/libprocess/configure.ac 0c7cc6d
>
> Diff: https://reviews.apache.org/r/20983/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 20983: Update system check (http-parser)
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20983/#review50690
-----------------------------------------------------------
3rdparty/libprocess/configure.ac
<https://reviews.apache.org/r/20983/#comment88555>
what about LDFLAGS?
3rdparty/libprocess/configure.ac
<https://reviews.apache.org/r/20983/#comment88556>
We also need to AC_CHECK_LIB for the lib here.
- Vinod Kone
On May 5, 2014, 7:13 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20983/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 7:13 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1175
> https://issues.apache.org/jira/browse/MESOS-1175
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Updates libprocess configure check for http-parser to build against system dependencies outlined in MESOS-1071
> + fixes libprocess local 'make check' which was broken
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 980146b
> 3rdparty/libprocess/Makefile.am 8990f38
> 3rdparty/libprocess/configure.ac 0c7cc6d
>
> Diff: https://reviews.apache.org/r/20983/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 20983: Update system check (http-parser)
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20983/#review42375
-----------------------------------------------------------
Ship it!
A handful of the issues were only partially addressed, but as they're just syntactic I'm going to make those cleanups and commit this (syntactic changes here: https://gist.github.com/benh/fe64caaac993d8feb3dd).
3rdparty/libprocess/configure.ac
<https://reviews.apache.org/r/20983/#comment76153>
This function was still not indented to be +2.
3rdparty/libprocess/configure.ac
<https://reviews.apache.org/r/20983/#comment76154>
Your change added a whitespace.
- Benjamin Hindman
On May 5, 2014, 7:13 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20983/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 7:13 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1175
> https://issues.apache.org/jira/browse/MESOS-1175
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Updates libprocess configure check for http-parser to build against system dependencies outlined in MESOS-1071
> + fixes libprocess local 'make check' which was broken
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 980146b
> 3rdparty/libprocess/Makefile.am 8990f38
> 3rdparty/libprocess/configure.ac 0c7cc6d
>
> Diff: https://reviews.apache.org/r/20983/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 20983: Update system check (http-parser)
Posted by "Timothy St. Clair" <ts...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20983/
-----------------------------------------------------------
(Updated May 5, 2014, 7:13 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Update per review.
Bugs: MESOS-1175
https://issues.apache.org/jira/browse/MESOS-1175
Repository: mesos-git
Description
-------
Updates libprocess configure check for http-parser to build against system dependencies outlined in MESOS-1071
+ fixes libprocess local 'make check' which was broken
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/Makefile.am 980146b
3rdparty/libprocess/Makefile.am 8990f38
3rdparty/libprocess/configure.ac 0c7cc6d
Diff: https://reviews.apache.org/r/20983/diff/
Testing
-------
make check
Thanks,
Timothy St. Clair
Re: Review Request 20983: Update system check (http-parser)
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On May 3, 2014, 2:47 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/configure.ac, line 182
> > <https://reviews.apache.org/r/20983/diff/1/?file=573032#file573032line182>
> >
> > Please wrap within 80 characters (this should now be caught with mesos-style.py):
> >
> > AM_CONDITIONAL([WITH_BUNDLED_HTTP_PARSER],
> > [test "x$with_bundled_http_parser" = "xyes"])
>
> Adam B wrote:
> Unfortunately, cpplint (used by mesos-style.py) does not check .ac files, just .cpp/hpp/c/h files. We'd have to do a bit more work to get some selective rules to also apply to .ac files, Makefiles, python or bash scripts, etc.
Ahh, thanks for letting us know Adam. How much more work? Seems like it might be worth it!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20983/#review42083
-----------------------------------------------------------
On May 1, 2014, 9:15 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20983/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 9:15 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1175
> https://issues.apache.org/jira/browse/MESOS-1175
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Updates libprocess configure check for http-parser to build against system dependencies outlined in MESOS-1071
> + fixes libprocess local 'make check' which was broken
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 8ccf0ef
> 3rdparty/libprocess/Makefile.am 8990f38
> 3rdparty/libprocess/configure.ac fc5e2ad
>
> Diff: https://reviews.apache.org/r/20983/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 20983: Update system check (http-parser)
Posted by Adam B <ad...@mesosphere.io>.
> On May 2, 2014, 7:47 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/configure.ac, line 182
> > <https://reviews.apache.org/r/20983/diff/1/?file=573032#file573032line182>
> >
> > Please wrap within 80 characters (this should now be caught with mesos-style.py):
> >
> > AM_CONDITIONAL([WITH_BUNDLED_HTTP_PARSER],
> > [test "x$with_bundled_http_parser" = "xyes"])
Unfortunately, cpplint (used by mesos-style.py) does not check .ac files, just .cpp/hpp/c/h files. We'd have to do a bit more work to get some selective rules to also apply to .ac files, Makefiles, python or bash scripts, etc.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20983/#review42083
-----------------------------------------------------------
On May 1, 2014, 2:15 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20983/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 2:15 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1175
> https://issues.apache.org/jira/browse/MESOS-1175
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Updates libprocess configure check for http-parser to build against system dependencies outlined in MESOS-1071
> + fixes libprocess local 'make check' which was broken
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 8ccf0ef
> 3rdparty/libprocess/Makefile.am 8990f38
> 3rdparty/libprocess/configure.ac fc5e2ad
>
> Diff: https://reviews.apache.org/r/20983/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 20983: Update system check (http-parser)
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20983/#review42083
-----------------------------------------------------------
Ship it!
Take one down, pass it around, 99 libraries left to unbundle. ;-)
3rdparty/libprocess/Makefile.am
<https://reviews.apache.org/r/20983/#comment75835>
FYI, I like pulling these into the 'if' blocks but we did not do this with the previous unbundling patches for ZooKeeper and LevelDB so we might want to revisit that to be consistent.
3rdparty/libprocess/Makefile.am
<https://reviews.apache.org/r/20983/#comment75832>
We haven't been as consistent as we've liked here, but when we wrap let's start on a newline and let's indent by +2 (see how we did it above for libprocess_la_LIBADD).
3rdparty/libprocess/Makefile.am
<https://reviews.apache.org/r/20983/#comment75831>
This is a weird one, but we use tabs here rather than spaces as most editors will align '\' correctly with tabs but not with spaces.
3rdparty/libprocess/configure.ac
<https://reviews.apache.org/r/20983/#comment75836>
Okay, so this is my fault but I missed this +4 indentation in the previous patches even though we had +2 everywhere else in these files. I've since fixed the +2 indentation from the previous patches as part of the whitespace/indent work that Adam and I did earlier this week but please let's do +2 going forward (also below in this file, and spaces not tabs please).
3rdparty/libprocess/configure.ac
<https://reviews.apache.org/r/20983/#comment75830>
Please wrap within 80 characters (this should now be caught with mesos-style.py):
AM_CONDITIONAL([WITH_BUNDLED_HTTP_PARSER],
[test "x$with_bundled_http_parser" = "xyes"])
- Benjamin Hindman
On May 1, 2014, 9:15 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20983/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 9:15 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1175
> https://issues.apache.org/jira/browse/MESOS-1175
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Updates libprocess configure check for http-parser to build against system dependencies outlined in MESOS-1071
> + fixes libprocess local 'make check' which was broken
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 8ccf0ef
> 3rdparty/libprocess/Makefile.am 8990f38
> 3rdparty/libprocess/configure.ac fc5e2ad
>
> Diff: https://reviews.apache.org/r/20983/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 20983: Update system check (http-parser)
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20983/#review42115
-----------------------------------------------------------
Bad patch!
Reviews applied: [20983]
Failed command: git apply --index 20983.patch
Error:
error: patch failed: 3rdparty/libprocess/configure.ac:116
error: 3rdparty/libprocess/configure.ac: patch does not apply
- Mesos ReviewBot
On May 1, 2014, 9:15 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20983/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 9:15 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1175
> https://issues.apache.org/jira/browse/MESOS-1175
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Updates libprocess configure check for http-parser to build against system dependencies outlined in MESOS-1071
> + fixes libprocess local 'make check' which was broken
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/Makefile.am 8ccf0ef
> 3rdparty/libprocess/Makefile.am 8990f38
> 3rdparty/libprocess/configure.ac fc5e2ad
>
> Diff: https://reviews.apache.org/r/20983/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>