You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2014/04/01 01:01:19 UTC

Re: Review Request 19854: Enable automake 1.14


> On March 31, 2014, 7:46 p.m., Benjamin Hindman wrote:
> >
> 
> Timothy St. Clair wrote:
>     FWIW I verified on several platforms prior to making this change, including latest Mac.

Awesome Tim, thanks for verifying it across platforms! Regardless, I think it's helpful to leave comments for future code archaeology. See below.


> On March 31, 2014, 7:46 p.m., Benjamin Hindman wrote:
> > configure.ac, line 34
> > <https://reviews.apache.org/r/19854/diff/1/?file=542452#file542452line34>
> >
> >     Can you explain why removing -Werror is necessary in a comment? Also, this seems dangerous ...
> 
> Timothy St. Clair wrote:
>     Warnings exist because the external builds do not have the 'subdir-objects' option enabled by default.  They are only warnings and as we progress further on MESOS-1071, this will become less of an issue.  
>     
>     From an automake perspective, it's not dangerous.

Okay, please let's leave a comment so this is easy for people to understand in the future.


> On March 31, 2014, 7:46 p.m., Benjamin Hindman wrote:
> > src/Makefile.am, lines 169-172
> > <https://reviews.apache.org/r/19854/diff/1/?file=542453#file542453line169>
> >
> >     Why are these changes necessary?
> 
> Timothy St. Clair wrote:
>     B/c the 'subdir-objects' option is specified and it's path is relative for both slave and master the collide.  The two options that exist are: 
>     
>     1. Change the name
>     2. Breakout into sub-Makefile.am(s)

Sorry for my ignorance, but where do they collide? Is it because of the relative path? Can we therefore make the path explicit so they won't collide? My understanding was that subdir-objects was supposed to make it easier to have things named the same in different sub-directories.

Also, I'm very intrigued by the sub-Makefile.am option, especially as it pertains to being able to go into, say, the 'master' subdirectory and do a make without requiring rebuilding things in say the 'slave' subdirectory. This has the benefits of being able to speed up the development cycle when you're trying to focus on a particular area of the codebase but making changes in headers that are used in numerous places. What's the work involved to pull these into sub-Makefile.am(s)? Will this allow us to do what I suggested above?

Also, if the best path is to just change the names I'd prefer us to pull out 'constants.cpp' and 'http.cpp' for both master and slave and give a very good comment to make this as explicit as possible:

# Please include as comment here as to why these names are mangled!
libmesos_no_3rdparty_la_SOURCES +=              \
        master/master_constants.cpp             \
        master/master_http.cpp                  \
        slave/slave_constants.cpp               \
        slave/slave_http.cpp


- Benjamin


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


On March 31, 2014, 6:16 p.m., Timothy St. Clair wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19854/
> -----------------------------------------------------------
> 
> (Updated March 31, 2014, 6:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Bugs: MESOS-577
>     https://issues.apache.org/jira/browse/MESOS-577
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Enable automake 1.14
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/configure.ac 2e0b72c223955a21573c0d2937420635c128b4d8 
>   bootstrap ed0bc36b18855f3e317dc6c059b7277fd7ac041b 
>   configure.ac 5404dc29a53368332b386a410226652ff7425fc7 
>   src/Makefile.am 47d03b393f68ebd4e9eba3206798a939078023c0 
>   src/master/constants.cpp  
>   src/master/http.cpp  
> 
> Diff: https://reviews.apache.org/r/19854/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy St. Clair
> 
>