Closed Bug 1459721 Opened 6 years ago Closed 6 years ago

remove the dist_idl install manifest

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 1 obsolete file)

We should just stop copying all the IDL files to the objdir everytime we build.  It makes generating IDL files at build time difficult, and there's no reason to do it anytime, since we e.g. no longer care about the SDK.
This method is only called in one place, and it doesn't pass
allow_existing.  Whatever ugly thing this keyword was working around
doesn't exist anymore, so let's get rid of it.

This is just a cleanup.
Attachment #8973810 - Flags: review?(core-build-config-reviews)
This member is unused, so we might as well dispense with it.

Another cleanup before we get to the intersting stuff.
Attachment #8973811 - Flags: review?(core-build-config-reviews)
The current IDL build setup assumes that all IDL files can be found in a
single directory.  This setup requires that all IDL files be copied to a
single directory, which is suboptimal in terms of disk I/O and also
complicates things like generating IDL files at build time.

As a first step in moving away from this state of affairs,
xpidl-process.py needs to be taught that the input IDL files could
potentially be found in multiple directories.  The current setup can
just specify $(DIST)/idl as the lone directory to examine.  Future
patches will change this to examine multiple directories.
Attachment #8973812 - Flags: review?(core-build-config-reviews)
Building on the last patch, we can change the build process to pass in
the directories where the input IDL files can be found.  It is
convenient to pass in just the relative source directory paths, to
encourage people to not look in the object directory and to make the
command lines slightly shorter.

xpidl-process.py still assumes that included IDL files can be found by
looking in a single directory.  We add a single -I argument to the
invocation of xpidl-process.py to accommodate this short-sightedness.
Attachment #8973813 - Flags: review?(core-build-config-reviews)
The previous patch required us to pass a single -I argument pointing at
$(DIST)/idl so IDL include statements would work correctly.  This patch
lifts that limitation and explicitly points xpidl-process.py at the
locations of all the IDL source directories to search for included IDL
files.  Invocations of xpidl-process.py no longer depend on IDL files
being copied to the objdir.
Attachment #8973814 - Flags: review?(core-build-config-reviews)
The tail end of the xpidl Makefile.in contains a line, generated for
every xpt file:

$(1): $(addsuffix .idl,$(addprefix $(dist_idl_dir)/,$($(basename $(notdir $(1)))_deps)))

This line, in context, is saying that the xpt file depends on all of its
input IDL files.  But xpidl-process.py already generates this
information when we pass it --depsdir, which we do.  So this code is
redundant with what we already generate, and it can be removed.
Attachment #8973815 - Flags: review?(core-build-config-reviews)
We no longer need to install the IDL files to the objdir for
processing. \o/
Attachment #8973816 - Flags: review?(core-build-config-reviews)
Attachment #8973810 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 8973811 [details] [diff] [review]
part 2 - remove install_target member from XPIDLManager.modules

Review of attachment 8973811 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/common.py
@@ +79,5 @@
>              raise Exception('IDL already registered: %s' % entry['basename'])
>  
>          self.idls[entry['basename']] = entry
> +        # First element is a set of interface file basenames (no extension).
> +        t = self.modules.setdefault(entry['module'], (set(),))

This one-element tuple doesn't seem to be buying us anything, can't you just use the set as the value?
Attachment #8973811 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 8973812 [details] [diff] [review]
part 3 - enable multiple input paths for xpidl-process.py

Review of attachment 8973812 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/action/xpidl-process.py
@@ +44,5 @@
>      rule.add_dependencies(iter_modules_in_path(topsrcdir))
>  
> +    def read_stem(stem):
> +        idl = '%s.idl' % stem
> +        for p in input_dirs:

So we search every directory for each file in case it has the one we care about? This seems clumsy and potentially slow, shouldn't we just pass the full paths this script and let it derive the set of directories from that if necessary? Does this get better in a patch later in the series?
Attachment #8973812 - Flags: review?(core-build-config-reviews)
Comment on attachment 8973813 [details] [diff] [review]
part 4 - explicitly specify input directories for xpidl modules

Review of attachment 8973813 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1066,5 @@
>  
>          mk = Makefile()
>  
>          for module in xpt_modules:
> +            (sources, directories) = modules[module]

Parens not necessary.

@@ +1082,5 @@
>              # reference to the new .idl. Since the new .idl presumably has
>              # an mtime newer than the .xpt, it will trigger xpt generation.
>              mk.add_statement('%s_deps = %s' % (module, ' '.join(deps)))
>  
> +            mk.add_statement('%s_dirs = %s' % (module, ' '.join(directories)))

A separate variable here doesn't really seem necessary, but ok.
Attachment #8973813 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 8973814 [details] [diff] [review]
part 5 - explicitly specify include directories for xpidl files

Review of attachment 8973814 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/tup.py
@@ +680,5 @@
>              ]
>  
>              for d in directories:
>                  cmd.extend(['--input-dir', d])
> +            for d in all_idl_directories:

This kinda looks like it's going to iterate of the files instead of the directories, does this pass the tup build on try?
Attachment #8973814 - Flags: review?(core-build-config-reviews) → review+
(In reply to Chris Manchester (:chmanchester) from comment #8)
> ::: python/mozbuild/mozbuild/backend/common.py
> @@ +79,5 @@
> >              raise Exception('IDL already registered: %s' % entry['basename'])
> >  
> >          self.idls[entry['basename']] = entry
> > +        # First element is a set of interface file basenames (no extension).
> > +        t = self.modules.setdefault(entry['module'], (set(),))
> 
> This one-element tuple doesn't seem to be buying us anything, can't you just
> use the set as the value?

We will be adding more elements later in the series, seemed a bit silly to switch it back and forth.

(In reply to Chris Manchester (:chmanchester) from comment #9)
> ::: python/mozbuild/mozbuild/action/xpidl-process.py
> @@ +44,5 @@
> >      rule.add_dependencies(iter_modules_in_path(topsrcdir))
> >  
> > +    def read_stem(stem):
> > +        idl = '%s.idl' % stem
> > +        for p in input_dirs:
> 
> So we search every directory for each file in case it has the one we care
> about? This seems clumsy and potentially slow, shouldn't we just pass the
> full paths this script and let it derive the set of directories from that if
> necessary? Does this get better in a patch later in the series?

It doesn't get better later in the series, but I am happy to add an additional patch to clean that up.  xpidl-process.py expects the IDL files to be passed in as stems, and it seemed easier to let that be.  I can clean it up in a part 8 or so, would that be OK?
Flags: needinfo?(cmanchester)
Attachment #8973815 - Flags: review?(core-build-config-reviews) → review+
Attachment #8973816 - Flags: review?(core-build-config-reviews) → review+
(In reply to Nathan Froyd [:froydnj] from comment #12)
> (In reply to Chris Manchester (:chmanchester) from comment #8)
> > ::: python/mozbuild/mozbuild/backend/common.py
> > @@ +79,5 @@
> > >              raise Exception('IDL already registered: %s' % entry['basename'])
> > >  
> > >          self.idls[entry['basename']] = entry
> > > +        # First element is a set of interface file basenames (no extension).
> > > +        t = self.modules.setdefault(entry['module'], (set(),))
> > 
> > This one-element tuple doesn't seem to be buying us anything, can't you just
> > use the set as the value?
> 
> We will be adding more elements later in the series, seemed a bit silly to
> switch it back and forth.
> 
> (In reply to Chris Manchester (:chmanchester) from comment #9)
> > ::: python/mozbuild/mozbuild/action/xpidl-process.py
> > @@ +44,5 @@
> > >      rule.add_dependencies(iter_modules_in_path(topsrcdir))
> > >  
> > > +    def read_stem(stem):
> > > +        idl = '%s.idl' % stem
> > > +        for p in input_dirs:
> > 
> > So we search every directory for each file in case it has the one we care
> > about? This seems clumsy and potentially slow, shouldn't we just pass the
> > full paths this script and let it derive the set of directories from that if
> > necessary? Does this get better in a patch later in the series?
> 
> It doesn't get better later in the series, but I am happy to add an
> additional patch to clean that up.  xpidl-process.py expects the IDL files
> to be passed in as stems, and it seemed easier to let that be.  I can clean
> it up in a part 8 or so, would that be OK?

Sure, although I think simplifying here would mean we wouldn't need the extra variable I mentioned in part 4 either.
Flags: needinfo?(cmanchester)
(In reply to Chris Manchester (:chmanchester) from comment #13)
> Sure, although I think simplifying here would mean we wouldn't need the
> extra variable I mentioned in part 4 either.

Oh!  I think we can remove that makefile variable regardless; I think I had it in there before I realized I could just consult the individual ${module}_dirs values with a `foreach`.
The build system knows at build-backend time where to find each IDL
file; making xpidl-process.py rediscover this by requiring
xpidl-process.py to search through directories to find input IDL files
is silly.  To rememdy this, we're going to modify things so full paths
are passed into the script.  Those paths can then be used directly, with
no searching.
Attachment #8975072 - Flags: review?(cmanchester)
Here, let's provide a patch that doesn't break the tup backend.
Attachment #8975106 - Flags: review?(cmanchester)
Attachment #8975072 - Attachment is obsolete: true
Attachment #8975072 - Flags: review?(cmanchester)
Comment on attachment 8975106 [details] [diff] [review]
part 8 - pass full paths for IDL files to xpidl-process.py

Review of attachment 8975106 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating this.

::: python/mozbuild/mozbuild/backend/tup.py
@@ +813,5 @@
>  
>              all_xpts.append('$(MOZ_OBJ_ROOT)/%s/%s.xpt' % (backend_file.relobjdir, module))
>              outputs = ['%s.xpt' % module]
> +            stems = list(sorted(map(lambda idl: mozpath.splitext(idl)[0],
> +                                    map(mozpath.basename, idls))))

I'm told that list comprehensions or generator expressions are more idiomatic in python than `map`... the nested `map` doesn't really seem necessary. The cast to a list doesn't, either. How about: `sorted(mozpath.splitext(mozpath.basename(f))[0] for f in idls)`?
Attachment #8975106 - Flags: review?(cmanchester) → review+
Thanks for the review.

(In reply to Chris Manchester (:chmanchester) from comment #17)
> ::: python/mozbuild/mozbuild/backend/tup.py
> @@ +813,5 @@
> >  
> >              all_xpts.append('$(MOZ_OBJ_ROOT)/%s/%s.xpt' % (backend_file.relobjdir, module))
> >              outputs = ['%s.xpt' % module]
> > +            stems = list(sorted(map(lambda idl: mozpath.splitext(idl)[0],
> > +                                    map(mozpath.basename, idls))))
> 
> I'm told that list comprehensions or generator expressions are more
> idiomatic in python than `map`... the nested `map` doesn't really seem
> necessary. The cast to a list doesn't, either. How about:
> `sorted(mozpath.splitext(mozpath.basename(f))[0] for f in idls)`?

That sounds great; for some reason I thought `sorted` returned an iterator, rather than a list.

Just to be clear, with this bit, part 3 has your r+ (cf comment 13), or would you prefer that I fold this part into part 3?
Flags: needinfo?(cmanchester)
(In reply to Nathan Froyd [:froydnj] from comment #18)
> Thanks for the review.
> 
> (In reply to Chris Manchester (:chmanchester) from comment #17)
> > ::: python/mozbuild/mozbuild/backend/tup.py
> > @@ +813,5 @@
> > >  
> > >              all_xpts.append('$(MOZ_OBJ_ROOT)/%s/%s.xpt' % (backend_file.relobjdir, module))
> > >              outputs = ['%s.xpt' % module]
> > > +            stems = list(sorted(map(lambda idl: mozpath.splitext(idl)[0],
> > > +                                    map(mozpath.basename, idls))))
> > 
> > I'm told that list comprehensions or generator expressions are more
> > idiomatic in python than `map`... the nested `map` doesn't really seem
> > necessary. The cast to a list doesn't, either. How about:
> > `sorted(mozpath.splitext(mozpath.basename(f))[0] for f in idls)`?
> 
> That sounds great; for some reason I thought `sorted` returned an iterator,
> rather than a list.
> 
> Just to be clear, with this bit, part 3 has your r+ (cf comment 13), or
> would you prefer that I fold this part into part 3?

Yes, this has r+, it's fine as a separate patch if that simplifies anything.
Flags: needinfo?(cmanchester)
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0bd15e1d5d
part 1 - remove allow_existing keyword arg from register_idl; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/94988ff9a3f3
part 2 - remove install_target member from XPIDLManager.modules; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/55957988163d
part 3 - enable multiple input paths for xpidl-process.py; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/356ba3cc76e1
part 4 - explicitly specify input directories for xpidl modules; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/545f0976650c
part 5 - explicitly specify include directories for xpidl files; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/05168b1b6cf7
part 6 - remove redundant dependency code from xpidl Makefile.in; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f618d13bd1c
part 7 - remove dist_idl install manifest; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/d52f0d61dd57
part 8 - pass full paths for IDL files to xpidl-process.py; r=chmanchester
Depends on: 1461881
Depends on: 1461880
No longer depends on: 1461881
No longer depends on: 1461880
You need to log in before you can comment on or make changes to this bug.