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.

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(-)

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 :)

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!

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!

master: 726777cbe7dc4f3a3731fcf4629bea56c352a603 thanks a lot Yuren !

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.

Comment on attachment 793963 [details] [diff] [review] follow-up Review of attachment 793963 [details] [diff] [review]: ----------------------------------------------------------------- r=me !

follow-up master: c088c47414937d615a94c45fab3e8842a2f4edc0

master: 2a3135de3fdc6847661d866ca26d9b45de9fe378, reverted 726777cbe7dc4f3a3731fcf4629bea56c352a603 Note: I haven't reverted the follow-up as it does no harm.

So, yuren, how about doing only one patch with all the follow-up patches ? Should be easier to test and re-land.

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

Note: the revert was for bug 908691

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.

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

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.

just noticed other places where the "xulrunner-sdk" path is assumed: * tools/xpcwindow/bin/xpcwindow * tests/js/bin/runner

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

|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.

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.

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!

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.

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...

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.

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?

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.

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 ?

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.

Comment on attachment 808179 [details] [diff] [review] patch v7 Review of attachment 808179 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks for the inlined followups!

master: 106001c4e0ab9253001bed815c03f46b8dd046f0 if there isn't any regression I'd like to request an approval for v1.2.

reverted: https://github.com/mozilla-b2g/gaia/commit/2230cac98911d1d770fc9c2eb26321a325fee14a sorry!) its red on ci... I tried restarting it on travis but it failed consistently https://travis-ci.org/mozilla-b2g/gaia/jobs/11821486

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 ?

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

master: f6df42dddb0ef62c64d90d9ca06fba779ea6cd78

carrying r+ The problem was with the look up for the xpcshell directory. All green (except the usual 5 python UI tests) so landing :)

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

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.