Closed Bug 73353 (requires) Opened 19 years ago Closed 12 years ago

Clean up our MODULE/REQUIRES story

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jag+mozbugs, Assigned: jag+mozilla)

Details

(Whiteboard: [not needed for 1.9] leaving untargetted to avoid perpetual spamming.)

Attachments

(5 files, 17 obsolete files)

22.69 KB, patch
bryner
: review+
Details | Diff | Splinter Review
35.89 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
16.53 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
89.29 KB, patch
dbaron
: review+
dbaron
: superreview+
brendan
: approval1.9+
Details | Diff | Splinter Review
52.30 KB, patch
Details | Diff | Splinter Review
Since these lines are (supposed to be) used to determine inter-module
relationships, it's best to keep the list minimal. It's quite a bit of work
though to keep track of modules no longer needed (where, if you need a new one,
the compiler on a MOZ_TRACK_MODULE_DEPS build will let you know).

A friend of mine wrote a little script which looks at the .deps/*.pp files to
determine which modules were needed, compares that against what's currently
listed on the REQUIRES line, and gives output in the form of a cvs diff -u.

I've applied this patch to a fresh tree, and built with it successfully.

I'll attach the patch for review.
Attached patch [patch] clean up REQUIRES lines (obsolete) — Splinter Review
Making dbaron the QA contact on this. doron, hope you don't mind :-)
Status: NEW → ASSIGNED
QA Contact: doronr → dbaron
r=cls
Attached patch [patch] More cleaning up (obsolete) — Splinter Review
r=dbaron on that, assuming it builds :-)

I guess anything it breaks should be fixed by a conditional (or even just
commented) |REQUIRES += ...|.
Changing the summary, I'll just use this as a tracker.
Summary: Remove cruft from REQUIRES lines → Clean up our MODULE/REQUIRES story
r=cls on attach 30060
After doing a clobber build it turned out I needed to fix these two too:
embedding/base/Makefile.in
rdf/tests/rdfcat/Makefile.in

Doing a "find -name 'Makefile.in' | xargs grep MODULE" from mozilla/gfx:

./idl/Makefile.in:MODULE                = gfx
./idl/Makefile.in:XPIDL_MODULE  = gfx
./src/ps/Makefile.in:MODULE             = gfx
./src/qt/Makefile.in:MODULE             = layout
./src/gtk/Makefile.in:MODULE            = gfx
./src/mac/Makefile.in:MODULE            = layout
./src/os2/Makefile.in:MODULE            = layout
./src/beos/Makefile.in:MODULE           = layout
./src/xlib/Makefile.in:MODULE           = layout
./src/xlibrgb/Makefile.in:MODULE                = xlibrgb
./src/xlibrgb/Makefile.in:XPIDL_MODULE  = layout
./src/motif/Makefile.in:MODULE          = layout
./src/rhapsody/Makefile.in:MODULE               = layout
./src/Makefile.in:MODULE                = gfx
./src/photon/Makefile.in:MODULE         = layout
./src/xprint/Makefile.in:MODULE         = gfx
./tests/Makefile.in:MODULE              = gfx
./public/Makefile.in:MODULE             = gfx

What should I do with all those other MODULE lines?
This might be a question for drivers.  Can gfx really be separated from layout? 
I think everything under gfx should be set to MODULE = gfx.

gfx doesn't (shouldn't) have any direct dependancies on layout.  It should be 
able to build without it.
Assignee: jag → jaggernaut
Status: ASSIGNED → NEW
Mass move to jaggernaut@netscape.com
Whiteboard: leaving untargetted to avoid perpetual spamming.
Nice.  r=dbaron.
Don't you have to add XPIDL_MODULE in some places (or change packaging files, 
but I'd prefer not)?  Are there any static build issues?
I checked for idl files but there are none where I change the MODULE name.

Btw, I talked to Dauphin and we agreed on putting everything in the "content"
MODULE, instead of all the seperate ones. I'm also tempted to change the
existing "xul", "xuldoc" and "xultmpl" over to "content" and add XPIDL_MODULE
there to keep the right .xpts.

I don't know what the impact of this is on the static build though. Waterson?
There should be no impact.
Attached patch [patch] intermediate clean-up. (obsolete) — Splinter Review
Comment on attachment 46749 [details] [diff] [review]
[patch] intermediate clean-up.

r=cls
Attachment #46749 - Flags: review+
Comment on attachment 49309 [details] [diff] [review]
Instead of adding "gfx" to manager/ssl/src/Makefile.in, I'll just remove a #include "nsIWidget.h" ;-)

r=bryner
Attachment #49309 - Flags: review+
Attachment #49309 - Flags: superreview+
Comment on attachment 49311 [details] [diff] [review]
Move gfx from module layout to module gfx and fix REQUIRES lines.

r=cls in e-mail
Attachment #49311 - Flags: review+
Attached patch Y'all are gonna love this! (obsolete) — Splinter Review
Attached patch Some nice clean-up. (obsolete) — Splinter Review
Comment on attachment 49667 [details] [diff] [review]
Y'all are gonna love this!

this patch was checked in with sr=alecf on Sep 18.
Stop confusing my module owner.
And don't touch extensions/transformiix/build/Makefile.in
until we moved to static libs for building.
> And don't touch extensions/transformiix/build/Makefile.in
> until we moved to static libs for building.

So should we remove it from the build until then?  Tree-wide changes often need
to be made everywhere at once.

Pike: I haven't touched anything transformiix after you mentioned it to me, but
thanks for reminding me and making it public so others know too :-)
Accepting.
Status: NEW → ASSIGNED
Attached patch Clean-up 2003-06-27 (obsolete) — Splinter Review
Attachment #126627 - Flags: superreview?(alecf)
Comment on attachment 126627 [details] [diff] [review]
Clean-up 2003-06-27

wow, so many great cleanups :)
I'll apply this on linux, see what happens.
Attachment #126627 - Flags: superreview?(alecf) → superreview+
Attachment #28725 - Attachment is obsolete: true
Attachment #30047 - Attachment is obsolete: true
Attachment #30060 - Attachment is obsolete: true
Attachment #30548 - Attachment is obsolete: true
Attachment #43229 - Attachment is obsolete: true
Attachment #45485 - Attachment is obsolete: true
Attachment #46749 - Attachment is obsolete: true
Attachment #49309 - Attachment is obsolete: true
Attachment #49311 - Attachment is obsolete: true
Attachment #49667 - Attachment is obsolete: true
Attachment #53731 - Attachment is obsolete: true
Attachment #126627 - Attachment is obsolete: true
Attached patch Diff -uw for review (obsolete) — Splinter Review
Attachment #127374 - Attachment is obsolete: true
Comment on attachment 127473 [details] [diff] [review]
Same as above, but diff -u for testing

r=cls
Attachment #127473 - Flags: review+
Attachment #127472 - Flags: superreview?(bryner)
Attachment #127472 - Flags: superreview?(bryner) → superreview+
Comment on attachment 127473 [details] [diff] [review]
Same as above, but diff -u for testing

Noting sr=bryner, requesting approval. Risk is low, if anything breaks it'll
show up as build bustage on tinderboxen, easy enough to fix.
Attachment #127473 - Flags: superreview+
Attachment #127473 - Flags: approval1.5a?
Comment on attachment 127473 [details] [diff] [review]
Same as above, but diff -u for testing

Minusing for 1.5a approval, since (a) we're probably going to cut a branch
tomorrow (b) I'd rather not risk having the source tarball not be buildable in
unusual configurations that, which is a risk here.  Sometimes bustage from this
type of thing for more unusual things doesn't show up for a few days.
Attachment #127473 - Flags: approval1.5a? → approval1.5a-
Let's get this into 1.5b as soon as the trunk opens.

/be
Attachment #127472 - Attachment is obsolete: true
Attachment #127473 - Attachment is obsolete: true
Neil said this patch built for him on Linux.
Attachment #153454 - Attachment is obsolete: true
Attachment #159146 - Flags: review+
Product: Browser → Seamonkey
Attached patch patchSplinter Review
Some additional cleanup, particularly for firefox/toolkit.  Tested the
following build configurations:

linux firefox opt
windows firefox opt
mac firefopx opt
mac seamonkey debug + tests
Attachment #173314 - Flags: superreview?(dbaron)
Attachment #173314 - Flags: review?(dbaron)
Comment on attachment 173314 [details] [diff] [review]
patch

You probably don't want the intl/uconv/src/Makefile.in change (although you may
want to move the ucnative into the ifdef, so it's a REQUIRES += ucnative
line that doesn't get caught by this script)
Attachment #173314 - Flags: superreview?(dbaron)
Attachment #173314 - Flags: superreview+
Attachment #173314 - Flags: review?(dbaron)
Attachment #173314 - Flags: review+
Tested mac seamonkey, windows seamonkey, and windows thunderbird.
Attachment #173348 - Flags: superreview?(dbaron)
Attachment #173348 - Flags: review?(dbaron)
Attachment #173348 - Flags: superreview?(dbaron)
Attachment #173348 - Flags: superreview+
Attachment #173348 - Flags: review?(dbaron)
Attachment #173348 - Flags: review+
Alias: requires
Assignee: jag → nobody
Status: ASSIGNED → NEW
OS: Linux → All
Product: Mozilla Application Suite → Core
QA Contact: dbaron → general
Assignee: nobody → jag
Status: NEW → ASSIGNED
Attachment #299946 - Flags: superreview?(dbaron)
Attachment #299946 - Flags: review?(dbaron)
Comment on attachment 299946 [details] [diff] [review]
Passes TryServer (Firefox), and SeaMonkey and Thunderbird on Mac and Linux.

Ignore the non-Makefile.in changes, they're from peterv's patch on bug 407034 which make using trunk bearable. I'll attach one without them in a bit.
Attachment #299946 - Attachment is obsolete: true
Attachment #299946 - Flags: superreview?(dbaron)
Attachment #299946 - Flags: review?(dbaron)
Attachment #300043 - Flags: superreview?(dbaron)
Attachment #300043 - Flags: review?(dbaron)
Comment on attachment 300043 [details] [diff] [review]
Passes TryServer (Firefox), and SeaMonkey and Thunderbird on Mac and Linux. [checked in]

So I'm not so happy about ifdef'd REQUIRES changes -- they make it easy for somebody in one configuration to make a change that compiles fine (since they have that thing turned on) but that breaks for somebody with the feature off.

However, they are helpful for automatic testing of whether requires lines are still needed.

Maybe the best thing to do would be a compromise:  comment rather than ifdef?  Does that seem reasonable?

rubber-stamp r+sr=dbaron with that (or without it if you can convince me otherwise)
Attachment #300043 - Flags: superreview?(dbaron)
Attachment #300043 - Flags: superreview+
Attachment #300043 - Flags: review?(dbaron)
Attachment #300043 - Flags: review+
<dbaron> the places where you have ifdef ACCESSIBILITY
<dbaron> just say
<dbaron> # for ACCESSIBILITY
<dbaron> or something like that
<jag>    If you're ok with that
<jag>    We've always done the ifdef
<jag>    I can change all locations to use the comment style.
<dbaron> I dunno.
<jag>    Would you be ok with me doing that in a separate bug?
<dbaron> I guess it's ok the way you have it.
<dbaron> separate bug is ok, yeah
<dbaron> If it's not biting people maybe it's not even worth the bother.

Filed bug 418037 for further discussion.
Attachment #300043 - Flags: approval1.9?
Comment on attachment 300043 [details] [diff] [review]
Passes TryServer (Firefox), and SeaMonkey and Thunderbird on Mac and Linux. [checked in]

jag is on the case on irc.

/be
Attachment #300043 - Flags: approval1.9? → approval1.9+
So in certain cases people have been picking up header files from dist/sdk/include instead of from dist/include/$MODULE. This changes config.mk to only add dist/sdk/include when MOZ_ENABLE_LIBXUL is defined, and fixes up the various REQUIRES lines that were missing modules due to this. This builds on my Intel Mac Mini under SeaMonkey, Firefox, Thunderbird, Camino and XulRunner.
Comment on attachment 300043 [details] [diff] [review]
Passes TryServer (Firefox), and SeaMonkey and Thunderbird on Mac and Linux. [checked in]

1.148 jag%tty.nl 2008-02-18 00:50
Attachment #300043 - Attachment description: Passes TryServer (Firefox), and SeaMonkey and Thunderbird on Mac and Linux. → Passes TryServer (Firefox), and SeaMonkey and Thunderbird on Mac and Linux. [checked in]
Comment on attachment 310745 [details] [diff] [review]
Another pass, this time also putting in missing modules.

This patch hasn't landed and is presumably holding this bug open. Can we spin this off to another bug? Drivers want all bugs with approval1.9+ closed ASAP.
Whiteboard: leaving untargetted to avoid perpetual spamming. → [patch with a+ landed, another without a unlanded] leaving untargetted to avoid perpetual spamming.
Whiteboard: [patch with a+ landed, another without a unlanded] leaving untargetted to avoid perpetual spamming. → [not needed for 1.9] leaving untargetted to avoid perpetual spamming.
Since bsmedberg wants to change the way this all works I've held off on getting that patch reviewed and checked in. I'm just gonna close this bug. I'll re-open it, or open a new one, if needed.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/32e089cc6736
Clean up our MODULE/REQUIRES story. rs=dbaron, a=brendan
You need to log in before you can comment on or make changes to this bug.