Closed Bug 398573 Opened 12 years ago Closed 10 years ago
Flatten dist/include, then get rid of REQUIRES
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+
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.
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: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 488175
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®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.
(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...
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®exp=on&case=on
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: 11 years ago → 10 years ago
Resolution: --- → FIXED
Pushed by email@example.com: 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
You need to log in before you can comment on or make changes to this bug.