Maybe someone could refresh my ailing memory as to why we are lumping 1619 headers into a single directory for our current builds? I thought it was because the mac couldn't handle the extra include paths but scc says that the mac is actually separating the headers based upon module. I want to move both the win32 & unix build back to old method of having a separate include directory for each module that is being used. Even resurrect REQUIRES if need be. I think this will help us move towards our goal of having independent modules by clearly defining module dependencies. Any thoughts?

I dimly recall the move to a single dist/include yielding a build speedup, but I could be wrong. Cc'ing waterson, who may still be guilty for doing away with REQUIRES. /be

I was responsible for this homogonizing headers. I did it because 1. We didn't have consistent support for the modularization on all build platforms, so it was a fairly common occurence (e.g., at least once a week) for the build to break because someone would add a new dependency between modules, but not update all the build environments correctly. 2. We would less frequently end up with bizarre situations in depend builds where a header was removed, or moved from one module to another module, but wouldn't get nuked from the "dist" directory. This would end up causing about a day's worth of confusion while some people's builds worked, but other's didn't. (This was the straw that finally broke my back and made me rip it out.) That said, warren hated me for it because it meant that it was easy for somebody to naively add bogo-dependencies between modules: all the headers are lumped into one big pot, so who knew that the necko module depended on the wallet module...until it was too late, for example. I'm skeptical that making this change is Going To Save Us. If you do it, please: 1. Open the issue for general discussion on n.p.m.builds before doing anything. If people agree it's The Right Thing, then... 2. Implement it consistently on all platforms (e.g., if Unix is going to have subdirs under $(DIST)/include, so should windows and mac) 3. Try do it in such a way that depend builds on Win32 and Mac (which copy the files; yeah, sure, Mac uses aliases, but aliases can point into the trash can) are not susceptible to the "'cvs remove'-d file can still be #included" problem.

> That said, warren hated me for it because it meant that it was easy for somebody > to naively add bogo-dependencies between modules: all the headers are lumped > into one big pot, so who knew that the necko module depended on the > wallet module...until it was too late, for example. That's my primary reason for wanting to make the change. Ask dougt about our wacky header dependencies sometime. As it is, you cannot build the subset of the tree needed for embedding without pulling the entire tree. This proposed change won't solved that problem but by making dependencies more explicit, it should help us avoid the problem in the future.

I had to make the following changes to my tree to get this to build: embedding/browser/webBrowser/Makefile.in ( add rdf ) embedding/browser/gtk/tests/Makefile.in ( add pref ) extensions/wallet/src/Makefile.in ( add unicharutil ) mailnews/base/util/Makefile.in ( add nkcache ) mailnews/news/src/Makefile.in ( add nkcache ) mailnews/news/build/Makefile.in ( add nkcache ) mailnews/local/src/Makefile.in ( add nkcache ) mailnews/local/build/Makefile.in ( add nkcache ) mailnews/compose/src/Makefile.in ( add nkcache ) mailnews/compose/build/Makefile.in ( add nkcache ) mailnews/imap/build/Makefile.in ( add nkcache ) r=blizzard I'm going to go unspank my tree now.

Well, I posted to m.builds and there seemed to be little concern about the changes (save Colin's concern about cmdline lengths). I also noted that the difference in build times before and after the patch were near neglible, but my build machines aren't exactly low-end. I think the fact that Blizzard uncovered more dependencies 3 days after the initial patch was made indicates that a change like this is needed. But since I have no plans to work on the windows build system (save scrapping it in favor of a gmake based one) these changes aren't going to get turned on by default any time soon. The updated build rules patch allows you to trigger the dependency tracking by setting MOZ_TRACK_MODULE_DEPS when running configure or in config/autoconf.mk. Once these changes are checked in, I plan to use this feature on at least one of the tinderboxes.

I must say that I really really really really don't want this to be turned on by default for mozilla develpers (or if it is, it must be easy to go back to the current system). cls, you said "Maybe someone could refresh my ailing memory as to why we are lumping 1619 headers into a single directory for our current builds?", and I must say, I don't know why, but it's damn convenient when coding, if you need to look at an interface you know exactly where it is, no need to start looking for it in lxr or in subdirectories in dist/include. Also, if you don't remember the exact name of an interface it's very convenient to just look for in in dist/include using your favorite shell/editor (i.e. start typing and use autocomplete...). To be honest, I personally don't care that much about module dependencies given the current over-all quality of the product, I put much higher value in making this puppy good, stable, small, fast, ... I wouldn't mind having a tinderbox build with this turned on so that we'd see if someone adds dependencies, but please don't turn this on for good for everyone, IMO it's a step backwards convenience wise for developers, and I value that very highly at this point in the develpment.

Oh oh, that 4k shell command line limit is looming nearer...

doing things right isn't always convenient. we're going to have to do this or something like this is we're ever going to be able to move to a modular build system. the longer we wait, the harder it's going to be to do and the more painful it will be to land. The only concern I have with landing this change is that it get announced and landed separately from the current pending libpr0n and other landings to avoid confusion amongst all the tree spankings.

What I'm seeing is that we are talking out of both sides of our collective mouths and neither side seems to be paying attention or caring what the other side is saying. On the one hand, we want to make the browser the best that it can be and will accept any conveniences that will further that goal. On the other hand, we want to make our codebase usable for everyone in the world by making it modular. In theory, we should be able to do both and it shouldn't require a major sacrifice of either goal to do so. The current system is convenient until you try to build a module by itself or you only want to build a subset of modules for a different purpose than Mozilla the browser+ (ie, embedding). At that point, the previously purported conveniences become a PITA as you try to avoid pulling and building every component in the known mozilla world just to test your tiny bit. I do not see how this proposed change would have any effect on making the browser "good, stable, small, & fast". If anything, it would help with the "good" part as interfaces & dependencies would be better defined and (maybe) developers would think twice before they added dependencies to other modules. I know that at least ccarlen has gone and removed some unnecessary dependencies since he knew about this option (primarily due to senna bustage). That said, there are no plans to land this change anytime soon (where soon is defined before 0.9). One of waterson's requirements is to make it work on win32 and that's not going to happen until we get win32 switched to autoconf (which generates a completely separate developer convenience discussion). Of course, if you want to punt on this whole modular build thing, that's ok too as it means I can mark about 1/3 of my bugs as invalid. ;)

cls: don't pay any attention to ``my requirements'', fool! :-) Seriously, I stand my remarks that this alone will probably not save us. But, I generally think that this is a step in the right direction for all the reasons you state. And a step in the right direction is probably better than a leap to the perfect system that never happens. (We will probably have more module dependency build bustage headaches introduced, but if this catches people with their pants down introducing bogo-dependencies, and gets 'em backed out, that's probably a win.) As I noted, windows and mac will each have their own interesting problems with the module directories so long as we use copying (which leaves a stale copy) or aliases (which can reach beyond the grave to find their targets), respectively. jst: c'mon, convenience (laziness?) is not a good reason. find mozilla/dist/include -name \*.h -print | xargs grep -n nsIWalletViewer And, like cls says, part of putting the ``small'' in the smallest, fastest, most standards compliant browser in the world (*cough*) is breaking some of the bogo-dependencies that have been introduced.

Yes, please, let's turn these on by default. Those of us who compile with them see multiple bogo-dependencies being introduced every week.

As I was pointing out in bug 92377, I think it's time that we start separating the module interfaces from the module implementations. This bug already takes the first step of actually separating the interfaces/public headers on a module basis. The next step would be to actually move the interfaces into a module agnostic directory (include/ ?). We could actually move (copy) the headers into include/<module> (CVS repo mucking) or we could just setup of the build system to generate that structure on the fly. The first option has the added benefit of removing the requirement that we pull major portions of the tree just to build a small subset. Either way, we are going to need to keep a master list of the files, like the mac does, so that we know which files to export and we can tell when files need to be removed from dist.

In order to know which headers should no longer be in dist/include, we need to make a running list of the currently "active" headers. This patch creates a "hidden" list under dist/include/<module>/ for each module during the export phase. At the end of the export phase, we run this purge script which will compare the files listed in the generated .headerlist files against the list of files currently in the directory. Files not in the generated list will be removed. The good news is that the patch seems to work great...so great that it was removing headers that weren't installed using the default rules in rules.mk. ;) I had to tweak a couple of makefiles to get around that. The bad news is that we're running an extra perl script about three hundred times during the export phase. On a dual p3-1ghz + Ultra 160 disks, it caused the "do-nothing" export phase time to go from 35s to 55s. I'm not sure how noticable the difference would be on less-hoss box. There are some mozLock.pm tweaks in that patch as well. I switched to using select() & fractions of a second for the timeout instead of 1 second when waiting for a lock to clear. This removed most of the locking overhead that I was seeing from these additional perl invocations (a -j2 build went *up* to 90s before the change).

Comment on attachment 49017 [details] [diff] [review] Purge old headers from dist/include at end of export phase Can't add nsBuildID.h to EXPORTS as nsinstall hasn't been built yet.

A comment on seperating interfaces vs. implementation: I honestly feel that this has very little value at this point in the game, because of the nature of our linking strategy. We don't do build-time linking of one DLL vs. another (aside from the 10-15 in the core executable) so if someone stupidly #includes a implementation class and tries to use it, then their dll won't compile, whether they're using REQUIRES or not. That said, I think that turning on REQUIRES is a great idea... which is why I'm adding it to the windows build system in bug 98371 As for the extra perl script, I think that it's not worth burdening every developer's daily build with having to run this script.. I think its just something that we should run periodically (i.e. every week or two) on the whole tree and purge those headers. I'll take a look at these patches today.

jag explained to me the value of the include-purging script. Now that I've thought about it a bit, I think I still object to adding it to the build system (though only enough to try to convince you :)) only because it's somewhat rare that we will be moving headers from one module to another... what if we just had a toplevel make target like "make purge-headers" that we could direct people to if they were having build problems?

oh, and the patch looks fine, so sr=alecf if you weren't convinced by my argument :)

Except that the "make purge-headers" solution won't fix the tinderboxes automatically. And really, the purge script takes no time at all to run. 98% of the overhead comes from calling the build-list.pl script to build up the .headerlist files. I'm attempting to make the transition seamless as possible. Outside of builds breaking when deps are unexpectantly introduced, there should be no other "gotchas" when we turn this on by default. The purge script will catch when we remove headers as well as move them. On unix, removing a header will cause a bogus link to hang around but on win32, you'll have an actual copy of the file that could cause problems.

Comment on attachment 49042 [details] Forgot to include purge script in last patch r/sr=alecf on the script minor nit, you're defining $listfile from @_ and then later assigning it a value.. Anyway, sounds

Comment on attachment 49035 [details] [diff] [review] Fix nsinstall-nsBuildID.h race & clean up purge script r=bryner

Comment on attachment 49042 [details] Forgot to include purge script in last patch r=bryner

FYI, the overall win32 build time increased from 47m to 53m on my Dell P3-650 laptop running w2k with all of the win32 REQUIRES patches applied and using the purge script. Also, I did run into a case where the headers aren't properly purged. If you completely rename a module, some old headers may be left behind with the old name because the .headerlist file is never created in dist/include/<oldmod> to remove the files. So you won't know which other modules are still using the oldname (if you haven't been paying attn) until you clobber dist/include. If you stagger the renaming, then it shouldn't be as big of a problem.

so the use of REQUIRES on win32 is probably causing most of that slowdown - nmake is so bad that I have to spawn out to perl in order to translate REQUIRES=foo bar into -I$(PUBLIC)\foo -I$(PUBLIC)\bar which means every directory with a REQUIRES= will run a short one line perl script. I wholeheartedly welcome any better solutions. nmake sucks.

Comment on attachment 49185 [details] [diff] [review] make win32 use the purge script as well r=jag

Comment on attachment 49185 [details] [diff] [review] make win32 use the purge script as well r=jag

Comment on attachment 49185 [details] [diff] [review] make win32 use the purge script as well sr=alecf

Comment on attachment 51095 [details] [diff] [review] Remove MOZ_TRACK_MODULE_DEPS ifdefs to turn it on by default nice! glad to see it so cleanly removed :) sr=alecf

I'm afraid this is a rather mundane thing, but I'm currently fixing galeon to work with the new include hierachy and I've run into a rather amusing problem. For various reasons, I use STL strings in one component so I #include <string> while at the same time I have -I$(PUBLIC)/string in my compile line, and the silly compiler b0rked on this declaring that: directory `/usr/include/mozilla/string' specified in #include I don't know if this is bad compiler behaviour (gcc 2.95.3) or a legitimate namespace collision issue. But, I thought I'd bring it up.

Seems to be the compiler. gcc3 doesn't exhibit this behaviour. I think it's likely that egcs will however.

OpenVMS build is broken. The problem is with this line: http://lxr.mozilla.org/seamonkey/source/config/rules.mk#886 I think: -I$(DIST)/include should be replaced by: $(INCLUDES) Otherwise we don't include from the new module include directories. And if we don't do that, then the build of xpcom/typelib/xpt/src/xpt_arena.c fails because we can't find xpt_arena.h.

I ran into this problem last night with my cross-compile build. I'll check in the patch when the tree opens.

I'm having the same problem as Philip when hacking nautilus to work with moz 095 g++ from gcc-2.96-85 on RedHat 7.1 It would probably be best if the string dir changed names, since it conflicts with the std C++ include of the name 'string' which was in existance first.

Open a seperate bug on the string module name change. Lacking a c++ std in front of me, I'd argue that it's really a gcc bug at this point.

Marking fixed.