Flatten dist/include, then get rid of REQUIRES

RESOLVED FIXED

Status

()

Core
Build Config
--
trivial
RESOLVED FIXED
11 years ago
3 months ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
Created attachment 283580 [details] [diff] [review]
Flatten dist/include, rev. 1

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+
(Assignee)

Updated

11 years ago
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.
(Assignee)

Comment 3

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

10 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

10 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"
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).
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 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&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.
(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

9 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
...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
(Assignee)

Comment 15

9 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
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 521673
Blocks: 234737

Comment 16

3 months 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
You need to log in before you can comment on or make changes to this bug.