Closed
Bug 1024993
Opened 10 years ago
Closed 10 years ago
Run the CSS Linter in Travis/TBPL
Categories
(Firefox OS Graveyard :: Gaia, defect)
Firefox OS Graveyard
Gaia
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.
Assignee | ||
Comment 1•10 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•10 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•10 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 ?
Comment 5•10 years ago
|
||
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•10 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)
Comment 7•10 years ago
|
||
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•10 years ago
|
||
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.
Comment 9•10 years ago
|
||
Comment on attachment 8442020 [details] https://github.com/mozilla-b2g/gaia/pull/20498 Close but I would like to see a new version.
Attachment #8442020 -
Flags: review?(21)
Assignee | ||
Updated•10 years ago
|
Attachment #8442020 -
Flags: review?(21)
Comment 10•10 years ago
|
||
Comment on attachment 8442020 [details] 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•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/53ab81f97b46ac91734e89286a664167e4f5dca2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 12•10 years ago
|
||
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 → ---
Comment 13•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 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 | ||
Comment 17•10 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)
Comment 18•10 years ago
|
||
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•10 years ago
|
||
We do ship this code so it should be linted. Where can I find instructions to submit a patch?
Flags: needinfo?(wilsonpage)
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 8466218 [details]
pull-request (master)
I'm not a Camera peer so redirecting.
Attachment #8466218 -
Flags: review?(anthony) → review?(dflanagan)
Comment 22•10 years ago
|
||
Comment on attachment 8466218 [details]
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)
Updated•10 years ago
|
Attachment #8466218 -
Flags: review?(dmarcos) → review+
Comment 23•10 years ago
|
||
Landed Camera fix on 'master' https://github.com/mozilla-b2g/gaia/commit/0e0131f7032f051635b39828a42f1cc8e6792218
Comment 24•10 years ago
|
||
Comment on attachment 8449583 [details] 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•10 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•10 years ago
|
||
Wilson, we have the same issue in shared/elements/gaia-icons/style.css now.
Flags: needinfo?(wilsonpage)
Comment 27•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8468481 -
Flags: review?(dmarcos) → review+
Comment 28•10 years ago
|
||
Landed 'pull-request (master) - shared' on 'master' https://github.com/mozilla-b2g/gaia/commit/147e0922d05c1c1f74c559475499bc1c017e7fcb
No longer blocks: 1016816
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 8449583 [details] https://github.com/mozilla-b2g/gaia/pull/21300 Green Try ! https://tbpl.mozilla.org/?rev=c0aaa215addd4bcab1d6140bc2b815ee3a4af7e7&tree=Gaia-Try
Attachment #8449583 -
Flags: review?(poirot.alex)
Comment 30•10 years ago
|
||
Comment on attachment 8449583 [details] 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•10 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
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•