1148062 - [Build] Add Lightsaber configs to Gecko, guarded by the `MOZ_DEV_EDITION` build-time flag

Status: RESOLVED WONTFIX

(Core :: General, defect, P1)

Description

[Doug Sherk (:drs) (inactive)]

We're planning on getting off the cypress Gecko branch for Lightsaber, and switching to mozilla-central, as soon as possible. To support this, we will need to be able to set some Gecko configs on build-time without switching branches.

To do this, we will have to:

1) B2G Repo
-----------
a. Add a check for |LIGHTSABER=1| on config, e.g. |LIGHTSABER=1 ./config.sh <device>|
b. Write `LIGHTSABER=1` to '.config'.
c. Export the `LIGHTSABER=1` env var in 'setup.sh'.

2) gonk-misc
------------
a. Add a check for `LIGHTSABER=1` in 'default-gonk-config', which sets the 'ac_add_options --enable-lightsaber' build config.

3) Gecko (mozilla-central)
--------------------------
a. Add a check for the '--enable-lightsaber' config in 'configure.in', which #defines 'LIGHTSABER' for cpp.
b. Change a series of configs in 'b2g/app/b2g.js' if 'LIGHTSABER' is defined.

I will break this up into mini-bugs once I get confirmation that this approach is reasonable from each respective reviewer.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Add --enable-lightsaber build config to configure.in, which defines LIGHTSABER.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Add lightsaber custom configs to b2g.js, guarded by LIGHTSABER definition.

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Add LIGHTSABER build-time exported flag.

PR: https://github.com/mozilla-b2g/B2G/pull/426

Comment 4

[Doug Sherk (:drs) (inactive)]

Attachment: Add --enable-lightsaber pref to default-gecko-config, guarded by LIGHTSABER=1.

Comment 5

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8584070 [details] [diff] [review]
Add lightsaber custom configs to b2g.js, guarded by LIGHTSABER definition.

When we land this on m-c, we can back out bug 1146294 on cypress. Wander I'm needinfoing you for visibility, as you will need to change the call to |./config.sh aries| to |LIGHTSABER=1 ./config.sh aries|.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8584069 [details] [diff] [review]
Add --enable-lightsaber build config to configure.in, which defines LIGHTSABER.

Review of attachment 8584069 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +8304,5 @@
> +MOZ_ARG_ENABLE_BOOL(lightsaber,
> +[  --enable-lightsaber      A group of experimental runtime settings],
> +    LIGHTSABER=1,
> +    LIGHTSABER= )
> +if test "$LIGHTSABER"; then

"lightsaber" "group of experimental runtime settings". Could that be less vague? I have no clue what lightsaber is and neither have people who'd see that help line.

Comment 7

[Mike Hommey [:glandium]]

Also note that if you're goal is just for LIGHTSABER=1 ./build.sh to do the right thing, you don't need to patch anything other than mozilla-central, and make configure set the AC_DEFINE if the environment variable is set: we don't need configure options for everything, especially when they are cryptic opt-ins.

Comment 8

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8584073 [details] [diff] [review]
Add LIGHTSABER build-time exported flag.

Dropping review and obsoleting based on comment 7.

Comment 9

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8584074 [details] [diff] [review]
Add --enable-lightsaber pref to default-gecko-config, guarded by LIGHTSABER=1.

Dropping review and obsoleting based on comment 7.

Comment 10

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8584070 [details] [diff] [review]
Add lightsaber custom configs to b2g.js, guarded by LIGHTSABER definition.

Dropping review and obsoleting based on comment 7.

Comment 11

[Doug Sherk (:drs) (inactive)]

I'm going to use the MOZ_DEV_EDITION flag instead, as suggested by Mike.

Comment 12

[Wander Lairson Costa]

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #5)
> Comment on attachment 8584070 [details] [diff] [review]
> Add lightsaber custom configs to b2g.js, guarded by LIGHTSABER definition.
> 
> When we land this on m-c, we can back out bug 1146294 on cypress. Wander I'm
> needinfoing you for visibility, as you will need to change the call to
> |./config.sh aries| to |LIGHTSABER=1 ./config.sh aries|.

Ok, np.

Comment 13

[Doug Sherk (:drs) (inactive)]

Attachment: Add lightsaber custom configs to b2g.js, guarded by MOZ_DEV_EDITION definition.

Comment 14

[Doug Sherk (:drs) (inactive)]

Attachment: Guard against MOZ_DEV_EDITION code running on B2G.

Benjamin, this is needed because we're re-using the MOZ_DEV_EDITION flag for B2G, but we don't want the special profile code to run in this situation. This is more of a precaution than anything.

Comment 15

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8584173 [details] [diff] [review]
Guard against MOZ_DEV_EDITION code running on B2G.

Oops!

Comment 16

[Doug Sherk (:drs) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d2d6c1546d5

Comment 17

[Benjamin Smedberg]

Comment on attachment 8584173 [details] [diff] [review]
Guard against MOZ_DEV_EDITION code running on B2G.

It would be really nice if bugs reports like this at least linked to a project page describing "lightsaber". I have no clue what this project is. But this change seems obvious-enough.

Comment 18

[Dave Hylands [:dhylands]]

Comment on attachment 8584172 [details] [diff] [review]
Add lightsaber custom configs to b2g.js, guarded by MOZ_DEV_EDITION definition.

Review of attachment 8584172 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ +1135,5 @@
> +#ifdef MOZ_DEV_EDITION
> +pref("javascript.options.discardSystemSource", false);
> +pref("dom.webcomponents.enabled", true);
> +pref("app.update.url", "https://aus4-dev.allizom.org/update/3/%PRODUCT%/%VERSION%/%BUILD_ID%/%PRODUCT_DEVICE%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml");
> +#endif

nit: Since this is overriding app.update.url which is defined elsewhere in this file, there should probably be a comment indicating that this has to come after that definition.

Briefly explaining why we need to override this is probably worthwhile too.

Comment 19

[[:fabrice] Fabrice Desré]

That never landed anywhere as far as I can see. Do we still need that bug?

Comment 20

[[:fabrice] Fabrice Desré]

We're doing bug 1157893 instead.