You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2019/08/14 11:04:18 UTC
Re: Review Request 71205: Switch commit hooks to pre-commit.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71205/#review217200
-----------------------------------------------------------
bootstrap
Line 55 (original), 55 (patched)
<https://reviews.apache.org/r/71205/#comment304493>
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.
```
- Benno Evers
On July 30, 2019, 9: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, 9: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/1/
>
>
> Testing
> -------
>
> * used successfully for a couple of months
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 71205: Switch commit hooks to pre-commit.
Posted by Benjamin Bannier <bb...@apache.org>.
> 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.
> > ```
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.
- 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/1/
>
>
> Testing
> -------
>
> * used successfully for a couple of months
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 71205: Switch commit hooks to pre-commit.
Posted by Benjamin Bannier <bb...@apache.org>.
> 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.
>
> Benjamin Bannier wrote:
> 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.
Broke installation of dev tools out into https://reviews.apache.org/r/71299/; transitional `./support/mesos-style.py` is removed with https://reviews.apache.org/r/71299/.
- 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
>
>
Re: Review Request 71205: Switch commit hooks to pre-commit.
Posted by Benjamin Bannier <bb...@apache.org>.
> 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
>
>