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)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

(Whiteboard: [ToDo: comment 11 & 14])

Attachments

(1 file)

Attached patch Flatten dist/include, rev. 1 (deleted) — 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 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+
Attachment #283580 - Flags: superreview?(dbaron)
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.
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.
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.
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?
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"
Attachment #283580 - Flags: superreview?(dbaron) → superreview-
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).
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
http://hg.mozilla.org/mozilla-central/rev/a0b2a2ffa124 remove REQUIRES from the tree since it is no longer used...
(In reply to comment #9) http://mxr.mozilla.org/mozilla-central/search?string=%5B%5E_%5DREQUIRES&regexp=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.
Flags: in-testsuite-
Version: unspecified → Trunk
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
...for which there is a patch in bug 513032.
(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&regexp=on&case=on
Severity: normal → trivial
Status: RESOLVED → REOPENED
Depends on: 512497, 488175
OS: Mac OS X → All
Hardware: x86 → All
Resolution: DUPLICATE → ---
Summary: Flatten dist/include → Flatten dist/include, then get rid of REQUIRES
Whiteboard: [ToDo: comment 11 & 14]
Depends on: 513398
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 ago15 years ago
Resolution: --- → FIXED
Depends on: 521673
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: