An idl file was removed.

Bug 1066383 is fixed.

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

松浦さん, please see comment 2.

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

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

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.

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.

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.

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.

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)

(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 on attachment 8558116 [details] [diff] [review] Fix xpt generation Review of attachment 8558116 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable to me.

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

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

(In reply to neil@parkwaycc.co.uk 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

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

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.

(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 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?

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

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

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