Closed Bug 1363533 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Bug 1363533 - Move nsIJSInspector from devtools/server/ to devtools/platform/.
59 bytes, text/x-review-board-request
jryans
: review+
gps
: review+
Details
Bug 1363533 - Introduce MOZ_DEVTOOLS=addon to allow building Firefox without DevTools.
59 bytes, text/x-review-board-request
jdescottes
: review+
jryans
: review+
gps
: review+
Details 
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 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.)
(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 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)
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.
Blocks: dt-addon-uishim
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+
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 #8866073 - Attachment is obsolete: true
Attachment #8866073 - Flags: review?(jdescottes)
Attachment #8866736 - Attachment is obsolete: true
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 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+
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)
Attachment #8866793 - Flags: review?(gps)
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
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 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 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 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.
Blocks: 1364876
Attachment #8866794 - Attachment is obsolete: true
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.
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: nobody → poirot.alex
Have you tried a full build, or only artifact builds?  You might need a full build to cover these changes.
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 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 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+
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.
Finally figured it out, I was missing
  firefox-appdir = browser
in xpcshell.ini
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
https://hg.mozilla.org/mozilla-central/rev/40eda96da608
https://hg.mozilla.org/mozilla-central/rev/ebb6631b83fc
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: