You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Paul Brett <pb...@twitter.com.INVALID> on 2015/06/26 00:18:29 UTC
Project wide updates to #include statements
The style guide prescribes the order of header file inclusions for the
project and requires that we #include or make explicit forward declarations
for any functions we use, however we were only enforcing this at review
time manually and not commit time. I would like to turn on the checks at
commit time, so I am in the process of raising changes against stout,
libprocess and mesos to bring the code base into compliance. Once this is
completed, I propose to update cpplint.py and mesos-style.py to enforce the
style guide.
Anyone interested can comment on the following tickets:
https://issues.apache.org/jira/browse/MESOS-2926 Extend
mesos-style.py/cpplint.py to check #include files
https://issues.apache.org/jira/browse/MESOS-2927 Update mesos #include
headers
https://issues.apache.org/jira/browse/MESOS-2928 Update stout #include
headers
https://issues.apache.org/jira/browse/MESOS-2929 Update libprocess #include
headers
-- Paul Brett
Re: Project wide updates to #include statements
Posted by Benjamin Mahler <be...@gmail.com>.
+1 to cody's suggestion for stout / libprocess.
Looking forward to seeing include style automated, will help us be more
effective with reviews. :)
It looks like we can further modify "include what you use" to capture stout
and libprocess headers, probably worth the effort:
https://github.com/google/styleguide/blob/gh-pages/cpplint/cpplint.py#L5604
On Thu, Jun 25, 2015 at 5:19 PM, Cody Maloney <co...@mesosphere.io> wrote:
> On Thu, Jun 25, 2015 at 5:01 PM Paul Brett <pb...@twitter.com.invalid>
> wrote:
>
> > As Kapil has pointed out on MESOS-2927, all the library files should be
> > identified by '#include <...>' and all the project files should use
> > '#include "..."' - i don't know yet if we have been consistent about
> this.
> >
>
> They aren't consistent currently. For example:
> https://issues.apache.org/jira/browse/MESOS-1877
>
> I would actually suggest we switch to using <> everywhere. Reduces
> cognitive burden reading the code, and practically headers move over time,
> If something moves from src/mesos/master/master.hpp to
> include/mesos/master/master.hpp given the current <> vs "" rules, every
> file which includes that file needs to be updated.
>
> Cody
>
>
> > I have created the stout patch on this basis and plan to incorporate any
> > comments before moving onto libprocess.
> >
> > -- Paul
> >
> > On Thu, Jun 25, 2015 at 3:31 PM, Marco Massenzio <ma...@mesosphere.io>
> > wrote:
> >
> > > As I mentioned in the review that Paul submitted, I've been working on
> > > cpplint.py to make it more Mesos-friendly.
> > > I have also submitted a few Pull Requests
> > > <https://github.com/google/styleguide/pulls> to the original github
> > repo,
> > > but got neither love nor attention.
> > >
> > > My fork is here: https://github.com/massenz/styleguide
> > > and the code in the `master` branch (
> > > https://github.com/massenz/styleguide/tree/master) has all my changes;
> > it
> > > currently works well with the existing code (in that, submitted, valid
> > > Mesos code does not raise errors) apart from the opening brace on a
> > newline
> > > for multi-line method declarations.
> > >
> > > Love to get contributions and pull requests folks, feel free to submit!
> > >
> > > An example CPPLINT.cfg that works with the code in `master` is
> something
> > > like this:
> > >
> > > $ cat CPPLINT.cfg
> > > # Apache Mesos cpplint custom file
> > >
> > > extensions=cpp,hpp
> > > access_keywords_indent=0
> > > headers=h,hpp
> > > custom_headers=mesos,process,stout
> > > set braces_newline
> > >
> > > PS - am I the only one to find it hilarious that code that supposedly
> > > checks on style correctness is written in some of the least readable,
> > badly
> > > PEP8-violating Python? :)
> > >
> > > *Marco Massenzio*
> > > *Distributed Systems Engineer*
> > >
> > > On Thu, Jun 25, 2015 at 3:18 PM, Paul Brett <pbrett@twitter.com.invalid
> >
> > > wrote:
> > >
> > > > The style guide prescribes the order of header file inclusions for
> the
> > > > project and requires that we #include or make explicit forward
> > > declarations
> > > > for any functions we use, however we were only enforcing this at
> > review
> > > > time manually and not commit time. I would like to turn on the
> checks
> > at
> > > > commit time, so I am in the process of raising changes against stout,
> > > > libprocess and mesos to bring the code base into compliance. Once
> this
> > > is
> > > > completed, I propose to update cpplint.py and mesos-style.py to
> enforce
> > > the
> > > > style guide.
> > > >
> > > > Anyone interested can comment on the following tickets:
> > > >
> > > > https://issues.apache.org/jira/browse/MESOS-2926 Extend
> > > > mesos-style.py/cpplint.py to check #include files
> > > > https://issues.apache.org/jira/browse/MESOS-2927 Update mesos
> #include
> > > > headers
> > > > https://issues.apache.org/jira/browse/MESOS-2928 Update stout
> #include
> > > > headers
> > > > https://issues.apache.org/jira/browse/MESOS-2929 Update libprocess
> > > > #include
> > > > headers
> > > >
> > > >
> > > > -- Paul Brett
> > > >
> > >
> >
> >
> >
> > --
> > -- Paul Brett
> >
>
Re: Project wide updates to #include statements
Posted by Cody Maloney <co...@mesosphere.io>.
On Thu, Jun 25, 2015 at 5:01 PM Paul Brett <pb...@twitter.com.invalid>
wrote:
> As Kapil has pointed out on MESOS-2927, all the library files should be
> identified by '#include <...>' and all the project files should use
> '#include "..."' - i don't know yet if we have been consistent about this.
>
They aren't consistent currently. For example:
https://issues.apache.org/jira/browse/MESOS-1877
I would actually suggest we switch to using <> everywhere. Reduces
cognitive burden reading the code, and practically headers move over time,
If something moves from src/mesos/master/master.hpp to
include/mesos/master/master.hpp given the current <> vs "" rules, every
file which includes that file needs to be updated.
Cody
> I have created the stout patch on this basis and plan to incorporate any
> comments before moving onto libprocess.
>
> -- Paul
>
> On Thu, Jun 25, 2015 at 3:31 PM, Marco Massenzio <ma...@mesosphere.io>
> wrote:
>
> > As I mentioned in the review that Paul submitted, I've been working on
> > cpplint.py to make it more Mesos-friendly.
> > I have also submitted a few Pull Requests
> > <https://github.com/google/styleguide/pulls> to the original github
> repo,
> > but got neither love nor attention.
> >
> > My fork is here: https://github.com/massenz/styleguide
> > and the code in the `master` branch (
> > https://github.com/massenz/styleguide/tree/master) has all my changes;
> it
> > currently works well with the existing code (in that, submitted, valid
> > Mesos code does not raise errors) apart from the opening brace on a
> newline
> > for multi-line method declarations.
> >
> > Love to get contributions and pull requests folks, feel free to submit!
> >
> > An example CPPLINT.cfg that works with the code in `master` is something
> > like this:
> >
> > $ cat CPPLINT.cfg
> > # Apache Mesos cpplint custom file
> >
> > extensions=cpp,hpp
> > access_keywords_indent=0
> > headers=h,hpp
> > custom_headers=mesos,process,stout
> > set braces_newline
> >
> > PS - am I the only one to find it hilarious that code that supposedly
> > checks on style correctness is written in some of the least readable,
> badly
> > PEP8-violating Python? :)
> >
> > *Marco Massenzio*
> > *Distributed Systems Engineer*
> >
> > On Thu, Jun 25, 2015 at 3:18 PM, Paul Brett <pb...@twitter.com.invalid>
> > wrote:
> >
> > > The style guide prescribes the order of header file inclusions for the
> > > project and requires that we #include or make explicit forward
> > declarations
> > > for any functions we use, however we were only enforcing this at
> review
> > > time manually and not commit time. I would like to turn on the checks
> at
> > > commit time, so I am in the process of raising changes against stout,
> > > libprocess and mesos to bring the code base into compliance. Once this
> > is
> > > completed, I propose to update cpplint.py and mesos-style.py to enforce
> > the
> > > style guide.
> > >
> > > Anyone interested can comment on the following tickets:
> > >
> > > https://issues.apache.org/jira/browse/MESOS-2926 Extend
> > > mesos-style.py/cpplint.py to check #include files
> > > https://issues.apache.org/jira/browse/MESOS-2927 Update mesos #include
> > > headers
> > > https://issues.apache.org/jira/browse/MESOS-2928 Update stout #include
> > > headers
> > > https://issues.apache.org/jira/browse/MESOS-2929 Update libprocess
> > > #include
> > > headers
> > >
> > >
> > > -- Paul Brett
> > >
> >
>
>
>
> --
> -- Paul Brett
>
Re: Project wide updates to #include statements
Posted by Paul Brett <pb...@twitter.com.INVALID>.
Thanks Marco, it would be great to get to a better checker and then turn on
more checks for the things we normally comment on in reviews.
The style guide currently states the following order:
1. dir2/foo2.h.
2. C system files.
3. C++ system files.
4. Other libraries' .h files.
5. Your project's .h files.
Generally speaking,we seem to use the following.
1. dir2/foo2.h - we don't go this
2. C system files.
3. C++ system files.
4. C library files - leveldb
5. C++ library files - boost, glog, gmock, libev, picojson, protobuf,
zookeeper
6. Project's .h files - I think these are all wrapped in .hpp files
7. Project .hpp files - mesos, stout, libprocess
As Kapil has pointed out on MESOS-2927, all the library files should be
identified by '#include <...>' and all the project files should use
'#include "..."' - i don't know yet if we have been consistent about this.
I have created the stout patch on this basis and plan to incorporate any
comments before moving onto libprocess.
-- Paul
On Thu, Jun 25, 2015 at 3:31 PM, Marco Massenzio <ma...@mesosphere.io>
wrote:
> As I mentioned in the review that Paul submitted, I've been working on
> cpplint.py to make it more Mesos-friendly.
> I have also submitted a few Pull Requests
> <https://github.com/google/styleguide/pulls> to the original github repo,
> but got neither love nor attention.
>
> My fork is here: https://github.com/massenz/styleguide
> and the code in the `master` branch (
> https://github.com/massenz/styleguide/tree/master) has all my changes; it
> currently works well with the existing code (in that, submitted, valid
> Mesos code does not raise errors) apart from the opening brace on a newline
> for multi-line method declarations.
>
> Love to get contributions and pull requests folks, feel free to submit!
>
> An example CPPLINT.cfg that works with the code in `master` is something
> like this:
>
> $ cat CPPLINT.cfg
> # Apache Mesos cpplint custom file
>
> extensions=cpp,hpp
> access_keywords_indent=0
> headers=h,hpp
> custom_headers=mesos,process,stout
> set braces_newline
>
> PS - am I the only one to find it hilarious that code that supposedly
> checks on style correctness is written in some of the least readable, badly
> PEP8-violating Python? :)
>
> *Marco Massenzio*
> *Distributed Systems Engineer*
>
> On Thu, Jun 25, 2015 at 3:18 PM, Paul Brett <pb...@twitter.com.invalid>
> wrote:
>
> > The style guide prescribes the order of header file inclusions for the
> > project and requires that we #include or make explicit forward
> declarations
> > for any functions we use, however we were only enforcing this at review
> > time manually and not commit time. I would like to turn on the checks at
> > commit time, so I am in the process of raising changes against stout,
> > libprocess and mesos to bring the code base into compliance. Once this
> is
> > completed, I propose to update cpplint.py and mesos-style.py to enforce
> the
> > style guide.
> >
> > Anyone interested can comment on the following tickets:
> >
> > https://issues.apache.org/jira/browse/MESOS-2926 Extend
> > mesos-style.py/cpplint.py to check #include files
> > https://issues.apache.org/jira/browse/MESOS-2927 Update mesos #include
> > headers
> > https://issues.apache.org/jira/browse/MESOS-2928 Update stout #include
> > headers
> > https://issues.apache.org/jira/browse/MESOS-2929 Update libprocess
> > #include
> > headers
> >
> >
> > -- Paul Brett
> >
>
--
-- Paul Brett
Re: Project wide updates to #include statements
Posted by Marco Massenzio <ma...@mesosphere.io>.
As I mentioned in the review that Paul submitted, I've been working on
cpplint.py to make it more Mesos-friendly.
I have also submitted a few Pull Requests
<https://github.com/google/styleguide/pulls> to the original github repo,
but got neither love nor attention.
My fork is here: https://github.com/massenz/styleguide
and the code in the `master` branch (
https://github.com/massenz/styleguide/tree/master) has all my changes; it
currently works well with the existing code (in that, submitted, valid
Mesos code does not raise errors) apart from the opening brace on a newline
for multi-line method declarations.
Love to get contributions and pull requests folks, feel free to submit!
An example CPPLINT.cfg that works with the code in `master` is something
like this:
$ cat CPPLINT.cfg
# Apache Mesos cpplint custom file
extensions=cpp,hpp
access_keywords_indent=0
headers=h,hpp
custom_headers=mesos,process,stout
set braces_newline
PS - am I the only one to find it hilarious that code that supposedly
checks on style correctness is written in some of the least readable, badly
PEP8-violating Python? :)
*Marco Massenzio*
*Distributed Systems Engineer*
On Thu, Jun 25, 2015 at 3:18 PM, Paul Brett <pb...@twitter.com.invalid>
wrote:
> The style guide prescribes the order of header file inclusions for the
> project and requires that we #include or make explicit forward declarations
> for any functions we use, however we were only enforcing this at review
> time manually and not commit time. I would like to turn on the checks at
> commit time, so I am in the process of raising changes against stout,
> libprocess and mesos to bring the code base into compliance. Once this is
> completed, I propose to update cpplint.py and mesos-style.py to enforce the
> style guide.
>
> Anyone interested can comment on the following tickets:
>
> https://issues.apache.org/jira/browse/MESOS-2926 Extend
> mesos-style.py/cpplint.py to check #include files
> https://issues.apache.org/jira/browse/MESOS-2927 Update mesos #include
> headers
> https://issues.apache.org/jira/browse/MESOS-2928 Update stout #include
> headers
> https://issues.apache.org/jira/browse/MESOS-2929 Update libprocess
> #include
> headers
>
>
> -- Paul Brett
>