Closed Bug 1024993 Opened 10 years ago Closed 10 years ago

Run the CSS Linter in Travis/TBPL

Categories

(Firefox OS Graveyard :: Gaia, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rik, Assigned: rik)

References

Details

Attachments

(3 files, 1 obsolete file)

We need to enforce this at the build bots level.
Vivien: I'll need to update the xfail list, do you have a command handy to generate it?
Flags: needinfo?(21)
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)
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 ?
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)
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)
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)
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+
https://github.com/mozilla-b2g/gaia/commit/53ab81f97b46ac91734e89286a664167e4f5dca2
Status: ASSIGNED → RESOLVED
Closed: 10 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
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.
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)
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
Depends on: 1034565
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)
We do ship this code so it should be linted. Where can I find instructions to submit a patch?
Flags: needinfo?(wilsonpage)
Attached file pull-request (master)
Attachment #8466218 - Flags: review?(anthony)
Flags: needinfo?(wilsonpage)
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)
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.
Wilson, we have the same issue in shared/elements/gaia-icons/style.css now.
Flags: needinfo?(wilsonpage)
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+
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
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: