Merge directories and modules in gfx/

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: zwol, Assigned: zwol)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 8 obsolete attachments)

24.79 KB, patch
zwol
: review+
Details | Diff | Splinter Review
11.94 KB, patch
zwol
: review+
Details | Diff | Splinter Review
44.85 KB, patch
zwol
: review+
Details | Diff | Splinter Review
4.75 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
49.39 KB, patch
zwol
: review+
Details | Diff | Splinter Review
7.44 KB, patch
zwol
: review+
Details | Diff | Splinter Review
12.80 KB, patch
joe
: review+
Details | Diff | Splinter Review
Spun off from bug 266236 comment 45:

> I'm starting to wonder if we shouldn't first collapse gfx/src/thebes (and maybe
> also gfx/thebes) into gfx/src before doing this deCOM.  Then there would be
> only one libgkgfx.so, it would not be in components/, but it would nonetheless
> register components (this has to be possible somehow) and we wouldn't have
> these linkage problems.  This would also pave the way to deCOMing
> nsIDeviceContext, which seems like the next logical step.
My plan here, for the record, is to collapse gfx/src, gfx/public, and gfx/idl entirely into gfx/thebes.  This is consistent with all the other top-level directories in gfx/ being modules (eg. ipc, cairo, harfbuzz, layers).

This patch is somewhat tangential, but I don't want psshared hanging around to complicate matters, so this folds it into its sole remaining user, widget/src/gtk2.  Probably most if not all of that code is unnecessary now but we can take care of that in bug 394413, one of these years.
Attachment #451154 - Flags: review?(karlt)
I would prefer gfx/src to not go into gfx/thebes, but instead go into something like gfx/legacy, gfx/rutabaga, or whatever -- it's the glue that links thebes with nsIRenderingContext, DeviceContext, etc., and we keep talking that at some point we'd like to get rid of it.  The merging will be helpful to get us to that point.
I'm happy to leave the contents of gfx/thebes alone and just merge src/thebes, src/, public/, idl/.  That still unjams us on bug 266236.
What should be done with gfx/src/utils, in your opinion?
Attachment #451183 - Flags: review?(vladimir)
Re part 2 I should have mentioned that the patch summarily deletes gfx/idl/geniid.pl, which is not used by the build, has never been modified since it was originally checked into CVS in 1998, and appears to be someone's script for a one-time mass definition of UUIDs -- in other words, I doubt it should have been committed at all.
Posted patch part 3: flatten gfx/thebes (obsolete) — Splinter Review
This just gets rid of the public/src split inside gfx/thebes.  I considered mashing all the test subdirectories together as well, but that gets fiddly because of C++ unit tests that can't be run in a libxul build, so it doesn't seem worth it.
Attachment #451186 - Flags: review?(vladimir)
This moves everything from gfx/src/thebes into gfx/src.

This is the piece I need for bug 266236 (see discussion there) but it doesn't work as is.  Everything compiles, but the firefox binary crashes on startup because the components defined by nsThebesGfxFactory.cpp are not registered.  That used to happen because gfx/src/thebes/Makefile.in defined IS_COMPONENT=1, but gfx/src/Makefile.in does not do that, and we cannot simply turn that on, because then libgkgfx.so won't be available for various other pieces to link against (again, see bug 266236).

So what we need here is a way for libgkgfx.so to manually register its components.  I am sure this is possible ( both libgklayout and libxpcom_core logically must be doing it), but I do not know how.  cc:ing bsmedberg, hopefully he knows.
Attachment #451188 - Flags: feedback?(vladimir)
Attachment #451188 - Flags: feedback?(benjamin)
also cc:ing Mossop, who told me in person that this should probably not land until after the component registration rewrite.  I am fine with that.
Hmm, I'm not sure that I like flattening gfx/thebes/* to gfx/thebes.  Anyone else have any opinions?  On one hand, it'll be nice not having to ../public/ to edit a header file, on the other it's a significant change to how other modules are structured (not in gfx, though many of the things in gfx are imports of external code modules).
We've been flattening X/{src,public,idl} into X in layout for a while now.  I like it because of not having to go somewhere else for a header file, and because it cuts down on recursive make invocations.
I think we should flatten. We've been doing it elsewhere and it seems to have been a win.
Comment on attachment 451186 [details] [diff] [review]
part 3: flatten gfx/thebes

ok, good enough for me
Attachment #451186 - Flags: review?(vladimir) → review+
Attachment #451154 - Flags: review?(karlt) → review+
I'm landing parts 1-3 of this shortly.  All required a bit of merging, but nothing complicated (there were e.g. some new files added to gfx/public recently)
Attachment #451154 - Attachment is obsolete: true
Attachment #454547 - Flags: review+
Attachment #454548 - Flags: review+
Attachment #451183 - Attachment is obsolete: true
Attachment #451186 - Attachment is obsolete: true
Attachment #454549 - Flags: review+
This is a new part 4 which consolidates all of the tests below gfx/ into gfx/tests.  The old gfx/tests/coverage directory is deleted - it wasn't being built, and as far as I can tell, has not done anything useful ever.  The programs formerly in gfx/thebes/test are now in gfx/tests with the other C++ unit tests that were already there, but are still not run by "make check".

There are still three subdirectories of gfx/tests (for crashtests, mochitests, and xpcshell-tests) but there is no Makefile recursion below gfx/tests.
Attachment #451188 - Attachment is obsolete: true
Attachment #454735 - Flags: review?(vladimir)
Attachment #451188 - Flags: feedback?(vladimir)
Attachment #451188 - Flags: feedback?(benjamin)
Moves everything formerly in gfx/src/thebes/utils into gfx/thebes.  It is possible that nsCoreAnimationSupport.mm should be renamed gfxCoreAnimationSupport.mm with this, but I didn't.

I kicked off try server builds with "new part 4" and with "new part 4" + "new part 5", which so far are all green.  The old part 4 will return as a part 6 once bsmedberg's component manager rewrite lands and I find out how to make libgkgfx.so do manual component registration.  Unfortunately, that is the piece which is the goal of this bug.
Attachment #454737 - Flags: review?(vladimir)
zwol, can you update "toolkit-makefiles.sh" to no longer refer to the directories you've flattened, too?

Right now it does, and I get this build-spam as a result (when I blow away my objdir and build fresh m-c):
> creating gfx/idl/Makefile
> can't read ../mozilla/gfx/idl/Makefile.in: No such file or directory
> creating gfx/public/Makefile
> can't read ../mozilla/gfx/public/Makefile.in: No such file or directory
> creating gfx/src/psshared/Makefile
> can't read ../mozilla/gfx/src/psshared/Makefile.in: No such file or directory
> creating gfx/thebes/public/Makefile
> can't read ../mozilla/gfx/thebes/public/Makefile.in: No such file or directory
> creating gfx/thebes/src/Makefile
> can't read ../mozilla/gfx/thebes/src/Makefile.in: No such file or directory
So, like this?  (Also strips some stale references in comments and docs elsewhere in the tree.)

I am not clear on what _needs_ to be in toolkit-makefiles.sh.  There are several gfx/ directories (presently) with their own makefiles that are not listed there, and it seems to just create them when it gets to them.
Attachment #455012 - Flags: review?(dholbert)
Comment on attachment 455012 [details] [diff] [review]
fix stale references from toolkit-makefiles.sh

I'd assumed that all Makefile.in files needed to be listed there in order to get their makefile generated, but I'm not 100% sure. (and I don't know specifically why the absence of the other Makefiles you mentioned wouldn't cause problems. Those ones must be built some other way, I guess.)

In any case, though, there's a blanket rs=ted on removing stale directories from toolkit-makefiles.sh (since this is a problem that crops up frequently when directories are removed).  So this fix definitely does the right thing for the directories involved.
Attachment #455012 - Flags: review?(dholbert) → review+
allmakefiles.sh is a performance optimization when configuring. If you don't include a directory, it has to use the much slower makefile-generation path when building.
Couldn't we run (the equivalent of) 'find . -name Makefile.in' at the end of configure, rather than having this list that has to be kept up to date manually?  It presumably does not matter if a few Makefiles get generated even though they won't be used.  I suppose we would need a blacklist for things like js and nspr that have their own configure scripts, but that would be easier to keep up to date.
Landed the toolkit-makefiles.sh patch: http://hg.mozilla.org/mozilla-central/rev/d5d4a04727a4
Depends on: 576074
Comment on attachment 454735 [details] [diff] [review]
new part 4: reorganize gfx/ test directories

FYI, zwol and vlad, my patch in Bug 576074 slightly bitrots this (changes one line in a removed file) and this patch OR my patch should fix Bug 576074, though the patch I made should be a much faster review.
Comment on attachment 454735 [details] [diff] [review]
new part 4: reorganize gfx/ test directories


>diff --git a/gfx/Makefile.in b/gfx/Makefile.in
>--- a/gfx/Makefile.in
>+++ b/gfx/Makefile.in
>@@ -62,14 +62,12 @@ ifdef BUILD_STATIC_LIBS
> DIRS		+= ycbcr
> endif
> 
> ifdef MOZ_IPC
> DIRS		+= ipc
> endif
> 
> ifdef ENABLE_TESTS
>-ifndef MOZ_ENABLE_LIBXUL
> TOOL_DIRS	+= tests
> endif
>-endif

Do these tests build with libxul?
Attachment #454735 - Flags: review?(vladimir) → review+
Comment on attachment 454737 [details] [diff] [review]
new part 5: gfx/src/thebes/utils/* move to gfx/thebes

I'm not jumping up and down with joy that we don't have a separation between thebes and utils any more, but I don't feel very strongly about it.
Attachment #454737 - Flags: review?(vladimir) → review+
Slight rediffing required on this one due to changes in deleted files; also, the component-manager landing seems to have made the explicit initialization/free of XPCOM bits in TestColorNames and TestRegion not only redundant but crashy, so I removed that.
Attachment #454735 - Attachment is obsolete: true
Attachment #457158 - Flags: review+
No changes of substance in this piece, just refreshed.
Attachment #454737 - Attachment is obsolete: true
Attachment #457159 - Flags: review+
Here now again is the piece I need for bug 266236.  (N.B. I haven't reactivated those patches yet, so I'm not *sure* this fixes the link problems in that bug, but it *should*.)

What happens: everything in gfx/src/thebes moves up to gfx/src, and *all* of the remaining sub-libraries in gfx/ (except cairo, 'cos of that possibly coming from the OS) are merged into one libgkgfx.so, which is a component.  This then has knock-on effects all over the tree: anywhere that referred to gkgfx or thebes in its EXTRA_DSO_LIBS must instead write

    $(call EXPAND_LIBNAME_PATH,gkgfx,$(DEPTH)/gfx/src)

in its EXTRA_DSO_LDOPTS.  This has been confirmed to build and smoke-test successfully on Linux in both --disable-libxul and --enable-libxul configurations.  I pushed it to the try server twice, once with no custom mozconfig, and once with --disable-libxul in a custom mozconfig.  I don't know if the latter will work at all, but if it does, it should flush out --disable-libxul bugs on Windows and OSX (which are highly probable).

Asking for review from both Vlad (for gfx) and Ted (for build system).
Attachment #457463 - Flags: review?(vladimir)
Attachment #457463 - Flags: review?(ted.mielczarek)
This revision updates for the addition of ANGLE to the tree, and makes some fixes with which I get green try server results cross-platform ... with --enable-libxul.  As the entire point here is to make --disable-libxul work again with bug 266236, that's not good enough.  And, unfortunately, while this patch + --disable-libxul works fine on my local desktop (x64 Linux) it fails at the try server on every builder tbpl sees, including Linux.  Some of the failures may not be this patch's fault - I'm going to queue a no-changes, disable-libxul-forced try server build and see what happens.

The failures on Linux and Windows look like they are probably fixable with sufficient dinking with the Makefiles, but the failure on OSX is more serious: apparently on OSX you cannot build a shared object which is simultaneously linkable and dlopen-able!  And we need exactly that, at least with the approach taken here -- libgkgfx.so has to be both a component and a library for other libraries to link against.

I'm not going to try to debug this any more right now.  I would really appreciate feedback from build and XPCOM folks about whether or not this has any chance of working, and if not, what to do instead.

Here are failure logs:
Linux Bo: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303336.1279305303.26158.gz
Linux64 Bo: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303335.1279305180.25617.gz
OSX Bo: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303336.1279305483.27026.gz
OSX Bd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303335.1279305492.27037.gz
OSX64 Bo: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303336.1279305483.27028.gz
OSX64 Bd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303499.1279305484.27021.gz
W2003 Bo: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303335.1279306480.31390.gz
W2003 Bd: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303336.1279305969.29365.gz
Android Bo: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303335.1279304860.24190.gz
Maemo5 Bo1: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303335.1279304679.23514.gz
Maemo5 Bo2: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303336.1279305483.27022.gz
Maemo4 Bo: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279303335.1279305008.24747.gz
Attachment #457463 - Attachment is obsolete: true
Attachment #457931 - Flags: review?(vladimir)
Attachment #457931 - Flags: review?(ted.mielczarek)
Attachment #457463 - Flags: review?(vladimir)
Attachment #457463 - Flags: review?(ted.mielczarek)
(In reply to comment #24)
> Couldn't we run (the equivalent of) 'find . -name Makefile.in' at the end of
> configure, rather than having this list that has to be kept up to date
> manually?  It presumably does not matter if a few Makefiles get generated even
> though they won't be used.  I suppose we would need a blacklist for things like
> js and nspr that have their own configure scripts, but that would be easier to
> keep up to date.

I filed this as bug 579494.
I guess we should just hold off on this until we kill non-libxul builds. We're not that far off.
Attachment #457931 - Flags: review?(ted.mielczarek)
(In reply to comment #35)
> I guess we should just hold off on this until we kill non-libxul builds. We're
> not that far off.

Agreed.
Posted patch new part 6Splinter Review
Now that we don't have to worry about non-libxul builds anymore, here is an updated version of the last patch for this bug.  I hope we can get this reviewed and landed quickly so I can get back to bug 266236.
Attachment #457931 - Attachment is obsolete: true
Attachment #523024 - Flags: review?(joe)
Attachment #457931 - Flags: review?(vladimir)
Comment on attachment 523024 [details] [diff] [review]
new part 6

You can remove the WINCE stuff altogether. Looks good as long as it passes try!
Attachment #523024 - Flags: review?(joe) → review+
(In reply to comment #38)
> 
> You can remove the WINCE stuff altogether. Looks good as long as it passes try!

It does.  WINCE block removed in my copy, will reupload after landing.
http://hg.mozilla.org/mozilla-central/rev/ff28c9aab04c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.