931718 - Multiple XULRunner SDK breaks mochitest-remote

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[:gerard-majax]

Follow up of bug 906316: running ./mach mochitest-remote ends up in:

$ ./mach mochitest-remote layout/base/tests/
From _tests: Kept 9140 existing; Added/updated 0; Removed 0 files and 0 directories.
waiting for system-message-listener-ready...
done
Usage: mach [options]

mach: error: --xre-path '/media/alex/FirefoxOS/Emulator/B2G/gaia/xulrunner-sdk-26/bin' is not a directory

Comment 1

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/B2G/pull/292

Please find attached a link to the Github pull request that addresses this issue.

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 823245 [details] [review]
Link to Github https://github.com/mozilla-b2g/B2G/pull/292

Lgtm, thanks!

Comment 3

[:gerard-majax]

https://github.com/mozilla-b2g/B2G/commit/895fd4ce2b7a48292c3c122b5e2d5bfc55a48928

Comment 4

[Julien Wajsberg [:julienw]]

Question: the B2G repository does not have any branches, and therefore this will now fail for 1.2 and 1.1 and etc.

Therefore I think the patch should first look in xulrunner-sdk-26/xulrunner-sdk if it exists, and then fallback on xulrunner-sdk...

Comment 5

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Question: the B2G repository does not have any branches, and therefore this
> will now fail for 1.2 and 1.1 and etc.
> 
> Therefore I think the patch should first look in
> xulrunner-sdk-26/xulrunner-sdk if it exists, and then fallback on
> xulrunner-sdk...

Then we should backout and I'll fix it in a better way!

Comment 6

[Julien Wajsberg [:julienw]]

As you wish.

I'm reopening this, you can either back out and do a new patch, or do a follow-up patch, I don't really mind (unless someone has a point towards one way or the other).

Comment 7

[:gerard-majax]

Reverted in https://github.com/lissyx/B2G/commit/b470477b175787f1508cd2954978f018355abbdf

Comment 8

[:gerard-majax]

Revert on master https://github.com/mozilla-b2g/B2G/commit/bd473180d67255513783a4fd10a5643bd7a2bd75

Comment 9

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/B2G/pull/294

Please find attached a new pull request that should work on both master and older branches of Gaia.

Comment 10

[Julien Wajsberg [:julienw]]

Reading [1] I wonder if an easy way could be to have a Gaia Makefile target that would just output XULRUNNER_DIRECTORY, so that the python script would just need to run it and get/parse its output ?

And we just have it now ;) "make install-xulrunner-sdk" is actually outputting:

  XULrunner directory: xulrunner-sdk-26/xulrunner-sdk

So might be a good idea to get it like this, and fallback on the old function if the target is inexistant.

"make install-xulrunner-sdk" is actually downloading and unzipping xulrunner if it's not here, so if we don't want this, we might just create a new target that would just output this var?

Andrew, any thought?

[1] https://github.com/lissyx/B2G/blob/edd999da421c44d8cde92978b744b58dfc586d3a/tools/mach_b2g_bootstrap.py#L99-L100

Comment 11

[Andrew Halberstadt [:ahal]]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> Reading [1] I wonder if an easy way could be to have a Gaia Makefile target
> that would just output XULRUNNER_DIRECTORY, so that the python script would
> just need to run it and get/parse its output ?
> 
> And we just have it now ;) "make install-xulrunner-sdk" is actually
> outputting:
> 
>   XULrunner directory: xulrunner-sdk-26/xulrunner-sdk
> 
> So might be a good idea to get it like this, and fallback on the old
> function if the target is inexistant.
> 
> "make install-xulrunner-sdk" is actually downloading and unzipping xulrunner
> if it's not here, so if we don't want this, we might just create a new
> target that would just output this var?
> 
> Andrew, any thought?

That would be the best way forward I think. Does make install-xulrunner-sdk do the download/unzip even if the sdk directory already exists? If so then yes, we'd need a way to get the output without doing the download (a flag to the target might be enough).

Comment 12

[Julien Wajsberg [:julienw]]

Andrew: nope, it's doing nothing if it exists already and is the correct version.

Comment 13

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> Andrew: nope, it's doing nothing if it exists already and is the correct
> version.

Then I just have to issue a make subprocess and if it fails, fallback to the old code. Are you both okay with this ?

Comment 14

[Andrew Halberstadt [:ahal]]

(In reply to Alexandre LISSY :gerard-majax from comment #13)
> (In reply to Julien Wajsberg [:julienw] from comment #12)
> > Andrew: nope, it's doing nothing if it exists already and is the correct
> > version.
> 
> Then I just have to issue a make subprocess and if it fails, fallback to the
> old code. Are you both okay with this ?

I wouldn't mind just letting install-xulrunner-sdk do it's thing in the event that it doesn't exist or needs an update, and removing the old method completely. That's assuming there aren't other ways it can fail.

Comment 15

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #14)
> I wouldn't mind just letting install-xulrunner-sdk do it's thing in the
> event that it doesn't exist or needs an update, and removing the old method
> completely. That's assuming there aren't other ways it can fail.

I should clarify I mean ways it can fail where the old method wouldn't.

Comment 16

[Julien Wajsberg [:julienw]]

Well, it can fail because while "install-xulrunner-sdk" exists for a long time, it doesn't output the location since bug 906316 landed :)

So to support the previous situation (expecially 1.2, 1.1) we unfortunately need to keep the old solution too.

Comment 17

[:gerard-majax]

I personnally don't really like the fact that we might end up running some make step to download the SDK when we just want to evaluate a value ; I'd prefer adding a print-xulrunner-sdk target that just prints the value, and make use of it.

Comment 18

[Julien Wajsberg [:julienw]]

Good for me :) just do it ;)

Comment 19

[:gerard-majax]

Attachment: Adding print-xulrunner-sdk make target in Gaia

Please find attached a link to the github pull request that introduces the print-xulrunner-sdk target. Please keep in mind that this patch must land before the other one.

Comment 20

[James Lal [:lightsofapollo]]

Comment on attachment 824528 [details] [review]
Adding print-xulrunner-sdk make target in Gaia

This looks fine (and works for me) but I really do hate our ever expanding Makefile

Comment 21

[James Lal [:lightsofapollo]]

gaia piece: https://github.com/mozilla-b2g/gaia/commit/1dd8c7d1209cdf23bd8bdfd1878fcdcdc58fb2bc

Comment 22

[:gerard-majax]

Thanks !

Comment 23

[Andrew Halberstadt [:ahal]]

Comment on attachment 824002 [details] [review]
Link to Github https://github.com/mozilla-b2g/B2G/pull/294

This looks good to me, and works locally. For now I'm fine leaving the old method for compatibility with older versions of gaia, but might be nice to leave a TODO comment to delete it once it's no longer necessary.

Comment 24

[:gerard-majax]

(In reply to Andrew Halberstadt [:ahal] from comment #23)
> Comment on attachment 824002 [details] [review]
> Link to Github https://github.com/mozilla-b2g/B2G/pull/294
> 
> This looks good to me, and works locally. For now I'm fine leaving the old
> method for compatibility with older versions of gaia, but might be nice to
> leave a TODO comment to delete it once it's no longer necessary.

Thanks. Julien noticed some enhancement we can apply to the make call, I've updated it.

Comment 25

[:gerard-majax]

fixed once again https://github.com/mozilla-b2g/B2G/commit/2df49e8d2607b973c66c6ffe7d8ade18c79a86c7