Hi Benno, As I added the new JavaScript linter to support/mesos-style.py, let me provide some details regarding its implementation.
"Indeed, it turns out that the pre-commit hook is installing both a pip package and an npm package, eslint, along with all of its 1450 dependencies. (https://pastebin.com/hTZWRxcy)" We chose ESLint due to its flexible CLI and good documentation, it provides us an output going well with the two other linters we use for C++ and Python and a linting that we have adapted to work with our JS codebase. Concerning the metric you used: JSHint, which is the other JS linter allowing customization from the collection https://github.com/collections/clean-code-linters, has 972 dependencies (number obtained using npm-remote-ls); this is a smaller number but still in the same league. There is no equivalent to cpplint.py for JS, we had to use a Node.js package (in that case a npm package). Node.js packages require node, we install it from a new Python virtual environment created for the linters (the Mesos Python linter also requires some pip packages) using nodeenv (https://github.com/ekalinin/nodeenv). This was a simple solution to install eslint and it explains the chain of dependencies you described (pip package => npm package => JS dependencies). "My proposal would be to move most checks to post-reviews.py, which is a slow operation anyways and which actually marks the point in time where the commits should be cleaned up enough to pass all checks." I personally agree and this is feasible, please create a ticket regarding that request and assign it to me. Comments from other devs would be useful to know if this would be a positive change or not. One last point regarding the length of the installation of this new linter due to its dependencies: the two Python virtual environments we use in Apache Mesos (one for the CLI, one for the linters) are only being rebuilt when we update the pip requirements or update the virtual environment installation scripts ( https://github.com/apache/mesos/blob/ab9039335723fc36cb8e8b4fbb17a15327520cc9/support/mesos-style.py#L406). This is a not a common event as you can see from the history of the CLI pip-requirements.txt: https://github.com/apache/mesos/commits/master/src/python/cli_new/pip-requirements.txt and https://github.com/apache/mesos/commits/2d19111e4852aed25161e4549ff704f9d4c2f37b/src/cli_new/pip-requirements.txt . Best regards, 2017-11-10 16:28 GMT+01:00 Benno Evers <bev...@mesosphere.com>: > Hi guys, > > Our commit hooks in mesos have been traditionally quite strict (something > many people here will have experienced when "quickly" commiting some work > in progress before changing branches). However, when I rebased a branch > today I was surprised to learn that I had to wait more than 5 minutes while > something was downloaded from the internet. > > Indeed, it turns out that the pre-commit hook is installing both a pip > package and an npm package, eslint, along with all of its 1450 > dependencies. (https://pastebin.com/hTZWRxcy) > > Aside from the security implications (these are unsigned, and every single > one gets to execute custom javascript) I feel like there is something > fundamentally wrong here - a git commit is supposed to be a local > operation, it should not need internet access: > > bevers@poincare:~/mesos$ git commit -m "Dummy commit on cleaned > worktree." > Virtualenv not detected... building > Rebuilding virtualenv... > Retrying (Retry(total=4, connect=None, read=None, redirect=None)) after > connection broken by > 'NewConnectionError('<pip._vendor.requests.packages.urllib3.connection. > VerifiedHTTPSConnection > object at 0x7fea82eb11d0>: Failed to establish a new connection: [Errno -2] > Name or service not known',)': /simple/pip/ > [...] > Retrying (Retry(total=0, connect=None, read=None, redirect=None)) after > connection broken by > 'NewConnectionError('<pip._vendor.requests.packages.urllib3.connection. > VerifiedHTTPSConnection > object at 0x7f2e627456d0>: Failed to establish a new connection: [Errno -2] > Name or service not known',)': /simple/nodeenv/ > Could not find a version that satisfies the requirement nodeenv==1.1.2 > (from -r /home/bevers/mesos/support/pip-requirements.txt (line 1)) (from > versions: ) > No matching distribution found for nodeenv==1.1.2 (from -r > /home/bevers/mesos/support/pip-requirements.txt (line 1)) > > > I'm sure running various linters improves code quality, but there must be a > better way to implement this. My proposal would be to move most checks to > post-reviews.py, which is a slow operation anyways and which actually marks > the point in time where the commits should be cleaned up enough to pass all > checks. > > What do you think? > > Best regards, > -- > Benno Evers > Software Engineer, Mesosphere > -- Armand Grillet Software Engineer, Mesosphere
