976967 - sdk/system module issue: the system.architecture and system.compiler return an invalid value

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[bgabrhelik]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.117 Safari/537.36

Steps to reproduce:

1) used folloving code in Add-on's main.js
{code}
  let {Cu, Cc, Ci}  = require("chrome");
  let console = (Cu.import("resource://gre/modules/devtools/Console.jsm", {})).console;
  let system = require("sdk/system");
  let runtime = require("sdk/system/runtime");

  console.log("system.architecture:" + system.architecture);
  console.log("system.compiler:" + system.compiler);
  console.log("runtime.XPCOMABI:" + runtime.XPCOMABI);

{code}



Actual results:

Following invalid output is reported (all results are executed on Windows x64 platform)

**Mozilla Firefox x86 (running under Wow64)**
console.log: system.architecture:x86-msvc
console.log: system.compiler:undefined
console.log: runtime.XPCOMABI:x86-msvc

*Mozilla Firefox x64*
console.log: system.architecture:x86
console.log: system.compiler:64-msvc
console.log: runtime.XPCOMABI:x86_64-msvc


Expected results:

*Mozilla Firefox x86*
console.log: system.architecture:x86
console.log: system.compiler:msvc
console.log: runtime.XPCOMABI:x86-msvc

*Mozilla Firefox x64*
console.log: system.architecture:x86_64
console.log: system.compiler:msvc
console.log: runtime.XPCOMABI:x86_64-msvc

The problem is that the separator dash ("-") should be used instead of underscore ("_") separator when values are extracted from runtime.XPCOMABI.

You could think that this problem is not so serious as javascript code should be portable and shouldn't depend on host system architecture at all, however I need some way to load proper dynamic library according to architecture by the js-types module.

Comment 1

[bgabrhelik]

As it is signed as a "good first bug" I can fix it. However I am not sure how to write a JS unit test for it, so if it is needed I will need some assistance or doc. I will get to it in one or two weeks. Can be used Github clone & pull request for fix? I am asking because I am not sure what is the primary repository as I found Hg Mozilla central repo and Github.
Thanks.

Comment 2

[Tomislav Jovanovic :zombie]

there might not be a great way to unit-test this. it is also fairly simple, so extensive unit-testing might not be necessary. you can do some sanity type checks similar to:

https://github.com/mozilla/addon-sdk/blob/master/test/test-system-runtime.js

and maybe do a few manual test for some known XPCOMABI values, like, if XPCOMABI equals "x86_64-msvc", assert that .compiler is equal to "msvc".

contributor guide is on the wiki, but in short, do a PR on github and link to it here as attachement, and flag (me) for review.

https://github.com/mozilla/addon-sdk/wiki/contribute

Comment 3

[bgabrhelik]

Attachment: cfx testpkgs doesn't work for me

I started to work on this. I have also created a test test\test-system.js however cfx testpkgs command doesn't work for me. What I am doing wrong. Any advice?

Thanks,
-bg

Environment: OS X 10.7.6, FF 28

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to bgabrhelik from comment #3)

> I started to work on this. I have also created a test test\test-system.js
> however cfx testpkgs command doesn't work for me. What I am doing wrong. Any
> advice?

Download the latest Firefox Nightly, and then execute: `cfx test -b <binaryofnightly> -o`. That would run all the tests. If you want filter only test-system: `cfx test -b <binaryofnightly> -o -f system`.

So, assuming your Firefox Nightly is under Applications, you will have `cfx test -b /Applications/FirefoxNightly.app -o`.

Comment 5

[Tomislav Jovanovic :zombie]

also, if you haven't already, fork/grab the master branch of addon-sdk from github, as that one is meant to work with Nightly, and that's the one you will be issuing pool requests to.

Comment 6

[bgabrhelik]

Thanks to all for detailed description.

I have created the pull request #1439:

Bug 976967 - sdk/system module issue: the system.architecture and system.compiler return an invalid value #1439

Comment 7

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1439

when submitting pull request, you should copy/paste the github PR url into the attachment, and flag for review?

Comment 8

[bgabrhelik]

OK, I am sorry. Tomislav, thanks for taking of actions instead of me.

Comment 9

[Tomislav Jovanovic :zombie]

Comment on attachment 8395132 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1439

this looks good, except for the part where compiler string can contain a dash.

also, check out other style nit-picks in the comments on github, and request review when you are done (under "details" of this same attachment).

Comment 10

[bgabrhelik]

Attachment: addressed code review issues #1 from pull request #1439

- adding request to re-review

Comment 11

[bgabrhelik]

Attachment: addressed code review issues #2 from pull request #1439

Comment 12

[Tomislav Jovanovic :zombie]

Comment on attachment 8395432 [details] [review]
addressed code review issues #1 from pull request #1439

it's better to re-request r? on the same attachment (under details) when you update the PR.

Comment 13

[Tomislav Jovanovic :zombie]

Comment on attachment 8395992 [details] [review]
addressed code review issues #2 from pull request #1439

good work!

thanks, and sorry for all the nit-picks, but that's how we (try to) keep our code clean and readable. 

last thing to do is to squash all your commits into one (with git rebase) so that it can be merged into the master branch as one single contribution.

Comment 14

[bgabrhelik]

Comment on attachment 8395992 [details] [review]
addressed code review issues #2 from pull request #1439

I have joined commits together. I am not sure which flag to set, so I set review again. I sorry if it is not correct.

Comment 15

[Tomislav Jovanovic :zombie]

Comment on attachment 8395992 [details] [review]
addressed code review issues #2 from pull request #1439

cool, thanks.

i'll merge this in a day or two (once i have my commit access sorted out), if you are not in a hurry.

in the meantime, you can search for other bugs with "good first bug" flag [1], if you wish to continue contributing..

and feel free to ask if you have any questions!

[1] https://bugzilla.mozilla.org/buglist.cgi?list_id=9786500&resolution=---&resolution=DUPLICATE&status_whiteboard_type=casesubstring&query_format=advanced&status_whiteboard=[good%20first%20bug]&product=Add-on%20SDK

Comment 16

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c0ca16d2cd4df982d65b5f6249d7898b3de8ed53
Bug 976967 - sdk/system module issue: the system.architecture and system.compiler return an invalid value

https://github.com/mozilla/addon-sdk/commit/5e417cb1190b09a60c44fdc33e8cc7179490e058
Merge pull request #1439 from bgabrhelik/master

Bug 976967 - sdk/system module issue: architecture and compiler return invalid values, r=@zombie