571989 - Merge directories and modules in gfx/

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Zack Weinberg (:zwol)]

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.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: part 1: dump gfx/src/psshared/* into widget/src/gtk2

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.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 3

[Zack Weinberg (:zwol)]

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.

Comment 4

[Zack Weinberg (:zwol)]

What should be done with gfx/src/utils, in your opinion?

Comment 5

[Zack Weinberg (:zwol)]

Attachment: part 2: fold gfx/idl and gfx/public into gfx/src

Comment 6

[Zack Weinberg (:zwol)]

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.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: part 3: flatten gfx/thebes

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.

Comment 8

[Zack Weinberg (:zwol)]

Attachment: part 4: merge gfx/src/thebes into gfx/src (DOES NOT WORK)

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.

Comment 9

[Zack Weinberg (:zwol)]

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.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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).

Comment 11

[Zack Weinberg (:zwol)]

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.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should flatten. We've been doing it elsewhere and it seems to have been a win.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 451186 [details] [diff] [review]
part 3: flatten gfx/thebes

ok, good enough for me

Comment 14

[Zack Weinberg (:zwol)]

Attachment: patch 1 as landed

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)

Comment 15

[Zack Weinberg (:zwol)]

Attachment: part 2 as landed

Comment 16

[Zack Weinberg (:zwol)]

Attachment: part 3 as landed

Comment 17

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/0e46aa941e64
http://hg.mozilla.org/mozilla-central/rev/6a6fbf66e1e0
http://hg.mozilla.org/mozilla-central/rev/72b5eccbe07c

Comment 18

[Zack Weinberg (:zwol)]

Attachment: new part 4: reorganize gfx/ test directories

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.

Comment 19

[Zack Weinberg (:zwol)]

Attachment: new part 5: gfx/src/thebes/utils/* move to gfx/thebes

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.

Comment 20

[Daniel Holbert [:dholbert]]

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

Comment 21

[Zack Weinberg (:zwol)]

Attachment: fix stale references from toolkit-makefiles.sh

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.

Comment 22

[Daniel Holbert [: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.

Comment 23

[Benjamin Smedberg]

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.

Comment 24

[Zack Weinberg (:zwol)]

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.

Comment 25

[Zack Weinberg (:zwol)]

Landed the toolkit-makefiles.sh patch: http://hg.mozilla.org/mozilla-central/rev/d5d4a04727a4

Comment 26

[Justin Wood (:Callek)]

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 27

[Joe Drew (not getting mail)]

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?

Comment 28

[Joe Drew (not getting mail)]

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.

Comment 29

[Zack Weinberg (:zwol)]

Attachment: part 4 as landed

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.

Comment 30

[Zack Weinberg (:zwol)]

Attachment: part 5 as landed

No changes of substance in this piece, just refreshed.

Comment 31

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/2e6221ac8518
http://hg.mozilla.org/mozilla-central/rev/75723e048528

Comment 32

[Zack Weinberg (:zwol)]

Attachment: part 6: collapse gfx/src/thebes into gfx/src

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).

Comment 33

[Zack Weinberg (:zwol)]

Attachment: part 6 v2: gfx/src/thebes into gfx/src

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

Comment 34

[Justin Lebar (not reading bugmail)]

(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.

Comment 35

[(not currently active) Ted Mielczarek]

I guess we should just hold off on this until we kill non-libxul builds. We're not that far off.

Comment 36

[Zack Weinberg (:zwol)]

(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.

Comment 37

[Zack Weinberg (:zwol)]

Attachment: new part 6

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.

Comment 38

[Joe Drew (not getting mail)]

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!

Comment 39

[Zack Weinberg (:zwol)]

(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.

Comment 40

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/ff28c9aab04c