1114669 - Removing an IDL file requires a CLOBBER touch

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Neil Deakin]

An idl file was removed.

Comment 1

[Takanori MATSUURA]

Bug 1066383 is fixed.

Comment 2

[Mike Hommey [:glandium]]

Please reopen all those bugs you just closed. They are about those fixes triggering bad behavior in the build system. So, while those bugs you mention _are_ fixed, the bugs about them breaking the build without a clobber _aren't_.

Comment 3

[Mike Hommey [:glandium]]

松浦さん, please see comment 2.

Comment 4

[Takanori MATSUURA]

(In reply to Mike Hommey [:glandium] from comment #2)
> Please reopen all those bugs you just closed. They are about those fixes
> triggering bad behavior in the build system. So, while those bugs you
> mention _are_ fixed, the bugs about them breaking the build without a
> clobber _aren't_.

If so, bugs which have a title of "Bug xxxxxxx requires clobber" is not good. They should have file or directory based title such as "Modification of browser/xxx/yyy files requires clobber".

Anyway I'll reopen bugs.

Comment 5

[Takanori MATSUURA]

(In reply to Takanori MATSUURA from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #2)
> They should have file or directory 
or function
> based title such as "Modification
> of browser/xxx/yyy files requires clobber".
> 
> Anyway I'll reopen bugs.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is pretty annoying. I have a patch that adds an IDL file and whenever I un-apply it (which deletes the idl file), the build breaks.

Comment 7

[Nathan Froyd [:froydnj]]

I looked at this, and I think I see what's going wrong.  We have this bit of code to generate config/makefiles/xpidl/Makefile:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#966

and it's generating rules for the .xpt files that include the IDL files as dependencies:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#947

Those dependencies *do* get updated when an IDL file gets removed from XPIDL_SOURCES in a moz.build file.

But, xpidl-process.py *also* figures out what IDLs we're processing for a given .xpt file:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/xpidl-process.py#53

and declares them as dependencies:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/xpidl-process.py#62

and my suspicion is that the dependency information from the .deps/*.pp files overwrites the dependency information generated from moz.build...but the actions generated in the rule from moz.build use the dependency information from the .pp file, not the dependencies generated from moz.build.  Which means that the actions always use old deps from the last run, not the updated information provided from moz.build.  And then we get to here:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/xpidl-process.py#40

and we try to open() the removed file, and things break.

I'm not sure how to fix this.  We really do want the generated deps from xpidl-process.py, because the transitive dependencies of the IDLs do matter.

Comment 8

[Nathan Froyd [:froydnj]]

I think bug 923080 broke this.  We used to generate, from moz.build:

foo.xpt: x.idl y.idl z.idl
  do-stuff x y z
bar.xpt: a.idl b.idl c.idl
  do-stuff a b c

and that would work OK when things were deleted, because the actions explicitly listed the files to use, and the actions were updated whenever we regenerated our Makefiles from moz.build.

Now we have this pattern rule:

http://mxr.mozilla.org/mozilla-central/source/config/makefiles/xpidl/Makefile.in#39

and we generate from moz.build:

foo.xpt: x.idl y.idl z.idl
bar.xpt: a.idl b.idl c.idl

which means that when our deps get overwritten, the pattern rule uses the wrong IDL files to invoke the IDL compiler.

Comment 9

[Brian O'Keefe [:bokeefe]]

Attachment: Fix xpt generation

This should help the situation. I haven't fully tested this (my copy of the tree isn't in a buildable state at the moment), but `make xpidl` in the xpidl dir was working.

I opted to emit a copy of the rules for every xpt file, like it used to be.

f? to see if you have other suggestions.

Comment 10

[Mike Hommey [:glandium]]

In make, deps are _never_ overwritten. When you have

foo: a b c
foo: d e f

then foo depends on a b c d e and f, independently of where those deps declarations are (spread over included files or not)

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #10)
> In make, deps are _never_ overwritten.

OK.  The effect in this case is the same: we end up providing a non-existent file to xpidl-process.py.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8558116 [details] [diff] [review]
Fix xpt generation

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

This seems reasonable to me.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #10)
> In make, deps are _never_ overwritten. When you have
> 
> foo: a b c
> foo: d e f
> 
> then foo depends on a b c d e and f, independently of where those deps
> declarations are (spread over included files or not)

This also means that we wind up reading the immediate dependencies of the .xpt files twice with attachment 8558116 [details] [diff] [review].  We should probably take this opportunity to fix things up.

Adding tests would be good, too.

Comment 14

[[e-mail]]

(In reply to Nathan Froyd from comment #13)
> This also means that we wind up reading the immediate dependencies of the
> .xpt files twice
I think $^ filters out duplicates ($+ does not), also I would be surprised if xpidl-process.py does no deduplication e.g. if it is passed foo.idl and bar.idl and foo.idl includes bar.idl so by the time it sees bar.idl on the command line it's already processed it.

(Before I found this bug I was thinking of using $(wildcard ...) as a workaround.)

Comment 15

[Brian O'Keefe [:bokeefe]]

Attachment: Fix xpt generation when an input file is deleted

(In reply to [e-mail] from comment #14)
> (In reply to Nathan Froyd from comment #13)
> > This also means that we wind up reading the immediate dependencies of the
> > .xpt files twice
> I think $^ filters out duplicates ($+ does not), also I would be surprised
> if xpidl-process.py does no deduplication e.g. if it is passed foo.idl and
> bar.idl and foo.idl includes bar.idl so by the time it sees bar.idl on the
> command line it's already processed it.
> 
> (Before I found this bug I was thinking of using $(wildcard ...) as a
> workaround.)

$^ does filter duplicates, but what we're doing is preventing that.
xpidl-process.py assumes everything is in $(DIST)/idl [1] which means you get things like this:

foo.xpt: srcdir/a.idl srcdir/b.idl other/dir/c.idl (in the Makefile)
foo.xpt: ../../../dist/idl/a.idl ../../../dist/idl/b.idl ../../../dist/idl/c.idl (in the dep file)

As far as make is concerned, srcdir/a.idl and $(DEPTH)/dist/idl/a.idl are different files. That means the way things are now, files get passed to xpidl-process.py twice, and it doesn't look like there's any actual filtering in there for duplicate files (unless yacc handles it internally).


I'm going to suggest that we fix things by going back to emitting the input idls directly into the rule. This patch does that (basically like the previous version, although I made the actual recipe into a define to reduce duplication). I figured out how to write a test for this too, to avoid regressing it again in the future.


[1]: Well, it's a parameter, but everything needs to be in that one directory

Comment 17

[Brian O'Keefe [:bokeefe]]

Attachment: Fix xpt generation when an input file is deleted

This needed a minor fix for the backend xpidl test, because it checks that the sources exist now.

Try with that fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb91d617724a

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8560929 [details] [diff] [review]
Fix xpt generation when an input file is deleted

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

LGTM.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +388,5 @@
>          for idl in context['XPIDL_SOURCES']:
>              yield XPIDLFile(context, mozpath.join(context.srcdir, idl),
>                  xpidl_module)
>  
> +        for symbol in ('SOURCES', 'HOST_SOURCES', 'UNIFIED_SOURCES', 'XPIDL_SOURCES'):

Thank you for this change. Any time you feel moz.build isn't doing enough validation and resulting in preventable errors, please send patches are way. We'll accept them as long as the performance hit isn't too high.

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8560929 [details] [diff] [review]
Fix xpt generation when an input file is deleted

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

::: config/makefiles/xpidl/Makefile.in
@@ +92,5 @@
> +	@echo "interface testPlugh;" > $(test_xpt_dir)/test_c.idl
> +	$(MAKE) test-xpidl TEST_INCLUDING_DELETED=1
> +	@echo "Remaking $(test_xpt_name) after deleting input"
> +	$(RM) $(test_xpt_dir)/test_c.idl
> +	$(MAKE) test-xpidl

I don't think this test is useful: it can pretty much stay stale wrt changes to the actual xpidl generation code, and fail to detect that the changes regressed this bug.
I also have an idea to fix this bug that doesn't involve restoring the one rule per xpt file pattern (even if this time it's using a variable for the command)

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8560929 [details] [diff] [review]
Fix xpt generation when an input file is deleted

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +388,5 @@
>          for idl in context['XPIDL_SOURCES']:
>              yield XPIDLFile(context, mozpath.join(context.srcdir, idl),
>                  xpidl_module)
>  
> +        for symbol in ('SOURCES', 'HOST_SOURCES', 'UNIFIED_SOURCES', 'XPIDL_SOURCES'):

This part should be a separate patch/bug, imho.

Comment 22

[Mike Hommey [:glandium]]

Attachment: Use a generated list of idl deps for xpt generation

Generating the list of idl deps to generate an xpt from its dependency list
makes us give all _previous_ dependencies, inherited from the .deps makefiles.
This leads to removed files being listed on xpidl-process.py command line, and
the command subsequently failing.

Instead, use generated lists of idl dependencies. At the same time, lighten the
generated Makefile further by not emitting xpt dependencies on their containing
directory, and instead generating it from the $xpt_files list.

This brings down the Makefile size from 100k to 38k.

Comment 23

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #22)
> This brings down the Makefile size from 100k to 38k.

As opposed to attachment 8560929 [details] [diff] [review] making it 126k.

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8563152 [details] [diff] [review]
Use a generated list of idl deps for xpt generation

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

::: config/makefiles/xpidl/Makefile.in
@@ +55,5 @@
>  xpidl:: $(xpt_files)
>  
>  $(xpt_files): $(process_py) $(call mkdir_deps,$(idl_deps_dir) $(dist_include_dir))
>  
> +$(foreach xpt,$(xpt_files),$(eval $(xpt): $(call mkdir_deps,$(dir $(xpt)))))

We /could/ create these directories in recursivemake.py so we only do this work once.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +934,5 @@
>              # reference to the new .idl. Since the new .idl presumably has
>              # an mtime newer than the .xpt, it will trigger xpt generation.
>              xpt_path = '$(DEPTH)/%s/components/%s.xpt' % (install_target, module)
>              xpt_files.add(xpt_path)
> +            mk.add_statement('%s_deps = %s' % (module, ' '.join(deps)))

Do we need a sorted() around deps to make output deterministic?

Comment 25

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #24)
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +934,5 @@
> >              # reference to the new .idl. Since the new .idl presumably has
> >              # an mtime newer than the .xpt, it will trigger xpt generation.
> >              xpt_path = '$(DEPTH)/%s/components/%s.xpt' % (install_target, module)
> >              xpt_files.add(xpt_path)
> > +            mk.add_statement('%s_deps = %s' % (module, ' '.join(deps)))
> 
> Do we need a sorted() around deps to make output deterministic?

There is a deps = sorted(sources) 15 lines above.

Comment 26

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a40aa0a281

Comment 27

[Mike Hommey [:glandium]]

Fixup, irc-reviewed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfdcb37cc953

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0a40aa0a281
https://hg.mozilla.org/mozilla-central/rev/cfdcb37cc953