1104198 - DESKTOP=1 profiles don't work with mulet

Status: RESOLVED FIXED

Description

[Eitan Isaacson [:eeejay]]

This is because of two main issues:
1. The desktop shims extension gets installed. Generally the if/else blocks in the extensions target in the Makefile looks wrong.
2. The start app is set to the system app, when it should be kept as the shell chrome.

Comment 1

[Eitan Isaacson [:eeejay]]

Created attachment 8527832 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26420

I guess the assumption here is that we are dropping desktop Firefox support. I removed all the prefs from the DESKTOP mode since they were mostly redundant and were used to override browser app prefs that we don't use anymore.

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8527832 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26420

Seems fine to me, but I wonder if we should remove the whole desktop pref function? Also moving the review over to Ricky as he is a build peer.

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8527832 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26420

LGTM for build part. r=@RickyChien

Please fix Gb and Gu failures in treeherder thanks!

Comment 4

[Eitan Isaacson [:eeejay]]

https://github.com/mozilla-b2g/gaia/commit/a157339f944c70813b443c4bd4125fbb42961a53

Comment 5

[Kevin Grandon :kgrandon]

Backed out for causing unit test failures (same ones seen in the gaia-try run): https://github.com/mozilla-b2g/gaia/commit/cbd571cecd61e4601eac8de709778a02c2cc1626

https://tbpl.mozilla.org/php/getParsedLog.php?id=53982805&tree=B2g-Inbound

My hunch is that we are expecting to pass in some shims when DEBUG is set, and this patch is changing that. Ni? on Eitan for awareness.

Comment 6

[Treeherder Robot]

submit_timestamp: 2014-12-04T12:35:42
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=b2g-inbound&job_id=948595
repository: b2g-inbound
who: kgrandon[at]mozilla[dot]com
machine: tst-linux64-spot-1038
buildname: b2g_ubuntu64_vm b2g-inbound opt test gaia-unit
revision: 2129c0182eed

TEST-UNEXPECTED-FAIL | system/test/unit/nfc_manager_test.js | Nfc Manager Functions "before each" hook - setting a property that has only a getter
TypeError: setting a property that has only a getter
TEST-UNEXPECTED-FAIL | system/test/unit/nfc_handover_manager_test.js | Nfc Handover Manager Functions "before all" hook - setting a property that has only a getter
TypeError: setting a property that has only a getter
Return code: 10

Comment 7

[Eitan Isaacson [:eeejay]]

This is failing because the desktop-helpers extension is being installed (because DESKTOP_SHIMS=1). Prior to this patch the DESKTOP_SHIMS=1 setting was broken, and the extension was not installed.

So we could fix this in three ways:
1. Modify mozharness, and remove DESKTOP_SHIMS=1 (I don't think it is actually needed in any test, is it?)
2. Somehow provide a build_config.json like the gaia ui tests have, haven't successfully managed to do that.
3. Re-introduce the broken conditional blocks in the Makefile that made this not an issue in the first place.

I'll defer to Zac, and hist testy wisdom.

Comment 8

[Kevin Grandon :kgrandon]

The shims were definitely needed at one point to load some APIs for python tests. I'm not sure if that's still the case though.

Comment 9

[Zac C (:zac)]

Sorry this is the unit tests, it's outside of my area so I can't help :(


(In reply to Kevin Grandon :kgrandon from comment #8)
> The shims were definitely needed at one point to load some APIs for python
> tests. I'm not sure if that's still the case though.

Yes that's the case, we don't nee the shims. (but also not the suite that's failing here).

Comment 10

[Eitan Isaacson [:eeejay]]

Oops, jgriffin is probably the right person to ask about that. See comment #7.

Comment 11

[Eitan Isaacson [:eeejay]]

More specifically, the issue seems to be that mozharness sets DESKTOP_SHIMS to 1 and, at least for the unit tests, there is no way to override it. Before the patch in this bug, DESKTOP_SHIMS was inert, and didn't work as advertised.

Do we need to change mozharness? Is there a way to override it in the gaia tree?

Comment 12

[Kevin Grandon :kgrandon]

I think the problem is the updates to the sytax:  else ifeq ($(DESKTOP),1)

Seems like we're no longer copying extensions in that case? I'm not sure, I haven't looked at the mozharness code lately.

Comment 13

[Eitan Isaacson [:eeejay]]

(In reply to Kevin Grandon :kgrandon from comment #12)
> I think the problem is the updates to the sytax:  else ifeq ($(DESKTOP),1)
> 
> Seems like we're no longer copying extensions in that case? I'm not sure, I
> haven't looked at the mozharness code lately.

The logs show DESKTOP=0

mozharness env looks like this:
http://hg.mozilla.org/user/armenzg_mozilla.com/mozharness/file/d061f6a73de9/mozharness/mozilla/gaia.py#l277

Comment 14

[Jonathan Griffin (:jgriffin)]

(In reply to Eitan Isaacson [:eeejay] from comment #7)
> This is failing because the desktop-helpers extension is being installed
> (because DESKTOP_SHIMS=1). Prior to this patch the DESKTOP_SHIMS=1 setting
> was broken, and the extension was not installed.
> 
> So we could fix this in three ways:
> 1. Modify mozharness, and remove DESKTOP_SHIMS=1 (I don't think it is
> actually needed in any test, is it?)
> 2. Somehow provide a build_config.json like the gaia ui tests have, haven't
> successfully managed to do that.
> 3. Re-introduce the broken conditional blocks in the Makefile that made this
> not an issue in the first place.
> 
> I'll defer to Zac, and hist testy wisdom.

Locally, gaia unit tests don't seem to work without DESKTOP_SHIMS, so I guess we do still need it.  Couldn't we just add desktop-helper (the only thing that DESKTOP_SHIMS seems to do) for all debug && desktop profiles?

Comment 15

[Eitan Isaacson [:eeejay]]

(In reply to Jonathan Griffin (:jgriffin) from comment #14)
> (In reply to Eitan Isaacson [:eeejay] from comment #7)
> > This is failing because the desktop-helpers extension is being installed
> > (because DESKTOP_SHIMS=1). Prior to this patch the DESKTOP_SHIMS=1 setting
> > was broken, and the extension was not installed.
> > 
> > So we could fix this in three ways:
> > 1. Modify mozharness, and remove DESKTOP_SHIMS=1 (I don't think it is
> > actually needed in any test, is it?)
> > 2. Somehow provide a build_config.json like the gaia ui tests have, haven't
> > successfully managed to do that.
> > 3. Re-introduce the broken conditional blocks in the Makefile that made this
> > not an issue in the first place.
> > 
> > I'll defer to Zac, and hist testy wisdom.
> 
> Locally, gaia unit tests don't seem to work without DESKTOP_SHIMS, so I
> guess we do still need it.  Couldn't we just add desktop-helper (the only
> thing that DESKTOP_SHIMS seems to do) for all debug && desktop profiles?

That is exactly what this patch does, It removes the desktop helpers, since they are not needed/break mulet.

I had the unit tests work locally with DESKTOP_SHIMS=0, I'll double check again.

Comment 16

[Eitan Isaacson [:eeejay]]

So I completely removed the DESKTOP_SHIMS option, and also the desktop-helper extension, and the tests are happy.
https://github.com/mozilla-b2g/gaia/pull/26752

I think DESKTOP_SHIMS should be entirely removed, since it was broken before, and it doesn't seem to serve anything anymore. I am not certain if we want to completely remove desktop-helper, since SIMULATOR=1 still uses that, but I don't know who would use that flag these days. What do you think, Kevin?

Comment 17

[Kevin Grandon :kgrandon]

I'm fine removing DESKTOP_SHIMS, but if the simulator still needs the desktop helper extension, I think we should keep it?

Alex - any opinion?

Comment 18

[Alexandre Poirot [:ochameau]]

Oh! Simulator no longer uses SIMULATOR=1, only the old r2d2b2g one, based on FxOS 1.1 was using this.
Now simulator uses just a regular device/production-like profile, with some tweaks like NOFTU=1 and custom prefs/settings. So that it no longer relies on DESKTOP, nor DESKTOP_SHIMS.

Having said that, speaking about the mulet, only DESKTOP=1 preferences messes with it, only the stuff related to Fx nightly support can break the mulet. The browser-helper addon installed with DEBUG=1 also conflicts with the code we landed in Mulet. The shims, installed implicitely with DESKTOP=1 or explicitely with DESKTOP_SHIMS=1 don't. These shims are valid on any desktop runtime, at least until we enable OOP on desktop. They should work on b2g desktop, mulet, fx nightly. They are mocking some API that doesn't work at all on desktop like mobile connection, bluetooth, camera, wifi, ... I think that the tests rely on this to execute some more tests on desktop. It isn't perfect as we are somewhat testing the mock itself, but I think it is handy for developers to be able to start writing and testing the tests on desktop before pushing to try in order to run it on the emulator.

`$ DEBUG=1 DESKTOP_SHIMS=1 make` was broken on master. We used to support `DESKTOP=1 DESKTOP_SHIMS=0` (this works as expected on master). This is why it broke. So it looks like tests, at the end of all that story, just no longer expect these shims. Looking at the failing test, nfc_manager_test.js, each test defines its own set of shims/mocks...

f+! (Feel free to also remove SIMULATOR=1 if you want to clean even more stuff in the Makefile)

Comment 19

[Eitan Isaacson [:eeejay]]

Created attachment 8536681 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26752

OK, this patch removes DESKTOP_SHIMS, and leaves desktop-helper and SIMULATOR in place in addition to the changes from the backed out patch.

I think the current use of both SIMULATOR and desktop-helper should be figured out in another bug.

Comment 20

[Kevin Grandon :kgrandon]

Comment on attachment 8536681 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26752

Seems good to me, though I think a build peer should probably put the final R+ on this. Thanks for sticking with it.

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8536681 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26752

You still change "DESKTOP=1 EVERYTHING_ELSE=0" by copying only additional addons,
instead of all (like browser helper, alarms, activities).
So that it basically disable most DESKTOP=1 features, but I think it is fine given that we have seen that tests do not expect these features anymore and most of them are only useful in Fx nightly mode that is broken.

r+, but we should continue cleanup up stuff.

Comment 22

[Eitan Isaacson [:eeejay]]

https://github.com/mozilla-b2g/gaia/commit/f385ed5c58b575042079d656b866877bf6c23f87

Comment 23

[Julien Wajsberg [:julienw]]

Hey Alexandre,

Do you kno why we did remove some of the pref used in setDebugPref? It breaks some behaviors, especially running things in Firefox Nightly.

I understand we want to eventually move everything to Mulet, but I don't think all documentation is ready, and it will confuse people. Is it ok if I revert the changes done in setDebugPref only?

Comment 24

[Alexandre Poirot [:ochameau]]

(In reply to Julien Wajsberg [:julienw] (PTO until 1/5) from comment #23)
> Hey Alexandre,
> 
> Do you kno why we did remove some of the pref used in setDebugPref? It
> breaks some behaviors, especially running things in Firefox Nightly.

Because we don't support nightly anymore. Mulet is the new thing.

> 
> I understand we want to eventually move everything to Mulet, but I don't
> think all documentation is ready, and it will confuse people. Is it ok if I
> revert the changes done in setDebugPref only?

Sure, but keep in mind that will just delay the removal of these nightly specifics.

Comment 25

[Julien Wajsberg [:julienw]]

(In reply to Alexandre Poirot [:ochameau] from comment #24)

> > 
> > I understand we want to eventually move everything to Mulet, but I don't
> > think all documentation is ready, and it will confuse people. Is it ok if I
> > revert the changes done in setDebugPref only?
> 
> Sure, but keep in mind that will just delay the removal of these nightly
> specifics.

I'm fine with this, but the change was quite unannounced now. I think we could even add a warning in httpd.js or bin/gaia-test when we're running in Firefox instead of Mulet. But with the current code, it's just broken. 

Preparing a fix in bug 1118829.