mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <bbann...@apache.org>
Subject Re: Review Request 71205: Switch commit hooks to pre-commit.
Date Fri, 16 Aug 2019 18:03:50 GMT


> On Aug. 14, 2019, 1:04 p.m., Benno Evers wrote:
> > bootstrap
> > Line 55 (original), 55 (patched)
> > <https://reviews.apache.org/r/71205/diff/1/?file=2158248#file2158248line55>
> >
> >     Copying my comment from slack here so the discussion isn't split over too many
places:
> >     
> >     As we discussed privately yesterday, I think installing this in bootstrap is
a bit problematic because that is also part of the source tarball, used by non-developers
to build mesos w/o even having a git repository. Additionally, I think I'd be not particularly
amused if I cloned a random open-source project and the first thing it does is install a bunch
of stuff in my home directory.
> >     
> >     I'm not sure if it's possible to implement, but imho the ideal workflow would
be something like this:
> >     
> >     ```
> >     $ git commit -m "Foo the bar."
> >     WARNING: Your commit touched a `.cpp` file, but `cpplint` is not installed on
your system. It is highly recommended to install it to avoid embarrassing mistakes.
> >     
> >     You can also run `pre-commit ????` to automatically install a usable version
of `cpplint` in you home directory.
> >     ```
> 
> Benjamin Bannier wrote:
>     Totally agree on the mix of building and dev setup in `bootstrap` is problematic.
Let me break out an `autogen.sh` to be used for setting up a build env, and `boostrap` also
setting up a dev env.
>     
>     We already download packages from the internet from `mesos-style.py` when e.g., setting
up the virtualenv to run node linters. There is however no caching so one needs to potentially
download this again and again after clearing the build dir; with `pre-commit` files are cached
in `$HOME/.cache` and even for different linter versions or configs with the lifetime managed
explicitly with `pre-commit`. I feel this is a superior, more controllable approach than pulling
stuff with weird heuristics (`mesos-style.py` hardcodes a few of these and they tend to perform
more work than necessary).
>     
>     The only dependency of the linters now becomes `pre-commit` (which needs to be clearly
documented) and potentially some Python interpreters or sim. Linters like e.g., eslint are
automatically set up and do not need to be installed by users (in fact we want to control
the exact versions of these tools, so `pre-commit` enforcing that is great). `cpplint` is
a linter present in the source tree and does not need to be installed.

I added a dummy script for `support/mesos-style.py` to ease the transition. In another patch
I'll also split installation of dev tools out of `./bootstrap` to reduce dependencies non-contributors
need to install, and add additional documentation.


- Benjamin


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


On July 30, 2019, 11:01 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71205/
> -----------------------------------------------------------
> 
> (Updated July 30, 2019, 11:01 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9360
>     https://issues.apache.org/jira/browse/MESOS-9360
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch switches commit hooks to be orchestrated by the pre-commit
> tool mirroring the previous linters invoked through git commit
> hooks (orchestrated by `support/mesos-style.py` or standalone hooks).
> 
> Using pre-commit removes the burden of maintaining
> `support/mesos-style.py`, making sure that hooks have the expected
> environment (e.g., Python version, Node installed). Additionally,
> upstream provides a number of additional linters which are not hard to
> add to Mesos' hooks.
> 
> 
> Diffs
> -----
> 
>   .pre-commit-config.yaml PRE-CREATION 
>   bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
> 
> 
> Diff: https://reviews.apache.org/r/71205/diff/2/
> 
> 
> Testing
> -------
> 
> * used successfully for a couple of months
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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