Closed
Bug 398573
Opened 17 years ago
Closed 15 years ago
Flatten dist/include, then get rid of REQUIRES
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
(Whiteboard: [ToDo: comment 11 & 14])
Attachments
(1 file)
(deleted),
patch
|
ted
:
review+
dbaron
:
superreview-
|
Details | Diff | Splinter Review |
We have had a deep dist/include/<module> structure for as long as I can remember, where individual makefiles had to specify REQUIRES for other modules that they pull in. I don't believe that this structure is necessary any longer now that we have well-defined tiers: these tiers prevent "base" code from relying on app-tier highers that it shouldn't.
I want this for mozilla2 so that I can reorder things without worrying about keeping REQUIRES up to date, but I don't see any reason we shouldn't land this now for 1.9.
Attachment #283580 -
Flags: review?(ted.mielczarek)
Comment 1•17 years ago
|
||
Comment on attachment 283580 [details] [diff] [review]
Flatten dist/include, rev. 1
I'm fine with this, but I feel like you should get a second opinion, just to make sure nobody has serious opposition.
Attachment #283580 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #283580 -
Flags: superreview?(dbaron)
Comment 2•17 years ago
|
||
I think we should keep REQUIRES -- it lets us catch when patches are adding dependencies that don't already exist. The bulk of Gecko is in a single tier, and there are a lot of things we'd like not to depend on other things, so tiers don't help us, but REQUIRES does.
Assignee | ||
Comment 3•17 years ago
|
||
Which things in tier-gecko do we not want depending on other things (that we wouldn't catch in review anyway)?
I think we should treat tier-gecko as the interconnected blob that it is and stop the pretenses that:
1) The MODULE lines in our makefiles actually represent defined code modules
2) The REQUIRES lines can somehow represent useful dependendencies between modules
We have all sorts of other dependencies hidden behind nsIObserver topics, contractID, and other abstractions that aren't represented by REQUIRES anyway.
Comment 4•17 years ago
|
||
I'd like intl and gfx and widget and the image code not to depend on content or layout -- something that used to be true (it broke before REQUIRES was added) and still is pretty close to true.
I'd like XPConnect not to depend on content.
I'd like neither XPCOM nor Javascript to depend on each other, despite that one has to be built before the other. (Although I occasionally make Javascript depend on XPCOM for debugging purposes -- which is actually the direction that's currently harder in the build system.)
I think more than just the three of us should discuss this.
Comment 5•17 years ago
|
||
REQUIRES is currently not strictly enforced due to |INCLUDES += -I$(LIBXUL_DIST)/sdk/include| in config.mk. I have a patch in bug 73353 that addresses that (somewhat), but in discussion on irc Benjamin pointed out that he added that on purpose so that anyone may depend on frozen headers without needing a REQUIRES line.
Perhaps the REQUIRES/modules story as it stands is too fine-grained? shaver suggested doing something like that but grouped by tier, which sounds like it may be too coarse, and Benjamin pointed out that in clobber builds lower tiers won't be able to use headers from higher tiers anyway, so we kinda get that for free.
As a middle ground, can we move intl, gfx, widget, the image code and xpconnect into tiers that come before content and layout? Not until we remove the existing dependencies, I guess. Are there any bugs on that?
Assignee | ||
Comment 6•17 years ago
|
||
Let's take this to the newsgroups. XPConnect/CAPS/DOM are tied up with each other, and unless/until we rework CAPS to be a low-level system service I really don't think that's a problem.
In any case, I don't think we should do anything for 1.9, and we should think more broadly for moz2: doing PCH with large meta-headers:
#include "mozilla/core"
#include "mozilla/dom"
#include "mozilla/layout"
Updated•16 years ago
|
Attachment #283580 -
Flags: superreview?(dbaron) → superreview-
Comment 7•16 years ago
|
||
Comment on attachment 283580 [details] [diff] [review]
Flatten dist/include, rev. 1
Did this ever get taken to the newsgroups? In any case, minusing review request pending wider discussion (and I still think REQUIRES is useful, although perhaps we should have slightly fewer modules for it).
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
Comment 9•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a0b2a2ffa124
remove REQUIRES from the tree since it is no longer used...
Comment 10•15 years ago
|
||
(In reply to comment #9)
http://mxr.mozilla.org/mozilla-central/search?string=%5B%5E_%5DREQUIRES®exp=on&case=on&find=%2FMakefile%5C.in%24
Benjamin, your first script left the "REQUIRES +="s lines. And probably some other cases...
Could you do a second script/pass for these? Then we'll see what's left.
Thanks.
Comment 11•15 years ago
|
||
(In reply to comment #9)
http://mxr.mozilla.org/mozilla-central/search?string=ZLIB_REQUIRES&case=on&find=%2Fconfig%2Fautoconf.mk.in
And you missed to manually remove 'ZLIB_REQUIRES' at least...
Flags: in-testsuite-
Version: unspecified → Trunk
Comment 12•15 years ago
|
||
This checkin left a lot of empty if-else-endif clauses behind:
http://hg.mozilla.org/mozilla-central/rev/cc3240bc917a
ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
-REQUIRES += editor
endif
ifdef ACCESSIBILITY
-REQUIRES += accessibility
endif
else
-REQUIRES += expat
endif
ifndef MOZ_XSLT_STANDALONE
-REQUIRES += \
(...)
- $(NULL)
else
-REQUIRES += \
- expat \
- $(NULL)
endif
Comment 13•15 years ago
|
||
...for which there is a patch in bug 513032.
Comment 14•15 years ago
|
||
(In reply to comment #10)
http://hg.mozilla.org/mozilla-central/rev/cc3240bc917a
remove REQUIRES from the tree even when they are in makefile conditional blocks
+
http://hg.mozilla.org/mozilla-central/rev/9faf54d97833
remove more REQUIRES by fixing another bug in the script
Missed 1 related:
http://mxr.mozilla.org/mozilla-central/search?string=STATIC_REQUIRES&case=on&find=%2Fconfig%2Fstatic-config.mk
***
Could you manually check (and remove) the last few different cases?
http://mxr.mozilla.org/mozilla-central/search?string=%5B%5E_%5DREQUIRES®exp=on&case=on
Assignee | ||
Comment 15•15 years ago
|
||
This bug is fixed. The followup/cleanup can happen in other bugs if it's that important to you (it's not that important to me, just dead-code elimination).
Status: REOPENED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
Comment 16•7 years ago
|
||
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/1ae48b2a2992
Followup to bug 398573 - remove REQUIRES from the tree since it is no longer used... automatically generated patch, rs=ted
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•