Closed Bug 1148062 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: General, defect, P1)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: drs, Assigned: drs)

References

Details

(Whiteboard: [spark])

User Story

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.

a. To do this, we will have to change a series of configs in 'b2g/app/b2g.js' if 'MOZ_DEV_EDITION' is defined.
b. Guard against a small amount of code in 'toolkit/profile/nsToolkitProfileService.cpp' and 'toolkit/xre/nsAppRunner.cpp' [1] that is normally built when 'MOZ_DEV_EDITION' is defined.

[1] Note that we are consciously allowing the code in 'modules/libpref/init/all.js' to be included in our build, as it's useful for a developer device.

Attachments

(2 files, 4 obsolete files)

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|.
Flags: needinfo?(wcosta)
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.
Attachment #8584069 - Flags: review?(mh+mozilla) → review-
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 on attachment 8584073 [details] [diff] [review]
Add LIGHTSABER build-time exported flag.

Dropping review and obsoleting based on comment 7.
Attachment #8584073 - Attachment is obsolete: true
Attachment #8584073 - Flags: review?(mwu)
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.
Attachment #8584074 - Attachment is obsolete: true
Attachment #8584074 - Flags: review?(mwu)
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.
Attachment #8584070 - Attachment is obsolete: true
Attachment #8584070 - Flags: review?(dhylands)
I'm going to use the MOZ_DEV_EDITION flag instead, as suggested by Mike.
(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.
Flags: needinfo?(wcosta)
Summary: [Build] Add Lightsaber configs to Gecko, guarded by the `LIGHTSABER` B2G config flag → [Build] Add Lightsaber configs to Gecko, guarded by the `MOZ_DEV_EDITION` build-time flag
User Story: (updated)
Product: Firefox OS → Core
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.
Attachment #8584173 - Flags: review?(benjamin)
Attachment #8584173 - Attachment filename: bug-1148062-gonk → Guard against MOZ_DEV_EDITION code running on B2G.
Comment on attachment 8584173 [details] [diff] [review]
Guard against MOZ_DEV_EDITION code running on B2G.

Oops!
Attachment #8584173 - Attachment description: bug-1148062-gonk → Guard against MOZ_DEV_EDITION code running on B2G.
Attachment #8584173 - Attachment filename: Guard against MOZ_DEV_EDITION code running on B2G. → bug-1148062-gonk
See Also: → 1146294
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.
Attachment #8584173 - Flags: review?(benjamin) → review+
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.
Attachment #8584172 - Flags: review?(dhylands) → review+
That never landed anywhere as far as I can see. Do we still need that bug?
Priority: -- → P1
Whiteboard: [lightsaber] → [spark]
We're doing bug 1157893 instead.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
See Also: → 1159422
Blocks: spark-build
No longer blocks: spark
You need to log in before you can comment on or make changes to this bug.