Closed Bug 143399 Opened 22 years ago Closed 20 years ago

Further-specify module directories to avoid export dependency problems

Categories

(SeaMonkey :: Build Config, defect)

Product:
SeaMonkey
Component:
Build Config
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: mcafee, Assigned: mcafee)

References

Details

Attachments

(1 file, 3 obsolete files)

Patch to add optional export-idl rule
750 bytes, patch
Details | Diff | Splinter Review 
I'm running into some idl errors like this:

nsIMIMEService.idl:52: can't open included file nsIURI.idl for reading

during the export phase of the build, to get around this
problem I've created an optional rule to export all the idl
files first, patch coming up.
This rule needs to work in directories w/o idl files, e.g.
the top-level directory; also needs a rule to create dist/idl
if needed.
Ugh. I really don't like duplicating the ruleset like that.  The circular idl
dependencies aren't surprising but how are you triggering that condition?  That
looks like a simple build ordering problem (ie, build netwerk/base before
netwerk/mime as we've always done) but I'm probably missing something here.

Current idl rules are scoped to only directories with idl stuff,
this doesn't work for top-level, etc. dirs.  I decided to leave current
idl rules alone and copied/modified only what I needed outside of
this scope.
 
I'm getting this condition from the requires build attempt for gtkembedmoz
(mozilla/tools/module-deps/bootstrap --module=gtkembedmoz)
We could move this part of the IDL rule outside
of the (XPIDLSRCS || SDK_XPIDLSRCS) scope so that
it would work everywhere, but I wasn't sure this
was worth the trouble and might be confusing.

> Current idl rules are scoped to only directories with idl stuff,
> this doesn't work for top-level, etc. dirs.  

Huh?  The current idl rules, as part of the export pass, are only invoked if
there are idl files listed; that is correct.  However, every dir is hit since
the export pass will $(LOOP_OVER_DIRS) due to an earlier export:: ruleset.  I'm
still confused by the "work everywhere" comment as the ruleset is only useful if
you have idls in the current dir.

Adding an export-idl pass is unnecessary if you've fixed the build ordering
problems.  I don't like the current export/libs split for the main build so I'm
not going to advocate adding another layer just to avoid getting our ordering
dependencies correct.

My bootstraping of gtkembedmoz seems to be taking its sweet old time.  I'm still
waiting for the new modules.mk to be generated so that I can see what DIRS looks
like.

The export-idl rule would be used before export:
  gmake export-idl
  gmake export
This is just exporting idl files, nothing else.
It's an optional rule, and would not be part of the main build.

> every dir is hit since the export pass will $(LOOP_OVER_DIRS)
> due to an earlier export:: ruleset.

I'm not doing export here since I get circular dependencies, so
I don't get the LOOP_OVER_DIRS you are describing.

I haven't figured out the build-order problem in
the export phase yet, this is a workaround.  If there's
a better solution I am all for it.  This is a 12-line
rule to get things rolling.  I think the build-order
in the libs phase is working better.
I've said this before and I'll say it again: we need some sort of partial
ordering which will give us a correct build order. We will probably have to
maintain this partial ordering

I thought about this a bit more, and I came up with this:

we keep a file which would have entries like
intl,netwerk/base
xpfe/components,mailnews

these indicate that, for example, "all intl should be built before anything in
netwerk/base" and "all xpfe should be built before all mailnews" - these would
essentially just be regular expressions which would match the paths.

then, assuming the current sorting is being done with perl, we use the perl
sort's comparison callback mechanism, to write our own comparison thing,
something like:

build_order_compare() {
  if ($foo = partial_order_compare($a, $b))
    return $foo;
  return leaf_order_compare($a, $b);
}

where partial_order_compare will look up comparisons in the table - if the given
$a and $b don't match any rules int he partial-order table, then it returns 0.
This allows us to drop back to leaf_order_compare, which is the mechanism that
you're using right now...
In the generated modules.mk, BUILD_MODULE_DIRS starts off with: config build
include js embedding/components/windowwatcher modules/libreg string xpcom gfx2
...  The fact that we're building a xpcom component before xpcom should have
been the first clue that we have a serious ordering issue here. I don't see how
that would work even with the export-idl patch.  

Alec is correct; we do need to make sure that our build order is correct for
certain modules.  My only current concern about the partial order file is that
it seems as though it could get unwieldy very quickly.  I was pondering a weight
system similar to what is being done in the toplevel Makefile but I haven't
fleshed out any details.

Regardless of which implementation we choose, we should be building in an order
similar to the normal build:

config/build/include
3rd party libs (including nspr & ldap)
moz application libs (ie, js & xpcom)
moz components (with their own TBD ordering issues)
Blocks: 143524
export-idl rule is a workaround for the module-inside-module dependency problem.
Resummarizing.
Summary: Need optional export-idl rule for modular embed build → Further-specify module directories to avoid export dependency problems
nsIURI.idl is a circular dependency problem:

REQUIRES tree looks like this:
  gtkembedmoz
    dom
      find
        ...
        layout
          htmlparser
            necko
              uconv
                locale
                  gfx
                    view
                      pref
                        xpconnect
                          js
                          caps
                            docshell
                              commandhandler
                                xuldoc
                                  chrome
                                    content
                                      imglib2
                                        mimetype
                                          [necko]

necko shows up again for mimetype.  I'll add this to bug 145333.
Note this is an export-only dependency, I don't seem to have
any problems in the libs phase.
there are MANY circular dependencies like this, we're not going to fix them
all..(in fact, its likely we wont fix any)

to reiterate, the fix is to:
1) specify each individual directory in each module (this bug)
2) for any stragglers that aren't fixed by 1), we need to forward-declare
classes and rearrange the #includes

I wouldn't even bother tackling the necko problem until 1) is complete.

also, that 2nd patch isn't what is advertised, did you attach the wrong file?
Attachment #84182 - Attachment is obsolete: true
forward declaring classes (part 2 of comment #13) is now bug 145777.
Depends on: 145777
No longer depends on: 145777
Blocks: 145777
No longer blocks: 143524
Comment on attachment 84261 [details] [diff] [review]
module2dir.pl: better necko dir specification. Added test_necko and TestStreamConv modules that were missing.

r=blythe

manual explicit directory listing is fine.  we can do a fancier dynamic
directory listing step at some future point (perhaps another bug that is not so
high in priority?).
Attachment #84261 - Flags: review+
Comment on attachment 84261 [details] [diff] [review]
module2dir.pl: better necko dir specification. Added test_necko and TestStreamConv modules that were missing.

sr=alecf - yeah, lets keep this bug open for the "real" fix where we
dynamically determine the module name of each directory
Attachment #84261 - Flags: superreview+
Comment on attachment 84261 [details] [diff] [review]
module2dir.pl: better necko dir specification. Added test_necko and TestStreamConv modules that were missing.

checked in necko patch
Attachment #84261 - Attachment is obsolete: true
Comment on attachment 86568 [details] [diff] [review]
Adding new modules, modules that live in mozilla/extensions, and modules buried a few levels deep that I missed before.  Cleaned up js module.

r=blythe
Attachment #86568 - Flags: review+
Attachment #86568 - Flags: superreview+
Comment on attachment 86568 [details] [diff] [review]
Adding new modules, modules that live in mozilla/extensions, and modules buried a few levels deep that I missed before.  Cleaned up js module.

sr=alecf
Comment on attachment 86568 [details] [diff] [review]
Adding new modules, modules that live in mozilla/extensions, and modules buried a few levels deep that I missed before.  Cleaned up js module.

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.
Attachment #86568 - Flags: approval+
Comment on attachment 86568 [details] [diff] [review]
Adding new modules, modules that live in mozilla/extensions, and modules buried a few levels deep that I missed before.  Cleaned up js module.

checked patch 86568 in, crossing it off in the bug.  Leaving this bug open for
other changes for now.
Attachment #86568 - Attachment is obsolete: true
Not sure if any work is going on here right now, I am suggesting we mark this as
done for now (wontfix) and file another bug if we need to.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → WONTFIX
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: