Closed Bug 1104198 Opened 7 years ago Closed 7 years ago

DESKTOP=1 profiles don't work with mulet

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 1 obsolete file)

Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26752
46 bytes, text/x-github-pull-request
ochameau
: review+
kgrandon
: feedback+
Details | Review 
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.
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.
Attachment #8527832 - Flags: review?(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.
Attachment #8527832 - Flags: review?(kgrandon) → review?(ricky060709)
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!
Attachment #8527832 - Flags: review?(ricky060709) → review+
https://github.com/mozilla-b2g/gaia/commit/a157339f944c70813b443c4bd4125fbb42961a53
Assignee: nobody → eitan
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(eitan)
Resolution: FIXED → ---
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.
Flags: needinfo?(eitan) → needinfo?(zcampbell)
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.
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).
Flags: needinfo?(zcampbell)
Oops, jgriffin is probably the right person to ask about that. See comment #7.
Flags: needinfo?(jgriffin)
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?
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.
(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
(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?
Flags: needinfo?(jgriffin)
(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.
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?
Flags: needinfo?(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?
Flags: needinfo?(kgrandon) → needinfo?(poirot.alex)
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)
Flags: needinfo?(poirot.alex)
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.
Attachment #8527832 - Attachment is obsolete: true
Attachment #8536681 - Flags: review?(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.
Attachment #8536681 - Flags: review?(poirot.alex)
Attachment #8536681 - Flags: review?(kgrandon)
Attachment #8536681 - Flags: feedback+
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.
Attachment #8536681 - Flags: review?(poirot.alex) → review+
https://github.com/mozilla-b2g/gaia/commit/f385ed5c58b575042079d656b866877bf6c23f87
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
See Also: → 1112745
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?
Flags: needinfo?(poirot.alex)
(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.
Flags: needinfo?(poirot.alex)
(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.
You need to log in before you can comment on or make changes to this bug.