Re: Review Request 71205: Switch commit hooks to pre-commit.

[Benjamin Bannier]

> On Aug. 14, 2019, 1:04 p.m., Benno Evers wrote:
> > bootstrap
> > Line 55 (original), 55 (patched)
> > 
> >
> > 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.

[Benjamin Bannier]

> On Aug. 14, 2019, 1:04 p.m., Benno Evers wrote:
> > bootstrap
> > Line 55 (original), 55 (patched)
> > 
> >
> > 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
> 
>

Re: Review Request 71205: Switch commit hooks to pre-commit.

[Benjamin Bannier]

> On Aug. 14, 2019, 1:04 p.m., Benno Evers wrote:
> > bootstrap
> > Line 55 (original), 55 (patched)
> > 
> >
> > 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.

[Benno Evers]

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




bootstrap
Line 55 (original), 55 (patched)


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
> 
>

Review Request 71205: Switch commit hooks to pre-commit.

[Benjamin Bannier]

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

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