Closed
Bug 1104198
Opened 8 years ago
Closed 8 years ago
DESKTOP=1 profiles don't work with mulet
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: eeejay, Assigned: eeejay)
References
Details
Attachments
(1 file, 1 obsolete file)
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.
Assignee | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/a157339f944c70813b443c4bd4125fbb42961a53
Assignee: nobody → eitan
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 5•8 years ago
|
||
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 → ---
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 7•8 years ago
|
||
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)
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
Oops, jgriffin is probably the right person to ask about that. See comment #7.
Flags: needinfo?(jgriffin)
Assignee | ||
Comment 11•8 years ago
|
||
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•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
(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•8 years ago
|
||
(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)
Assignee | ||
Comment 15•8 years ago
|
||
(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.
Assignee | ||
Comment 16•8 years ago
|
||
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)
Comment 17•8 years ago
|
||
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)
Comment 18•8 years ago
|
||
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)
Assignee | ||
Comment 19•8 years ago
|
||
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 20•8 years ago
|
||
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 21•8 years ago
|
||
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+
Assignee | ||
Comment 22•8 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/f385ed5c58b575042079d656b866877bf6c23f87
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 23•8 years ago
|
||
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)
Comment 24•8 years ago
|
||
(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)
Comment 25•8 years ago
|
||
(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.
Description
•