On Fri, Aug 16, 2013 at 05:43:22PM +0800, Andreas Gal wrote: > > First of all, thanks for raising this. Its definitely a problem that > needs fixing. > > I am not convinced by your approach though. In a few months from now > disabling WebRTC is like calling for the DOM or JS or CSS to be > disabled in local developer builds. It will become a natural part of > the core Web platform.
I'm not running my local builds to browse the web. I bet a vast majority don't. I don't think what we build for local developer builds should be driven by what defines the web, but what brings useful results to developers in a timely manner. If disabling non central features can allow to build significantly faster, it's worth at least talking about it. No amount of widespread-ness on the web is going to make webrtc as central as js, layout, or gfx. If default is not an option, then maybe we can have --disable-all-the-things subsequently allowing opt-ins like --enable-webrtc. But I like to think that most people shouldn't have to edit a .mozconfig. > I would like to propose the opposite approach: > > - Remove all conditional feature configuration from configure. > WebRTC et al are always on. Features should be disabled dynamically > (prefs), if at all. > - Reduce configure settings to choice of OS and release or developer. With my Debian hat on, let me say this is both x86/x86-64/arm and Firefox/Firefox-OS centric. There are features that don't build on non mainstream architectures (hey webrtc, i'm looking at you), and while I do understand the horror it can be to some people that there could be a Firefox (^WIceweasel) build that doesn't support all the web, it's still better to have a browser that doesn't support everything than no browser at all (and considering i get bug reports from people using or trying to use iceweasel on e.g ppc or ia64, yes, there *are* people out there that like or would like to have a working browser, even if it doesn't make coffee). And Gecko is not used only to build web browsers (for how long?), so it makes sense for some features to be disableable at build time. > - Require triple super-reviews (hand signed, in blood) for any > changes to configure. > - Make parts of the code base more modular and avoid super include > files cross modules (hello LayoutUtils.h). > > Rationale: > > Its not slow for you to build WebRTC. Its slow for you to have it > build over and over. Almost every time I pull from mozilla-central, > someone touched configure and I have to rebuild from scratch, which > is infuriating (argh!). Minimizing changes to configure and banning > static defines for feature management would solve that. If we make > sure major subsystems like WebRTC can stand on their own, you will > take a hit building it one time, and then occasionally as the team > lands changes. Its a pretty small team, so the amount of code they > can possibly check in is actually pretty small. You will see churn > all over the three when you pull, but you won't have to rebuild the > entire Universe every time you pull. With my build-config peer hat on, let me say this may not be as much a problem as you feel it is. At least not one that requires drastic restrictive measures. There are three different things that make configure changes a pain: - Re-running configure even with no changes, changes some central files, and that means rebuilding a lot of things. I recently fixed bug 903341 (guess what, running configure meant rebuilding all webrtc), and bug 903321 (running configure meant rebuilding all js). There remains bug 903369, which makes us rebuild FFI, which makes us relink the js library and everything that statically links against it, including many tests and libxul. Another one that is yet to file is new and similar: running configure runs ICU's configure which makes us rebuild ICU, relinking the js library, and so on. When I get 903369 to work on windows, it will be easy to fix ICU in a similar way. - Adding or removing a AC_DEFINE in configure means modifying mozilla-config.h, which is included in every single file. Adding or removing an AC_DEFINE means rebuilding the entire tree. Now you know why I'm always reluctant when i see people adding an AC_DEFINE for something that is #ifdef'ed in 3 different directories. bug 902825 is on file about this. Note we're not adding or removing AC_DEFINEs /that/ often. - Adding or removing a AC_SUBST in configure means modifying autoconf.mk if the added variable has a value (if the variable doesn't have a value, it ends up in emptyvars.mk, which is treated differently). Autoconf.mk is essentially made a dependency of everything, so a change in autoconf.mk is going to rebuild everything. emptyvars.mk isn't. And now that I'm writing this, I'm thinking of an obvious solution to the adding/removing AC_SUBST problem: for features enabled by default, instead of landing AC_SUBST(ENABLE_FEATURE), land AC_SUBST(DISABLE_FEATURE). That would make the variable empty by default, not changing autoconf.mk. That's something we can enforce at review time, and any change to configure.in should already be going through build config peers. Mike _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
