Setup a build configuration to build firefox without built-in DevTools

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 months ago
In order to ease testing the upcoming setup where mozilla-central doesn't ship DevTools, we should introduce a build configuration allowing to build Firefox without DevTools.

MOZ_DEVTOOLS variable could be an option.
Today it can be set to: server or all.
In order to ship only the server part, or everything.
We could introduce a third option: "addon", which would only ship binary components and any shim module that is meant to stay on mozilla-central.
Comment hidden (mozreview-request)
Comment on attachment 8866073 [details]
Bug 1363533 - Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools.

https://reviewboard.mozilla.org/r/137672/#review140836

At a high level, I think this seems like a resonable approach so you can start to move to the configuration you're expecting.

However, it might be simpler if you move these native pieces to a location like `devtools/platform` or something.  Then you'd modify `devtools/moz.build` so that in `addon` mode, it builds only that dir (the existing modes would also include it).  That way the changes are contained to only `devtools/moz.build`, and you wouldn't need to list out each native / platform thing you want to include.

I assume you need to leave these things behind in m-c somewhere anyway, so it probably makes sense for them to move somewhere other than where they are now.

Where are you expecting the shim modules to live?  That might also help clarify things.

When you move DevTools code to GitHub, will there still be a /devtools directory in m-c at all?  (I am curious what the final location of platform things would look like after the code moves.)
(Assignee)

Comment 3

7 months ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> However, it might be simpler if you move these native pieces to a location
> like `devtools/platform` or something.  Then you'd modify
> `devtools/moz.build` so that in `addon` mode, it builds only that dir (the
> existing modes would also include it).  That way the changes are contained
> to only `devtools/moz.build`, and you wouldn't need to list out each native
> / platform thing you want to include.

Yes I thought about moving heapsnapshot to js/ or somewhere in platform as it sounds closer to that rather than DevTools.

> Where are you expecting the shim modules to live?  That might also help
> clarify things.

We are about to introduce some new code that is going to stay in mozilla-central:
 - compatibility shim, replacing gDevTools.jsm (bug 1356244)
   Julian suggests to put them in /devtools/shim/
   I suggested to put them at /devtools/
 - new startup components which triggers the install UI (bug 1361080)
   I originaly put it at /devtools/, but could move them into /devtools/shim as well
 
> When you move DevTools code to GitHub, will there still be a /devtools
> directory in m-c at all?  (I am curious what the final location of platform
> things would look like after the code moves.)

Yes. To keep things simple.
The D-Day, we would just toggle the MOZ_DEVTOOLS to "addon" in /browser/confvars.sh
Once we are confident the move to github is stable (i.e. we won't backout this change),
we can remove all devtools files that are now on github. (all but shim and c++)
And after that, we can shuffle things around in m-c.
We could imagine moving the shim to /browser/ and C++ to js/, but tbh, I don't see a big value in doing this.
We should just ensure that /devtools folder is simple, clear and isn't complex just for historical reasons.

Here is what I had in mind as final layout, post file removal.
/devtools/moz.build
/devtools/README.MD (explain add-on + github story)
/devtools/DevToolsShim.jsm (compatiblity shim)
/devtools/devtools-startup.js (startup xpcom, addon-installation)
/devtools/InstallDialog.{jsm,xhtml,js,css} (to come, define the addon installation dialog, may be hosted online)
/devtools/heapsnapshot/

We could add a /devtools/shim/ folder to ease comprehension in today's layout.
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Thanks, that helps!  So I'd say make devtools/platform for the native bits you want to keep in all modes, and a devtools/shim for the new shim layer.

That should make it easier to follow what's included where, and the moz.build changes should change to just whichever directories you need in each case.

I agree you could move native bits to /js, the shim to /browser, etc., but leaving them in subdirs of /devtools seems fine too.  I don't have a preference either way myself.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

7 months ago
Comment on attachment 8866073 [details]
Bug 1363533 - Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools.

I introduce empty moz.build in platform and shim folder.
Second patch starts contributing to platform one,
while shim should be modifed by bug 1361080 or bug 1356244 (whatever lands first)
(Assignee)

Comment 10

7 months ago
Comment on attachment 8866736 [details]
Bug 1363533 - Move heapsnapshot native components to /devtools/platform/heapsnapshot.

I'll move this patch as followup as I expect more discussion about the js dependencies of this code and its tests.
(Assignee)

Updated

7 months ago
Blocks: 1361080
(Assignee)

Updated

7 months ago
Blocks: 1356244
Comment on attachment 8866073 [details]
Bug 1363533 - Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools.

https://reviewboard.mozilla.org/r/137672/#review141586

Thanks, looks good to me!
Attachment #8866073 - Flags: review?(jryans) → review+
Comment on attachment 8866735 [details]
Bug 1363533 - Move nsIJSInspector from devtools/server/ to devtools/platform/.

https://reviewboard.mozilla.org/r/138342/#review141588

Looks good! :)
Attachment #8866735 - Flags: review?(jryans) → review+
(Assignee)

Comment 13

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce4bcf33d56972aa170c93255928f2c5907a499e
Comment on attachment 8866735 [details]
Bug 1363533 - Move nsIJSInspector from devtools/server/ to devtools/platform/.

https://reviewboard.mozilla.org/r/138342/#review141588

Looks good! :)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8866073 - Attachment is obsolete: true
Attachment #8866073 - Flags: review?(jdescottes)
(Assignee)

Updated

7 months ago
Attachment #8866736 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8866793 [details]
Bug 1363533 - Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools.

https://reviewboard.mozilla.org/r/138400/#review141598
Attachment #8866793 - Flags: review?(jryans) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

7 months ago
mozreview-review
Comment on attachment 8866793 [details]
Bug 1363533 - Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools.

https://reviewboard.mozilla.org/r/138400/#review141608
Attachment #8866793 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 23

7 months ago
Comment on attachment 8866735 [details]
Bug 1363533 - Move nsIJSInspector from devtools/server/ to devtools/platform/.

Hi gps, do you mind looking at these moz.build changes?

The story here is to use MOZ_DEVTOOLS=addon to build firefox without devtools.
That to anticipate the upcoming "devtools as an add-on" project.
But we have to keep just a few bits related to devtools:
* any c++/native code, like heapsnapshot
* some glue code, here called "shims", that will connect mozilla-central/firefox codebase with the add-on.
Attachment #8866735 - Flags: review?(gps)
(Assignee)

Updated

7 months ago
Attachment #8866793 - Flags: review?(gps)

Comment 24

7 months ago
mozreview-review
Comment on attachment 8866793 [details]
Bug 1363533 - Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools.

https://reviewboard.mozilla.org/r/138400/#review141990

::: devtools/moz.build:31
(Diff revision 2)
> -    'server',
> +        'server',
> -    'shared',
> +        'shared',
> -]
> +    ]
> +else:
> +    DIRS += [
> +        'shim',

I didn't pay enough attention to the changes in the 2nd revision.

My initial idea was to put the about:debugging registration code under shim/ as I thought it would be processed in any case. This way I could get rid of the current registration of about:debugging in nsAboutRedirector.cpp and and nsDocShellModule.cpp
(Assignee)

Comment 25

7 months ago
I went back and forth with that, I originaly put it next to platform, so always included.
But move it there for devtools-startup.js needs, I can move this if MOZ_DEVTOOLS==addon within shim/moz.build.

Comment 26

7 months ago
mozreview-review
Comment on attachment 8866793 [details]
Bug 1363533 - Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools.

https://reviewboard.mozilla.org/r/138400/#review142270

Usually we don't add moz.build files until they are necessary. Why do you add them in this commit? Also, AFAICT the "shim" directory is empty at the end of this series. Or am I missing something?
Attachment #8866793 - Flags: review?(gps)

Comment 27

7 months ago
mozreview-review
Comment on attachment 8866735 [details]
Bug 1363533 - Move nsIJSInspector from devtools/server/ to devtools/platform/.

https://reviewboard.mozilla.org/r/138342/#review142272

::: devtools/platform/tests/unit/test_nsjsinspector.js:1
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.

This file seems to have lost its rename annotation.
Attachment #8866735 - Flags: review?(gps)

Comment 28

7 months ago
mozreview-review
Comment on attachment 8866794 [details]
Bug 1363533 - Move heapsnapshot native components to /devtools/platform/heapsnapshot.

https://reviewboard.mozilla.org/r/138402/#review142274

This series seems pretty reasonable to me from a build system perspective.
(Assignee)

Updated

7 months ago
Blocks: 1364876
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8866794 - Attachment is obsolete: true
(Assignee)

Comment 31

7 months ago
I removed the empty moz.build, it was to help understanding where I'm going with this new flag.
I also removed heapsnapshot for a dedicated followup.

But there is something broken, still. 
I can't understand why nsIJSInspector interface isn't registered.
I thought it was about missing modification to dumbmake-dependencies, but no.
This interface is still missing. If you have any idea... Components.interfaces.nsIJSInspector is undefined.
(In reply to Alexandre Poirot [:ochameau] from comment #31)
> I removed the empty moz.build, it was to help understanding where I'm going
> with this new flag.
> I also removed heapsnapshot for a dedicated followup.
> 
> But there is something broken, still. 
> I can't understand why nsIJSInspector interface isn't registered.
> I thought it was about missing modification to dumbmake-dependencies, but no.
> This interface is still missing. If you have any idea...
> Components.interfaces.nsIJSInspector is undefined.

Did the .xpt change paths[1]?

Maybe you need "@RESPATH@/components/jsinspector.xpt" instead of "@RESPATH@/browser/components/jsinspector.xpt"?

[1]: http://searchfox.org/mozilla-central/source/browser/installer/package-manifest.in#253
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #32)
> Did the .xpt change paths[1]?
> 
> Maybe you need "@RESPATH@/components/jsinspector.xpt" instead of
> "@RESPATH@/browser/components/jsinspector.xpt"?

It still _seems_ like the old path should be correct, since devtools/moz.build is still setting `DIST_SUBDIR = 'browser'`...

Anyway, I'd say inspect your objdir/dist directory to see where files like the .xpt are ending up.
(Assignee)

Comment 34

7 months ago
The xpt path doesn't change. I tried to do some diff on the objdir and can't find anything significantly different :/
I tried chanding the IDs in the idl and c++, but got the same result.
(Assignee)

Updated

7 months ago
Assignee: nobody → poirot.alex
Have you tried a full build, or only artifact builds?  You might need a full build to cover these changes.
(Assignee)

Comment 36

7 months ago
Full build, with wipping my objdir, this is really weird. Now I'm trying to move pieces bit by bit to see when it breaks.

Comment 37

7 months ago
mozreview-review
Comment on attachment 8866793 [details]
Bug 1363533 - Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools.

https://reviewboard.mozilla.org/r/138400/#review144356
Attachment #8866793 - Flags: review?(gps) → review+

Comment 38

7 months ago
mozreview-review
Comment on attachment 8866735 [details]
Bug 1363533 - Move nsIJSInspector from devtools/server/ to devtools/platform/.

https://reviewboard.mozilla.org/r/138342/#review144358
Attachment #8866735 - Flags: review?(gps) → review+

Comment 39

7 months ago
The browser/ prefix for the xpt comes from the export('DIST_SUBDIR') in devtools/moz.build. As long as the moz.build defining the XPIDL interface isn't having its DIST_SUBDIR reset by itself or in a calling moz.build, the xpt path should remain the same. Note that devtools/server/shims/toolkit/moz.build and devtools/shared/shims/moz.build reset DIST_SUBDIR to '', thus removing the browser/ prefix. But I don't think this comes into play here unless I'm missing something.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

7 months ago
Finally figured it out, I was missing
  firefox-appdir = browser
in xpcshell.ini

Comment 45

7 months ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40eda96da608
Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools. r=gps,jdescottes,jryans
https://hg.mozilla.org/integration/autoland/rev/ebb6631b83fc
Move nsIJSInspector from devtools/server/ to devtools/platform/. r=gps,jryans

Comment 46

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/40eda96da608
https://hg.mozilla.org/mozilla-central/rev/ebb6631b83fc
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.