Closed Bug 1354262 Opened 7 years ago Closed 7 years ago

allow devedition behavior changes to be specified via a repack

Categories

(DevTools :: General, enhancement, P1)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Keywords: addon-compat)

Attachments

(4 files)

Bug 1354262 - turn devedition into a pref for js code;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1354262 - remove MOZ_DEV_EDITION in favor of a repack setting;
59 bytes, text/x-review-board-request
benjamin
: review+
Details
part 1, rebased as of June 2018 (turn devedition into a pref for js code)
18.26 KB, patch
Details | Diff | Splinter Review
part 2, rebased as of June 2018 (remove MOZ_DEV_EDITION in favor of a repack setting)
21.11 KB, patch
Details | Diff | Splinter Review 
We'd like devedition to be implemented via a repack.
The main goal of this bug is to make the devedition profile
handling be implementable via a webpack, somehow.
However it seems to me that all compile-time uses of MOZ_DEV_EDITION
need to be removed.
After an attempt to use a simple distribution.ini, and some debugging,
I suspect this depends on bug 802022.
Depends on: 802022
Useful info on repacks: https://wiki.mozilla.org/Distribution_INI_File
I'm curious how fixing 802022 would solve the profile problem?

Distribution.ini still happens after profile selection, doesn't it?
More logging revealed that the file should be
obj-x86_64-pc-linux-gnu/dist/bin/distribution/distribution.ini
... and once I did this, setting a pref works ok, and so I don't think
bug 802022 is needed.
No longer depends on: 802022
Most likely we're going to go a different route and try to have a different default
profile per update channel.  Leaving this open for the time being though.
See bug 1355204 for the other approach.
I'll attach my initial patches momentarily.

If you want to try this out:

* Apply patches, rebuild.
* mkdir obj-x86_64-pc-linux-gnu/dist/bin/distribution
  (your path may vary)
* Put this into that directory as "distribution.ini":

=============================================
[Global]
id=fixme
version=99.99
about=Developer Edition
about.en-US=Developer Edition

[Preferences]
devtools.devedition=true
app.feedback.baseURL="https://input.mozilla.org/%LOCALE%/feedback/firefoxdev/%VERSION%/"
lightweightThemes.selectedThemeID="firefox-compact-dark@mozilla.org"
identity.fxaccounts.migrateToDevEdition=true
devtools.theme="dark"
devtools.webide.widget.enabled=true
devtools.webide.widget.inNavbarByDefault=true
=============================================


I don't think there's a way to make some pref change its default based on another pref,
so the simplest approach was to just require the devedition repack config to carry all
the modified prefs.
(In reply to Tom Tromey :tromey from comment #7)
> I'll attach my initial patches momentarily.
> 
> If you want to try this out:
> 
> * Apply patches, rebuild.
> * mkdir obj-x86_64-pc-linux-gnu/dist/bin/distribution
>   (your path may vary)
> * Put this into that directory as "distribution.ini":
> 
> =============================================
> [Global]
> id=fixme
> version=99.99
> about=Developer Edition
> about.en-US=Developer Edition
> 
> [Preferences]
> devtools.devedition=true
> app.feedback.baseURL="https://input.mozilla.org/%LOCALE%/feedback/firefoxdev/
> %VERSION%/"
> lightweightThemes.selectedThemeID="firefox-compact-dark@mozilla.org"
> identity.fxaccounts.migrateToDevEdition=true
> devtools.theme="dark"
> devtools.webide.widget.enabled=true
> devtools.webide.widget.inNavbarByDefault=true
> =============================================

Is distribution.ini the only file we need? Do we need to include the firefox-compact-dark@mozilla.org theme as well?
Flags: needinfo?(ttromey)
https://github.com/mozilla-partners/devedition

I made this a public repo.

Let me know when you are ready to start building and I can put the proper information in the build script that kicks off the builds.

If you have any questions, don't hesitate to ask.
(In reply to Ben Hearsum (:bhearsum) from comment #10)

> Is distribution.ini the only file we need? Do we need to include the
> firefox-compact-dark@mozilla.org theme as well?

I don't think so, because I think that theme is part of the browser.
Thanks to :bgrins for pointing me at the spot where this is handled:

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/browser/components/nsBrowserGlue.js#547
Clearing NI.
Flags: needinfo?(ttromey)
(In reply to Mike Kaply [:mkaply] from comment #11)
> https://github.com/mozilla-partners/devedition
> 
> I made this a public repo.
> 
> Let me know when you are ready to start building and I can put the proper
> information in the build script that kicks off the builds.
> 
> If you have any questions, don't hesitate to ask.

We're going to want a custom channel here, too. I was thinking we should set app.distributor.channel to "devedition".
(In reply to Ben Hearsum (:bhearsum) from comment #14)
> (In reply to Mike Kaply [:mkaply] from comment #11)
> > https://github.com/mozilla-partners/devedition
> > 
> > I made this a public repo.
> > 
> > Let me know when you are ready to start building and I can put the proper
> > information in the build script that kicks off the builds.
> > 
> > If you have any questions, don't hesitate to ask.
> 
> We're going to want a custom channel here, too. I was thinking we should set
> app.distributor.channel to "devedition".

Erm, that's probably app.partner.devedition="devedition". Seems that the former only affects Telemetry (though we may want to set that, too?).
> Seems that the former only affects Telemetry (though we may want to set that, too?).

Yes, we'd like to keep a separate Telemetry channel if possible.  Currently we track this as the Aurora channel and don't want data to disappear.
I'll update the distribution.ini so you should get all the reporting.
(In reply to Mike Kaply [:mkaply] from comment #11)

> If you have any questions, don't hesitate to ask.

I see this:

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/browser/branding/aurora/configure.sh#5

Is it possible to replicate these settings in distribution.ini?
I don't know the mapping, if any, between these settings and the various [Global] things.
I also don't know if these are even important.
Flags: needinfo?(mozilla)
Note to self - I missed some spots mentioning MOZ_DEV_EDITION, importantly in main.xul.
> Is it possible to replicate these settings in distribution.ini?
I don't know the mapping, if any, between these settings and the various [Global] things.
I also don't know if these are even important.

No, those settings can't be overridden.

I honestly don't know what those are actually used for...
Flags: needinfo?(mozilla)
I've fixed all remaining uses of MOZ_DEV_EDITION, except this one:

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/browser/branding/aurora/configure.sh#7

... which doesn't seem important to deal with as presumably it will go away once aurora itself does.
Blocks: 1356606
Comment on attachment 8857597 [details]
Bug 1354262 - turn devedition into a pref for js code;

https://reviewboard.mozilla.org/r/129566/#review133674

Code-wise, I think this makes sense, though I haven't checked that you got all of the relevant places...

I'm a little concerned about testing though. Right now we have test coverage of devedition by virtue of the separate aurora tree, and it's not uncommon we catch issues this way. Are we planning to still have a twig with this configuration on which we run automated tests, to check we're not breaking worse things (see e.g. the things that are already broken in devedition that have bug numbers listed in this diff, which we just live with at this stage...) when repacking with this pref set?
Attachment #8857597 - Flags: review?(gijskruitbosch+bugs) → review+
It looks like treestyletabs and possibly other add-ons depend on this signal, though there aren't *too* many ( https://dxr.mozilla.org/addons/search?q=MOZ_DEV_EDITION&redirect=false ). Because JS, they'll now just get a falsy value if they check AppConstants.MOZ_DEV_EDITION, and I don't know how bad that will affect them, but we should probably keep it in mind. We could potentially reimplement it as a pref getter to reduce compat risk.
Keywords: addon-compat
Comment on attachment 8857598 [details]
Bug 1354262 - remove MOZ_DEV_EDITION in favor of a repack setting;

https://reviewboard.mozilla.org/r/129568/#review134058

Overall this looks good: I have a couple of naming/API nits which I need to see again because this is complex and I want to avoid as much confusion as possible for future people who touch this.

::: commit-message-1547c:1
(Diff revision 3)
> +Bug 1354262 - remove MOZ_DEV_EDITION in favor of a pref; r?bsmedberg

So after reading this, I think the commit message mentioning a "pref" is incorrect. Nowhere does this patch actually set a pref (in the nsIPrefService sense), correct?

::: toolkit/profile/nsIToolkitProfileService.idl:98
(Diff revision 3)
>  
>      /**
> +     * Set special "devedition" flag.  This changes some aspects of
> +     * default profile selection.  Once set this cannot be undone.
> +     */
> +    void developerEdition();

I think I must have missed context from elsewhere, but why do we need this setter? It's not used in this patch, and if we're consistently reading it from override.ini I can't think of a reason we'd need it to be JS-accessible.

::: toolkit/xre/nsAppRunner.cpp:4225
(Diff revision 3)
> +{
> +  if (NS_FAILED(mINIParserResult)) {
> +    return false;
> +  }
> +  nsAutoCString buf;
> +  nsresult rv = mINIParser->GetString("Preferences", "devtools.devedition", buf);

I'm concerned that "Preferences" and the dotted.name convention here looks like a preference but isn't. I think it would be better to put this in the "App" section, maybe "App" "DevEditionProfileDefault" ?

::: toolkit/xre/nsAppRunner.cpp:4229
(Diff revision 3)
> +  nsAutoCString buf;
> +  nsresult rv = mINIParser->GetString("Preferences", "devtools.devedition", buf);
> +  if (NS_FAILED(rv)) {
> +    return false;
> +  }
> +  return buf == "true";

I realize that application.ini behavior isn't documented at all. But other flags are parsed here and don't use full matching:

https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/CreateAppData.cpp#37

Since we don't report errors, I'd prefer to be similarly lenient here.
Attachment #8857598 - Flags: review?(benjamin) → review-
(In reply to :Gijs from comment #26)
> We could potentially
> reimplement it as a pref getter to reduce compat risk.

Thanks for doing this search.  I had forgotten about this.
I think adding a getter here is the safest approach, so I will do that.
Comment on attachment 8857598 [details]
Bug 1354262 - remove MOZ_DEV_EDITION in favor of a repack setting;

https://reviewboard.mozilla.org/r/129568/#review134058

> I think I must have missed context from elsewhere, but why do we need this setter? It's not used in this patch, and if we're consistently reading it from override.ini I can't think of a reason we'd need it to be JS-accessible.

Sorry about this.  This was a leftover from an earlier approach that I forgot to remove.  I've removed it now.

> I'm concerned that "Preferences" and the dotted.name convention here looks like a preference but isn't. I think it would be better to put this in the "App" section, maybe "App" "DevEditionProfileDefault" ?

This actually is a pref.  The [Preferences] section is read later and the values there are set using the preference service.  See https://wiki.mozilla.org/Distribution_INI_File.  The overall approach of these two patches is to turn dev edition from a compile-time define into a pref, so that various bits of JS can adapt (see the first patch for those adaptations).

It would be possible to have this be a pref and have a separate setting in [App], but this way seemed more parsimonious.  I don't mind doing that if you'd prefer, though; it's just a single addition in https://github.com/mozilla-partners/devedition/blob/master/desktop/devedition/distribution/distribution.ini

> I realize that application.ini behavior isn't documented at all. But other flags are parsed here and don't use full matching:
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/CreateAppData.cpp#37
> 
> Since we don't report errors, I'd prefer to be similarly lenient here.

Because it's also a pref I think in this case it's best to use full matching.  Otherwise, in case of a typo, the startup code could think it is dev edition but the JS bits would not. 

The value parsing for the [Preferences] section is done here:

https://dxr.mozilla.org/mozilla-central/rev/722fdbff1efc308a22060e75b603311d23541bb5/browser/components/distribution.js#469

... which is not lenient.  So, I'm going to drop this one -- but as above, it's possible to introduce a new setting in [App] and change distribution.js to use that to set the devedition pref.  In this case leniency would be possible.
Comment on attachment 8857598 [details]
Bug 1354262 - remove MOZ_DEV_EDITION in favor of a repack setting;

https://reviewboard.mozilla.org/r/129568/#review134058

> So after reading this, I think the commit message mentioning a "pref" is incorrect. Nowhere does this patch actually set a pref (in the nsIPrefService sense), correct?

A pref is set, but I agree that this message is not very good, and I will change it.
(In reply to :Gijs from comment #25)

> I'm a little concerned about testing though. Right now we have test coverage
> of devedition by virtue of the separate aurora tree, and it's not uncommon
> we catch issues this way. Are we planning to still have a twig with this
> configuration on which we run automated tests

I do not know but I will forward this to the people involved with QA for this project to see
what they think.
(In reply to :Gijs from comment #25)
> Comment on attachment 8857597 [details]
> Bug 1354262 - turn devedition into a pref for js code;
> 
> https://reviewboard.mozilla.org/r/129566/#review133674
> 
> Code-wise, I think this makes sense, though I haven't checked that you got
> all of the relevant places...
> 
> I'm a little concerned about testing though. Right now we have test coverage
> of devedition by virtue of the separate aurora tree, and it's not uncommon
> we catch issues this way. Are we planning to still have a twig with this
> configuration on which we run automated tests, to check we're not breaking
> worse things (see e.g. the things that are already broken in devedition that
> have bug numbers listed in this diff, which we just live with at this
> stage...) when repacking with this pref set?

We do not do separate testing for any partner repacks (yahoo, bing, etc.). If we end up shipping this as a repack, I do not expect any automated tests to be run.
(In reply to :Gijs from comment #25)
> I'm a little concerned about testing though. Right now we have test coverage
> of devedition by virtue of the separate aurora tree, and it's not uncommon
> we catch issues this way. Are we planning to still have a twig with this
> configuration on which we run automated tests, to check we're not breaking
> worse things (see e.g. the things that are already broken in devedition that
> have bug numbers listed in this diff, which we just live with at this
> stage...) when repacking with this pref set?

This is a question for Ritu if there is another solution for this automated testing need.
Flags: needinfo?(rkothari)
The most recent debate is whether to address the DevEd needs via a repack or rebuild. I'd hold off until that decision is made. 

Hi Gijs, could you please elaborate on the specific DevEd configuration aspects (other than branding, themes, profiles that we plan to test during sign-offs) that differ from regular Beta builds and need specific testing? Depending on those requirements, we can explore potential solutions.
Flags: needinfo?(rkothari) → needinfo?(gijskruitbosch+bugs)
(In reply to Ritu Kothari (:ritu) from comment #36)
> The most recent debate is whether to address the DevEd needs via a repack or
> rebuild. I'd hold off until that decision is made. 
> 
> Hi Gijs, could you please elaborate on the specific DevEd configuration
> aspects (other than branding, themes, profiles that we plan to test during
> sign-offs) that differ from regular Beta builds and need specific testing?
> Depending on those requirements, we can explore potential solutions.

The theming influences Firefox's and devtools' behaviour, though (especially in the devtools themselves and the tabstrip, where esp. the latter is pretty primary functionality), as do the different default button placements (especially wrt customization / customize mode). Automated tests verify that a number of things "just work", and I don't think replacing that with manual QE attention will have the same effect. We'll find issues later and require a lot more person-hours to find/fix, potentially shipping regressions if we can't scramble together fixes in time. Plus person-hours are more expensive than automation-hours.

I think a potential solution would be running mochitest-bc and/or mochitest-e10s-bc (including devtools m-bc) tests against repacks on at least one platform. This won't need a separate rebuild job, just running the test against a build with a distribution file dropped in that triggers the changes these patches encompass. This would also be beneficial because we would gain rudimentary test coverage of the repack infrastructure "working", which I believe we don't have today. We do rely on that working properly, also for partner builds and for funnelcakes, so having some coverage of that functionality (mostly related to startup and changed default prefs) would be an added bonus.

If there are serious cost or machine availability concerns around this, the devtools team and I could help identify a subset of test dirs that would be helpful to run. But, to be fair, getting rid of aurora will free up availability/cost to the point where I think what I proposed should be pretty feasible.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(rkothari)
Thanks Gijs for the detailed write up! It is a fact that we will be freeing up aurora build/test run resources and that quota could be reused for DevEd builds.

We (relmgmt, releng, product team) made a decision this morning that all automated tests will be run on the DevEd builds. Ben, please correct me if I am wrong.
Flags: needinfo?(rkothari) → needinfo?(bhearsum)
(In reply to Ritu Kothari (:ritu) from comment #38)
> Thanks Gijs for the detailed write up! It is a fact that we will be freeing
> up aurora build/test run resources and that quota could be reused for DevEd
> builds.
> 
> We (relmgmt, releng, product team) made a decision this morning that all
> automated tests will be run on the DevEd builds. Ben, please correct me if I
> am wrong.

Yes, and we are no longer doing repacks for them, we're doing rebuilds instead. From a CI standpoint, this will look a lot like the "debug" builds we already do on branches like mozilla-beta.
Flags: needinfo?(bhearsum)
Comment on attachment 8857598 [details]
Bug 1354262 - remove MOZ_DEV_EDITION in favor of a repack setting;

https://reviewboard.mozilla.org/r/129568/#review136300

r=me with that change

::: commit-message-1547c:6
(Diff revision 4)
> +Bug 1354262 - remove MOZ_DEV_EDITION in favor of a repack setting; r?bsmedberg
> +
> +This changes MOZ_DEV_EDITION from a compile-time flag to a runtime flag,
> +in particular allowing it to be set in a repack.  It changes
> +nsToolkitProfileService so that this new flag can be propagated from
> +nsAppRunner.

I'd like this commit message to be more specific, for example: "the startup code will check distribution.ini for a [Preferences] devtools.devedition pref and if present alter default profile selection to use the devedition path.
Attachment #8857598 - Flags: review?(benjamin) → review+
(In reply to Ben Hearsum (:bhearsum) from comment #39)
> (In reply to Ritu Kothari (:ritu) from comment #38)
> > Thanks Gijs for the detailed write up! It is a fact that we will be freeing
> > up aurora build/test run resources and that quota could be reused for DevEd
> > builds.
> > 
> > We (relmgmt, releng, product team) made a decision this morning that all
> > automated tests will be run on the DevEd builds. Ben, please correct me if I
> > am wrong.
> 
> Yes, and we are no longer doing repacks for them, we're doing rebuilds
> instead. From a CI standpoint, this will look a lot like the "debug" builds
> we already do on branches like mozilla-beta.

If this is the case, do we still need/want to go ahead with these changes, or should we just mark this wontfix and move on?
Thanks. We've decided against the repack and are going to a rebuild for DE.  You should be cc'd on the other bugs now.  Closing this.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
See Also: → 1459240
No longer blocks: 1459240
Product: Firefox → DevTools
In case we end up wanting to take some version of this for bug 1459240: here's a rebased version of "part 1" here.  Haven't tested thoroughly -- just rebased, basically.
status-firefox55: affected → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: