Run the CSS Linter in Travis/TBPL

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rik, Assigned: rik)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
We need to enforce this at the build bots level.
(Assignee)

Comment 1

4 years ago
Vivien: I'll need to update the xfail list, do you have a command handy to generate it?
Flags: needinfo?(21)
(Assignee)

Comment 2

4 years ago
IRL, Vivien said he modified the script to output the list. We may want to commit that kind of command in another bug.
Flags: needinfo?(21)
(Assignee)

Comment 3

4 years ago
https://travis-ci.org/mozilla-b2g/gaia/jobs/27487432 The code works as expected but |make csslint| is not returning an error code.

I see that the pre-commit hook uses a quit function https://github.com/mozilla-b2g/gaia/blob/b47b5d1f08dce81fdf16bb3eeebea6ce7e5d225b/tools/pre-commit#L102

But when I use it, I get "ReferenceError: quit is not defined". How can I include this quit function in https://github.com/Rik/gaia/blob/c2a04f555685eb7bde639896a905eb88d1487b44/build/csslint.js#L52 ?
(Assignee)

Comment 4

4 years ago
Alex: Can you help with comment 3?
Flags: needinfo?(poirot.alex)
csslint.js is loaded within a module, that ends up having a different scope and globals than xpcshell scripts.
So that you can't access regular xpcshell functions.
In order to get access to exit (and any other xpcshell specifics), I would do something like this:
+++ b/build/xpcshell-commonjs.js
@@ -10,6 +10,8 @@ let { Loader } = Cu.import(loaderURI, {});
 Cu.import('resource://gre/modules/Services.jsm');
 Cu.import("resource://gre/modules/FileUtils.jsm");
 
+let xpcshellScope = this;
+
 var CommonjsRunner = function(module) {
   const GAIA_DIR = env.get('GAIA_DIR');
   const APP_DIR = env.get('APP_DIR');
@@ -39,7 +41,8 @@ var CommonjsRunner = function(module) {
   let loader = Loader.Loader({
     paths: paths,
     modules: {
-      'toolkit/loader': Loader
+      'toolkit/loader': Loader,
+      'xpcshell-scope': xpcshellScope

And then, in csslint.js: require('xpcshell-scope').quit(42);
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 6

4 years ago
Thanks. Sadly, this is what I get with just your patch, no modification in csslint.js:
Exception: [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: /Users/rik24d/code/gaia/build/xpcshell-commonjs.js :: CommonjsRunner.prototype.run :: line 59"  data: no]

Line 59 is the first one of the try block:
let options = JSON.parse(env.get("BUILD_CONFIG"));
Flags: needinfo?(poirot.alex)
Oops, the SDK module loader is most likely freezing xpcshellScope...
So you would have to do more something like this:

@@ -39,7 +41,8 @@ var CommonjsRunner = function(module) {
   let loader = Loader.Loader({
     paths: paths,
     modules: {
-      'toolkit/loader': Loader
+      'toolkit/loader': Loader,
+      'xpcshell': {quit: xpcshellScope.quit.bind(xpcshellScope)}
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 8

4 years ago
Created attachment 8442020 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20498

The pull request has two commits, you can locally run make csslint on the first commit to see the output in case of failure.

I'll update the list when pushing so that the tree stays green. And I'll send a message to dev.gaia about this new linter.
Assignee: nobody → anthony
Status: NEW → ASSIGNED
Attachment #8442020 - Flags: review?(21)
(Assignee)

Updated

4 years ago
Attachment #8442020 - Flags: review?(21)
Comment on attachment 8442020 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20498

r+ with nits and the use of an explicit |const| instead of 0.
Attachment #8442020 - Flags: review?(21) → review+
(Assignee)

Comment 11

4 years ago
https://github.com/mozilla-b2g/gaia/commit/53ab81f97b46ac91734e89286a664167e4f5dca2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Reverted for being highly suspicious of causing gaia unit test failures on TBPL:
https://github.com/mozilla-b2g/gaia/commit/31d04ec7b1d4a5f27d54d7adf8f1afc203b66411
https://github.com/mozilla-b2g/gaia/commit/ed5231ff7c14eec6c4e7b0944dfb44e5f4da58e6

(This was the only commit in the group which had a related failing gaia-try bustage)


https://tbpl.mozilla.org/?tree=B2g-Inbound&jobname=b2g_ubuntu64_vm%20b2g-inbound%20opt%20test%20gaia-unit&rev=f8589884fd45
Status: RESOLVED → REOPENED
Flags: needinfo?(anthony)
Resolution: FIXED → ---
Confirmed, I'm not sure why - but backing it out appeared to fix the failing unit test.

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=a1d092e62b86
(Assignee)

Comment 14

4 years ago
The error comes from this:
16:39:02     INFO -  21479424871403566742783	addons.xpi	WARN	Exception running bootstrap method startup on httpd@gaiamobile.org: TypeError: xpcshellScope.quit is undefined (resource://gre/modules/addons/XPIProvider.jsm -> file:///tmp/tmpaJvvPT.gaiaunittest/profile/extensions/httpd/bootstrap.js -> file:///builds/slave/test/gaia/build//xpcshell-commonjs.js:45:19) JS Stack trace: CommonjsRunner@xpcshell-commonjs.js:45:20 < require@xpcshell-commonjs.js:91:7 < startupHttpd@bootstrap.js:94:5 < startup@bootstrap.js:458:3 < XPI_callBootstrapMethod@XPIProvider.jsm:628:15 < XPI_startup@XPIProvider.jsm:262:40 < AMI_callProviders@AddonManager.jsm:103:1 < AMI_startup@AddonManager.jsm:86:126 < AMP_startup@AddonManager.jsm:282:168 < AMC_observe@addonManager.js:3:1

I see the error on OS X but somehow the unit tests still work.

So I'm just adding an empty quit function to not break httpd.js.
(Assignee)

Comment 15

4 years ago
Created attachment 8449583 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21300

Asking Alex for review since the only changes from the previous patch are in xpcshell-commons.js
Attachment #8442020 - Attachment is obsolete: true
Attachment #8449583 - Flags: review?(poirot.alex)
Flags: needinfo?(anthony)
(Assignee)

Comment 16

4 years ago
I just pushed a new version with your comment addressed. Let's see what Try has to say.

https://tbpl.mozilla.org/?rev=51f7f2ac39b58002edec84919f07bf1b6995a865&tree=Gaia-Try
(Assignee)

Updated

4 years ago
Depends on: 1034565
(Assignee)

Comment 17

4 years ago
https://github.com/mozilla-b2g/gaia/blob/5011cfbdc4d82e2e01528655f660c72b2808b8e5/apps/camera/bower_components/gaia-icons/style.css#L26
This line is considered a parsing error on platform others than Linux. That means we will have a green TBPL but an always red csslint when running on developers' Macs.

Wilson: Can we get rid of this line?
Flags: needinfo?(wilsonpage)
rik: Yeah don't think that line is needed. Although CSSLint should not really be linting 'third-party' code inside bower_components/. Can we black-list bower_components/** ?
Flags: needinfo?(wilsonpage)
(Assignee)

Comment 19

4 years ago
We do ship this code so it should be linted. Where can I find instructions to submit a patch?
Flags: needinfo?(wilsonpage)
Created attachment 8466218 [details] [review]
pull-request (master)
Attachment #8466218 - Flags: review?(anthony)
Flags: needinfo?(wilsonpage)
(Assignee)

Comment 21

4 years ago
Comment on attachment 8466218 [details] [review]
pull-request (master)

I'm not a Camera peer so redirecting.
Attachment #8466218 - Flags: review?(anthony) → review?(dflanagan)
Comment on attachment 8466218 [details] [review]
pull-request (master)

djf is pretty overloaded with reviews. Redirecting to dmarcos who is also more familiar with Camera.
Attachment #8466218 - Flags: review?(dflanagan) → review?(dmarcos)
Attachment #8466218 - Flags: review?(dmarcos) → review+
Comment on attachment 8449583 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21300

Li is red and looks broken.
Please reflag for review once the patch is ready.
Attachment #8449583 - Flags: review?(poirot.alex)
(Assignee)

Comment 25

4 years ago
Thanks Wilson for the quick fix. I've pushed another try: https://tbpl.mozilla.org/?rev=4414e97d27d3723e07127147faac097b3da7ce0c&tree=Gaia-Try. It is green on my computer, let's see what TBPL says.
(Assignee)

Comment 26

4 years ago
Wilson, we have the same issue in shared/elements/gaia-icons/style.css now.
Flags: needinfo?(wilsonpage)
Created attachment 8468481 [details] [review]
pull-request (master) - shared

dmarcos: I needed to update gaia-icons inside shared/elements/ too so that CSSLint doesn't choke on that same line. Ping me if you have any questions.
Attachment #8468481 - Flags: review?(dmarcos)
Flags: needinfo?(wilsonpage)
Attachment #8468481 - Flags: review?(dmarcos) → review+
Blocks: 1015248
Blocks: 1016816
Landed 'pull-request (master) - shared' on 'master'

https://github.com/mozilla-b2g/gaia/commit/147e0922d05c1c1f74c559475499bc1c017e7fcb
No longer blocks: 1016816
No longer blocks: 1015248
Comment on attachment 8449583 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21300

Looks good, thanks!

Note that I'm seeing these errors when running:
$ make csslint
run-js-command gaia/csslint
Parsing errors in /home/alex/gaia/apps/pdfjs/content/web/viewer.css
	Expected end of value but found 'no-repeat'.  Error in parsing value for 'background-size'.  Declaration dropped. line: 227 column: 29 source: "  background-size: 100% 100% no-repeat;"
Parsing errors in /home/alex/gaia/shared/style/drawer.css
	Error in parsing value for 'padding-right'.  Declaration dropped. line: 253 column: 17 source: "  padding-right: auto;"
Attachment #8449583 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 31

4 years ago
The script always outputs parser errors, whether they are in the blacklist or not. 

Landed with the nit addressed. Hope it sticks this time!
https://github.com/mozilla-b2g/gaia/commit/ab51d7db756a554b8d03c90c299f9ecba6ce6467
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.