Closed Bug 906316 Opened 11 years ago Closed 11 years ago

Don't download the xulrunner SDK each time we change a branch if their configuration are different

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(b2g-v1.2 fixed)

RESOLVED FIXED
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(2 files, 7 obsolete files)

Currently, we have 2 different XULRunner on v1-train and master. This makes it painful when we switch branches, as we need to redownload the XULrunner SDK each time, because the previous one is deleted.

In this bug we want to store different XULRunner SDK versions in different directories, to avoid redownloading the SDK when it's unnecessary.
Attached patch patch v1 (obsolete) — Splinter Review
This also shows a message when the archive file seems to be corrupted, advising
to run "make really-clean". Also changed "make really-clean" to remove all
xulrunner related files.

We now uncompress the archive directly in a specific directory that should
change when we change the URL.

We use a dot file inside the xulrunner SDK directory to keep the URL used to
download that SDK.

I tried to do a migration path (if we have a old xulrunner-sdk directory and .xulrunner-url contains the current url, then rename the directory instead of downloading) but it made the Makefile quite unreadable at the end (because it involved having nearly all the code inside a bash if block, which means we need to end each line with "; \"), and I thought it was not worth the added complexity.

I made a Pull Request at https://github.com/mozilla-b2g/gaia/pull/11585 so that we can see if this breaks Travis (well, Travis is quite broken now because of other things...)
---
 Makefile |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)
Attachment #791697 - Flags: review?(poirot.alex)
Comment on attachment 791697 [details] [diff] [review]
patch v1

Yuren, as Alex is busy this week, I'd be happy if you can review this :)
Attachment #791697 - Flags: review?(yurenju.mozilla)
Comment on attachment 791697 [details] [diff] [review]
patch v1

Review of attachment 791697 [details] [diff] [review]:
-----------------------------------------------------------------

Julien, the patch looks good for me, but I would like to review on v1-train at the same time to make sure everything is okay when I switch branches between master & v1-train. so please set r? to me again when you attach patch on v1-train, thanks!
Attachment #791697 - Flags: review?(yurenju.mozilla)
Yuren, my plan was to actually not land anything on v1-train. Just keep it like it is on v1-train, but on master do something different.

In the future, we'll have different directories for different versions, but right now we'll have the old directory for v1-train and the new one for master.

If you feel that we need a v1-train patch then I can do one but I thought it was not necessary.

Please tell me !
Comment on attachment 791697 [details] [diff] [review]
patch v1

Review of attachment 791697 [details] [diff] [review]:
-----------------------------------------------------------------

sound resaonable, r=yurenju

re-download xulrunner is super annoying, thanks for resolving it!
Attachment #791697 - Flags: review+
master: 726777cbe7dc4f3a3731fcf4629bea56c352a603

thanks a lot Yuren !
Attached patch follow-upSplinter Review
This adds the new .xulrunner-sdk* directories to .gitignore.

I chose to not remove the old one (along with .xulrunner-url) as we still can have them if we use the old branches.
Attachment #793963 - Flags: review?(etienne)
Attachment #791697 - Flags: review?(poirot.alex)
Comment on attachment 793963 [details] [diff] [review]
follow-up

Review of attachment 793963 [details] [diff] [review]:
-----------------------------------------------------------------

r=me !
Attachment #793963 - Flags: review?(etienne) → review+
follow-up master: c088c47414937d615a94c45fab3e8842a2f4edc0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 908367
Depends on: 908984
Depends on: 908580
Depends on: 909190
See Also: 909190
Depends on: 908698
master: 2a3135de3fdc6847661d866ca26d9b45de9fe378, reverted 726777cbe7dc4f3a3731fcf4629bea56c352a603

Note: I haven't reverted the follow-up as it does no harm.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So, yuren, how about doing only one patch with all the follow-up patches ?

Should be easier to test and re-land.
Attached patch consolidated patch v2 (obsolete) — Splinter Review
Yuren, this is the patch made with all other patches.

I'll test it on various environment before asking review again on it. Please do the same on your side !

Especially we must pay attention that the email Makefile is ran correctly. I added the "build directory" on the "rm" line in the "make clean" goal, so running "make clean" should trigger a new email build at the next profile generation.

This works for me on Linux, now we must test on Windows' TBPL environment.

I've made a pull request there: https://github.com/mozilla-b2g/gaia/pull/11812
Attachment #791697 - Attachment is obsolete: true
Attachment #796664 - Flags: feedback?(yurenju.mozilla)
Note: the revert was for bug 908691
Attached patch consolidated patch v3 (obsolete) — Splinter Review
Changes from the previous patch:
* use && instead of ; between commands, as we discussed on IRC
* use $TEMP and $RANDOM to create a temporary directory on Windows, as `mktemp` is not in TBPL environment. (untested for now, that's why I don't ask for a review)

untested on Mac OS too.
Attachment #796664 - Attachment is obsolete: true
Attachment #796664 - Flags: feedback?(yurenju.mozilla)
Attachment #797122 - Flags: feedback?(yurenju.mozilla)
Attached patch consalidated patch v4 (obsolete) — Splinter Review
changes:
* use cp+rm instead of mv because of failures in some environments (especially mine, I think that happens when /tmp and the gaia directory are actually in different partitions)
* unzip is quieter
* added progress messages
* fail early if a submake fails

The email submake doesn't get the variables on Windows currently.

note: the PR is still uptodate at https://github.com/mozilla-b2g/gaia/pull/11812
Attachment #797122 - Attachment is obsolete: true
Attachment #797122 - Flags: feedback?(yurenju.mozilla)
Attachment #797135 - Flags: feedback?(yurenju.mozilla)
New idea that arised from a discussion with :ochameau: let's just uncompress xulrunner inside xulrunner-sdk-26; then we'll have "xulrunner-sdk-26/xulrunner-sdk" as base directory for Xulrunner. Should work very easily in all systems without too much problems.

Will work out a patch based on this idea. Will not resolve the email submake, but this will make things easier to understand.
Attachment #797135 - Flags: feedback?(yurenju.mozilla)
just noticed other places where the "xulrunner-sdk" path is assumed:

* tools/xpcwindow/bin/xpcwindow
* tests/js/bin/runner
Attached patch patch v5 (obsolete) — Splinter Review
Just wanted to show progress.

I changed how it works: now we unzip/untar the XULrunner archive inside the versioned directory, which makes the commands easier, including on Windows.

This works for me on Linux, didn't try on MacOS X, and I think the variable export still doesn't work on Windows Make.

Still same PR at https://github.com/mozilla-b2g/gaia/pull/11812
Attachment #797135 - Attachment is obsolete: true
|export| should work on mingw. I used it to generate config module and it works well.

https://github.com/mozilla-b2g/gaia/blob/master/Makefile#L289-L323
The problem was that the var that I was testing in the submake is empty on windows. yay.

Looks good now, tested on linux/mac/windows, preparing an updated patch.
Attached patch patch v6 (obsolete) — Splinter Review
I guess we gonna need several pair of eyes.

Asking review from the module owner Alexandre, Yuren, and also James so that he  can check that the integration tests are still working (I checked that already but we never sure), and maybe he knows places that uses XULrunner that I haven't checked.
Attachment #801522 - Attachment is obsolete: true
Attachment #804484 - Flags: review?(yurenju.mozilla)
Attachment #804484 - Flags: review?(poirot.alex)
Attachment #804484 - Flags: review?(jlal)
Comment on attachment 804484 [details] [diff] [review]
patch v6

My review was more a test on OSX with a clean profile for both: |make test-integration| and the ./bin/gaia-marionette standalone.

Looks good!
Attachment #804484 - Flags: review?(jlal) → review+
Comment on attachment 804484 [details] [diff] [review]
patch v6

Review of attachment 804484 [details] [diff] [review]:
-----------------------------------------------------------------

r=yurenju with nit addressed and tested on Ubuntu 13.04 and Windows Vista with MinGW.

::: Makefile
@@ +81,5 @@
>  endif
>  
>  PROFILE_FOLDER?=profile
>  
> +BUILD_FOLDER?=build_stage

a little bit confusing with GAIA_DIR/build/, BUILD_STAGE_FOLDER or STAGE_FOLDER would be better.
Attachment #804484 - Flags: review?(yurenju.mozilla) → review+
Comment on attachment 804484 [details] [diff] [review]
patch v6

Review of attachment 804484 [details] [diff] [review]:
-----------------------------------------------------------------

Works fine for me as well on windows, even in multilocale mode!
But I have some doubts for the marionette script.

::: Makefile
@@ +81,5 @@
>  endif
>  
>  PROFILE_FOLDER?=profile
>  
> +BUILD_FOLDER?=build_stage

+1

::: apps/email/Makefile
@@ +1,4 @@
> +# We can't figure out XULRUNNERSDK on our own; it's complex and some builders
> +# # may want to override our find logic (ex: TBPL), so let's just leave it up to
> +# # the root Makefile.  If you know what you're doing, you can manually define
> +# # XULRUNNERSDK and XPCSHELLSDK on the command line.

That looks more like a message to email devs than an actual code comment.
May be a comment like this would be more appropriate next to the exports XULRUNNERSDK XPCSHELLSDK in the root Makefile. Where you simply put "# we need these variables in external processes".

@@ +2,5 @@
> +# # may want to override our find logic (ex: TBPL), so let's just leave it up to
> +# # the root Makefile.  If you know what you're doing, you can manually define
> +# # XULRUNNERSDK and XPCSHELLSDK on the command line.
> +ifndef XPCSHELLSDK
> +$(error This Makefile needs to be run by the root gaia makefile. Use APP=email)

Use `/gaia $ APP=email make`

::: bin/gaia-marionette
@@ +26,5 @@
> +  # find xulrunner ourselves
> +  XULRUNNER=`cd $DIR/../xulrunner-sdk* && pwd`
> +  if [ -d "$XULRUNNER/xulrunner-sdk" ] ; then
> +    XULRUNNER="$XULRUNNER/xulrunner-sdk"
> +  fi

Shouldn't we bail out it there is no XULRUNNER_DIRECTORY?

If not, in which cases we are running this script without running gaia's makefile?

Otherwise, it can be simplified and strengthen with something like this:
XULRUNNER=`cd $DIR/../xulrunner-sdk*/xulrunner-sdk && pwd`
or even better
XULRUNNER=$(abspath $DIR/../xulrunner-sdk*/xulrunner-sdk)

The bad thing is that we will select a random directory, as the main goal of your patch is to keep these xulrunner-xxx directories...
Attachment #804484 - Flags: review?(poirot.alex)
Fixed most of the changes in the PR, thanks a lot Alexandre !

About the last bit, James, when launching gaia-marionette directly, would it be ok for you if we exit with an error code if there is no XULRUNNER_DIRECTORY ? In another terms, is it ok for you to specify XULRUNNER_DIRECTORY as an env variable when you run gaia-marionette directly ?

Will propose another patch once this is answered.
Flags: needinfo?(jlal)
I don't feel too good about that its one more thing people need to know about when running the tests. 
My goal for the bin/gaia-marionette executable is you point it at a file and no matter your state you get a working test run.

Is there another way to work around this?
Flags: needinfo?(jlal)
maybe we can use `ls -d xulrunner-sdk* | sort -n | tail -n1` to find at least the most current one, then ?
That should work- The latest and greatest is not really needed... We use xpcshell only so we can run the thunderbird fake servers.
Attached patch patch v7 (obsolete) — Splinter Review
The PR at https://github.com/mozilla-b2g/gaia/pull/11812 has an additional commit containing all follow-ups.

I didn't test this on Windows yet, will do if Alexandre doesn't test it himself before ;)

I guess the integration tests need to work on windows too ?
Attachment #804484 - Attachment is obsolete: true
Attachment #808179 - Flags: review?(poirot.alex)
Attachment #808179 - Flags: feedback?(jlal)
Windows build works for me. (didn't try multilocale on Windows)

Integration tests don't, even after installing npm. James, are they supposed to work on windows ?
James told me they're not supposed to work on Windows, so it seems everything is green.
Comment on attachment 808179 [details] [diff] [review]
patch v7

makes sense to me for the bin/gaia-marionette stuff... I tested on OSX and it found my xpcshell! Travis should hopefully do the rest.
Attachment #808179 - Flags: feedback?(jlal) → feedback+
Comment on attachment 808179 [details] [diff] [review]
patch v7

Review of attachment 808179 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for the inlined followups!
Attachment #808179 - Flags: review?(poirot.alex) → review+
master: 106001c4e0ab9253001bed815c03f46b8dd046f0

if there isn't any regression I'd like to request an approval for v1.2.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Seems like I broke email -- again.

Thanks for the Gaia UI Tests ;)
The problem is that FTU is showing with my patch whereas it's not without.
The lockscreen is enabled too.

James, I really can't find what I could break here, would you point me where you disable FTU and lockscreen in the marionette runner ?
Flags: needinfo?(jlal)
sorry for taking forever to respond... here is where you can try to add those settings globally:

https://github.com/mozilla-b2g/gaia/blob/master/shared/test/integration/profile.js
Flags: needinfo?(jlal)
master: f6df42dddb0ef62c64d90d9ca06fba779ea6cd78
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attached patch patch v8Splinter Review
carrying r+

The problem was with the look up for the xpcshell directory.

All green (except the usual 5 python UI tests) so landing :)
Attachment #808179 - Attachment is obsolete: true
Attachment #819481 - Flags: review+
Is there any missing patch?

Looks like running mochitest-remote is broken. I don't have 'gaia/xulrunner-sdk-26/bin', but I do have 'gaia/xulrunner-sdk-26/xulrunner-sdk/bin'.

$ ./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
I didn't know we had a mach in B2G and that it was using gaia's xulrunner for something.

Seems like TBPL doesn't test for this either ;-)

I think it makes sense to fix this in a follow-up patch. Do you think you can handle this ?
(In reply to Julien Wajsberg [:julienw] from comment #44)
> I didn't know we had a mach in B2G and that it was using gaia's xulrunner
> for something.
> 
> Seems like TBPL doesn't test for this either ;-)
> 
> I think it makes sense to fix this in a follow-up patch. Do you think you
> can handle this ?

It's not like I need to be able to run those tests :), I'll come up with some fix asap.
Opened as bug 931718
Depends on: 931718
Github PR for v1.2 uplifting: https://github.com/mozilla-b2g/gaia/pull/13503
Waiting for a green travis for such an important topic.
travis green!
a=npotb
v1.2: d9b12735a00eb7ed44b10d5592a4cb8ee778c03d

This will esp. help with failing earlier for submake, eg for bug 931550.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: