885713 - propose a "make hint" that would use jshint instead of gjslint

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Julien Wajsberg [:julienw]]

Goal of this bug:

* provide a .jshintrc at the top of the gaia directory, with the same setup that what we're used to with gjslint
* provide a new "hint" goal in the makefile

Long term goal is to enable more checks.

Advantages: jshint is more "open source" than gjslint, should be easier to request features and provide patches.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

jshint seems to run significantly faster than gjslint as well.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: wip patch

I just tried on the SMS codebase and it doesn't pass cleanly.

I can't disable the "undef" check, maybe there is a bug in jshint. Anyway we should probably define the globals used in our various files in jshint directives, but this is a huge task.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> I can't disable the "undef" check, maybe there is a bug in jshint. Anyway we
> should probably define the globals used in our various files in jshint
> directives, but this is a huge task.

I guess that's one advantage of using a secondary make target.  It would let modules adjust to jshint over time.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

It appears strict implies undef:

  https://github.com/jshint/jshint/issues/1033#issuecomment-17149137

Comment 5

[Julien Wajsberg [:julienw]]

(In reply to Ben Kelly [:bkelly] from comment #3)

> 
> I guess that's one advantage of using a secondary make target.  It would let
> modules adjust to jshint over time.

You're right, then I'll turn on/off some directives I think are useful.

We'll need to file bugs to adjust to jshint after this.

Thanks !

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: patch v1

see also PR at https://github.com/mozilla-b2g/gaia/pull/12248

This adds
* a default .jshintrc configuration file
* a `make hint` launcher
* a way to launch the linters (including the old lint linter) for a specific app
  only.
* you can also use another jshint configuration file
---
 .jshintrc                      |   43 ++++++++++++++++++++++++++++++++++++++++
 Makefile                       |   33 +++++++++++++++++++++++++++++-
 build/lint-excluded-dirs.list  |   10 +++++++++-
 build/lint-excluded-files.list |   10 +++++++++-
 4 files changed, 93 insertions(+), 3 deletions(-)
 create mode 100644 .jshintrc

Asking review from Rik because he did the original linter, Alexandre as owner, and asking feedback from Ben because he was interested in this too.

Comment 7

[Julien Wajsberg [:julienw]]

So, I did the config file quite strict, but the rules can be relaxed using comment directives in the files.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 805392 [details] [diff] [review]
patch v1

will push a new patch thanks to Ben Kelly's feedback

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: patch v2

PR is at https://github.com/mozilla-b2g/gaia/pull/12248: I added one commit to show the diff relative to the first patch.

Follow-ups:
    
* use npm to install jshint
* simplify the jshint command line, now that I checked that current jshint does
  not choke on our files
* use .jshintignore file as main exclude file, from both gjslint and jshint

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 806671 [details] [diff] [review]
patch v2

Review of attachment 806671 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good for me.

Comment 11

[Anthony Ricaud (:rik)]

Comment on attachment 806671 [details] [diff] [review]
patch v2

Review of attachment 806671 [details] [diff] [review]:
-----------------------------------------------------------------

tools/pre-commit is also using lint-excluded-[dirs|files].list. Please reflect the changes there too.

It's also not working on my mac. Apparently a problem with the paste tool.
"usage: paste [-s] [-d delimiters] file ..."

Comment 12

[James Lal [:lightsofapollo]]

Comment on attachment 806671 [details] [diff] [review]
patch v2

looks good to me on the hint side. I see the same bug as :Rik though |make lint| is busted.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 806671 [details] [diff] [review]
patch v2

Review of attachment 806671 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.  Obviously the breakage on mac needs to be fixed, though.

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: patch v3

Follow-ups:
    
    * fix paste call in MacOS X
    * pre-commit change
    * remove node_modules from .PHONY because I don't want to always run this,
      especially when I'm offline
    * don't fail the build if the call to npm install fails
    * remove the node_modules directory in really_clean

The PR is updated at https://github.com/mozilla-b2g/gaia/pull/12248 with an additional commit containing these follow-ups.

Comment 15

[Anthony Ricaud (:rik)]

Comment on attachment 807698 [details] [diff] [review]
patch v3

Review of attachment 807698 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me as intended.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 807698 [details] [diff] [review]
patch v3

Review of attachment 807698 [details] [diff] [review]:
-----------------------------------------------------------------

Some cosmetic suggestions.
Looks good, but ensure preventing the lint in precommit if no js file has been modified.

::: Makefile
@@ +777,5 @@
> +  JSHINTED_PATH = apps/$(APP)
> +else
> +  JSHINTED_PATH = apps shared
> +endif
> +endif

We already faced some conflicts with too generic env variable name. We may face the same issue with FILES.

What about merging FILES, JSHINTED_PATH (and GJSLINTED_PATH)?
That would even more comprehensible at the end:
ifndef LINTED_PATH
ifdef APP
  LINTED_PATH = ...
else
  LINTED_PATH = ...
endif
endif
...
    ./node_modules/.bin/jshint $(JSHINT_ARGS) $(LINTED_PATH)

In order to handle the different schemes for gjslint (need -r for folders), you can define different default LINTED_PATH in each rule: hint and lint.

::: tools/pre-commit
@@ +7,5 @@
>  then
>    exit 0
>  fi
>  
> +hash gjslint > /dev/null 2>&1

Is there anything wrong with &> on Mac?

@@ +13,5 @@
>  then
>    echo >&2 "You should install gjslint to lint your patch"
>    echo >&2 "https://developers.google.com/closure/utilities/docs/linter_howto"
>    exit 0
>  fi

Ideally, makefile would do these checks (.jshintignore exists and gjslint available), so that pre-commit would be just a make lint alias with the additional `git diff` argument.

@@ +20,2 @@
>  
> +make lint FILES="$diff_with_exclude"

nit: even if make interprets env variable on the right, it feels more natural to keep then defined on the left.

non-nit: we shouldn't lint if there is no JS file being modified, otherwise we would do a full lint that takes a while and is unecessary.

Comment 17

[Julien Wajsberg [:julienw]]

(In reply to Alexandre Poirot (:ochameau) from comment #16)
> 
> ::: Makefile
> @@ +777,5 @@
> > +  JSHINTED_PATH = apps/$(APP)
> > +else
> > +  JSHINTED_PATH = apps shared
> > +endif
> > +endif
> 
> We already faced some conflicts with too generic env variable name. We may
> face the same issue with FILES.
> 
> What about merging FILES, JSHINTED_PATH (and GJSLINTED_PATH)?
> That would even more comprehensible at the end:
> ifndef LINTED_PATH
> ifdef APP
>   LINTED_PATH = ...
> else
>   LINTED_PATH = ...
> endif
> endif
> ...
>     ./node_modules/.bin/jshint $(JSHINT_ARGS) $(LINTED_PATH)
> 
> In order to handle the different schemes for gjslint (need -r for folders),
> you can define different default LINTED_PATH in each rule: hint and lint.

yep, will try to do better.

> 
> ::: tools/pre-commit
> @@ +7,5 @@
> >  then
> >    exit 0
> >  fi
> >  
> > +hash gjslint > /dev/null 2>&1
> 
> Is there anything wrong with &> on Mac?

&> is not POSIX and for such a tool I'd like to be as posix-compliant as possible.

> 
> @@ +13,5 @@
> >  then
> >    echo >&2 "You should install gjslint to lint your patch"
> >    echo >&2 "https://developers.google.com/closure/utilities/docs/linter_howto"
> >    exit 0
> >  fi
> 
> Ideally, makefile would do these checks (.jshintignore exists and gjslint
> available), so that pre-commit would be just a make lint alias with the
> additional `git diff` argument.

The problem is that we don't want to fail the commit if gjslint is not installed, whereas we'd want to fail the make in the same situation.

> 
> @@ +20,2 @@
> >  
> > +make lint FILES="$diff_with_exclude"
> 
> nit: even if make interprets env variable on the right, it feels more
> natural to keep then defined on the left.

I'm really not sure this is more natural.

Actually, they are really 2 different ways of interpreting variables.
When defining them on the right, it's an overriding variable: it will override any variable definition in the Makefile.
When defining them on the left, it's an environment variable, and any non-conditional variable definition in the Makefile will have priority.

So this is not a cosmetic change, this is not a matter of natural/unnatural, these are 2 different things.

In that specific case, I can do what you suggest though.

> 
> non-nit: we shouldn't lint if there is no JS file being modified, otherwise
> we would do a full lint that takes a while and is unecessary.

will check this.

Comment 18

[Julien Wajsberg [:julienw]]

Attachment: patch v4

hopefully the last patch ;)

carrying r=rik,r=ochameau

Follow-ups:

* use real files instead of phony targets for node modules and git hooks
* don't execute make lint in the commit hook if we have no js files
* make make quieter in the commit hook
* use target-only variables for the lint target

The github PR is updated at https://github.com/mozilla-b2g/gaia/pull/12248, with a specific commit for these follow-ups at https://github.com/julienw/gaia/commit/ad9d88eebffc39b04a648dfc2dbd6d3bb92d238b

Comment 19

[Julien Wajsberg [:julienw]]

master: 3ec823d0594a53090c39cb0b865257bb34262da0

Comment 20

[Julien Wajsberg [:julienw]]

Attachment: patch v5

carrying r=ochameau

This is the final patch. I realized the previous commit hook was not working when we were committing several patches, which was quite unfortunate ;-)

The long story is that the LINTED_FILES environment variable contained the files separated by new lines, and when we were replacing that variable in the Makefile, make was really replacing it litteraly, which means only the first file was the argument to gjslint, and the second file was on a new line, and therefore make was trying to execute it as a program.

So now I'm replacing all space-like characters by a real space character in the commit hook, and this works like a charm.

After discussing with Yuren, I'm also reverting the part of bug 918295 affecting the git hook copy, because we want it copied as soon as it changes.