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
