1420119 - Changes to accessibility IDL files should not require a clobber

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[James Teh [:Jamie]]

When changes are made to accessibility IDL files, a clobber build is required to avoid build failures. For example, bug 1416986 introduced changes to accessible/ipc/win/handler/handlerData.idl, causing build failures described in bug 1419814 and thus requiring a clobber. This is most certainly not the last time this will happen. This is nasty and needs fixing.

Unfortunately, at this stage, I don't have a clue where to even start. :(

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch for a11y handler idl

This patch fixes an error in the INSTALL_TARGETS stuff inside the handler's Makefile.

When applied, this *should* mean that you just have to rebuild the export tier to pick up the IDL changes.

I think this bug exists in other IDL Makefiles in the a11y tree, but let's start with this one.

Note that the documentation for INSTALL_TARGETS is located at:
https://searchfox.org/mozilla-central/source/config/rules.mk#1344

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Hmm, a newly rebased build still fails with this patch. Further investigating...

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Oooh, I think I've fixed this! We can get rid of the INSTALL_TARGETS crap in the makefile and just use EXPORTS in moz.build!

Once I've confirmed this with a finished build, I'll post patches.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Fix makefiles containing midl

This one looks better...

Comment 6

[James Teh [:Jamie]]

Comment on attachment 8948816 [details] [diff] [review]
Fix makefiles containing midl

I only very loosely understand what this is doing; I don't quite understand how Makefile and moz.build interact. In principle, it makes sense to me.

Unfortunately, this doesn't seem to work for me. After applying your patch, I built mozilla-central. Then, I reverted the patches from bug 1431264. Running mach build after that failed:

HandlerProvider.cpp
c:/Users/jamie/src/gecko/accessible/ipc/win/HandlerProvider.cpp(494): error C2259: 'mozilla::a11y::HandlerProvider': cannot instantiate abstract class
c:/Users/jamie/src/gecko/accessible/ipc/win/HandlerProvider.cpp(494): note: due to following members:
c:/Users/jamie/src/gecko/accessible/ipc/win/HandlerProvider.cpp(494): note: 'HRESULT IGeckoBackChannel::get_RelationsInfo(IARelationData **,long *)': is abstract
c:\Users\jamie\src\gecko\obj32\dist\include\HandlerData.h(409): note: see declaration of 'IGeckoBackChannel::get_RelationsInfo'

If I run mach build again, it succeeds. So, the build system seems to know it needs to re-build the idl and its generated output, but it doesn't do it before it gets to HandlerProvider.cpp. That doesn't make sense to me; as I understand it, everything in the export tier should be run before the libs tier (and presumably, HandlerProvider.cpp is in the libs tier).

Comment 7

[Gregory Szorc [:gps]]

mshal: would you mind having a look at this? It seems relevant to Tup and overall work to shore up the dependencies.

Comment 8

[Michael Shal [:mshal]]

Sure, I can take a look.

Comment 9

[Michael Shal [:mshal]]

Although we do want to move this stuff to moz.build (:aklotz - maybe you can take a look at bug 881446, and also https://bugzilla.mozilla.org/show_bug.cgi?id=1435889#c1), unfortunately the problem is with the part that's still in the Makefiles. The issue is that make does some timestamp caching, and it doesn't know that HandlerData.h was actually changed by the midl rule, and so it thinks it is still out of date even though the install target for dist/include/HandlerData.h runs after midl. The relevant dependencies from the Makefile and rules.mk look like this (simplified):

export: HandlerData.h ../../../../dist/include/HandlerData.h midl_done

../../../../dist/include/HandlerData.h: HandlerData.h
        (install rule to copy into dist/include)

HandlerData.h: midl_done
# Note: No recipe here - just a dependency!

midl_done: HandlerData.idl
        (run midl on the idl to create HandlerData.h and similar files)

When we run 'make export' in this directory, make checks the timestamp of HandlerData.h (since it is a prerequisite of export), and caches its value. It also sees that dist/include/HandlerData.h depends on HandlerData.h, which depends on midl_done, so it runs the midl rule first. However, since $@ for this rule is midl_done, and there is no corresponding recipe in the line for "HandlerData.h: midl_done", make doesn't know it should invalidate the cached timestamp of HandlerData.h. So when it then processes the install target for the dist/include version, it still thinks HandlerData.h is older than the installed version, even though the file is actually newer.

I think the solution is to just add some recipe text to the '$(MIDL_GENERATED_FILES): midl_done' lines, so that make will know to update the timestamps in its caches for these files. Adding a 'touch $@' is probably most explicit, so that's what I'm currently testing on try. Though even just an 'echo hello world' is enough to tell make that the files are actually being updated and should update the cache.

:Jamie, can you give https://hg.mozilla.org/try/rev/3cf935028384398fbb3e4aa91f3f3f193677e084 a test to confirm it's working?

Comment 10

[Mike Hommey [:glandium]]

You shouldn't need commands at all.

$(MIDL_GENERATED_FILES): midl_done ;

should do the deed (with the semi-colon)

Comment 11

[Michael Shal [:mshal]]

Yeah, that will work fine too. Either way is fine with me, as long as we make sure not to regress the behavior. I think if we had a midl template in moz.build like ted suggests in https://bugzilla.mozilla.org/show_bug.cgi?id=1435889#c1, it would be easier to add a test case for this.

Comment 12

[Michael Shal [:mshal]]

Attachment: Bug 1420119 - avoid issues with make's timestamp caching;

When an idl file is updated, files like HandlerData.h are generated as
part of the midl target (eg: midl_done). However, Make may have already
stat'd HandlerData.h and cached its timestamp. Although there is a
dependency from HandlerData.h on midl_done, there is no recipe. As such,
Make assumes that HandlerData.h hasn't actually changed, and uses the
cached value of the timestamp when determining if it should install the
file into dist/include. If the cached value is older, make may not
trigger the install rule, leaving the old header in place and breaking
the build.

Review commit: https://reviewboard.mozilla.org/r/220206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220206/

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8950945 [details]
Bug 1420119 - avoid issues with make's timestamp caching;

https://reviewboard.mozilla.org/r/220206/#review226192

I think this will address the issue, but I don't think your diagnosis is correct.  As in https://searchfox.org/mozilla-central/source/mobile/android/base/Makefile.in#371-375, there's a Make weirdness where-by `target: dep` is treated differently from `target: dep\n\trule`.  You can recover the empty rule semantics that make sense with `target: dep ;`.

I think using the `;` is cleaner than your solution, but either should work, and I'm fine with you landing this.  If I'm not understanding what's really happening, can you test my proposed solution and explain where I'm wrong?

Comment 14

[Michael Shal [:mshal]]

(In reply to Nick Alexander :nalexander from comment #13)
> I think this will address the issue, but I don't think your diagnosis is
> correct.  As in
> https://searchfox.org/mozilla-central/source/mobile/android/base/Makefile.
> in#371-375, there's a Make weirdness where-by `target: dep` is treated
> differently from `target: dep\n\trule`.  You can recover the empty rule
> semantics that make sense with `target: dep ;`.

What part is not correct? I can update the commit message (requesting ni? for this).

> I think using the `;` is cleaner than your solution, but either should work,
> and I'm fine with you landing this.  If I'm not understanding what's really
> happening, can you test my proposed solution and explain where I'm wrong?

Yeah, as long as there is any recipe it will work. If there's no recipe, make doesn't update the timestamp cache, which breaks the install target rules. I'm fine with just making it a ';' (see also #c10 / #c11). I'll update the patch.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Shal [:mshal] from comment #14)
> (In reply to Nick Alexander :nalexander from comment #13)
> > I think this will address the issue, but I don't think your diagnosis is
> > correct.  As in
> > https://searchfox.org/mozilla-central/source/mobile/android/base/Makefile.
> > in#371-375, there's a Make weirdness where-by `target: dep` is treated
> > differently from `target: dep\n\trule`.  You can recover the empty rule
> > semantics that make sense with `target: dep ;`.
> 
> What part is not correct? I can update the commit message (requesting ni?
> for this).

Sorry, I wasn't clear: I meant that your conclusion about Make's timestamp cache is new to me.  Maybe it's correct, since I've just internalized that "no rule == bad things" and not the details of the cache.  Are you confident that's what "no rule" is actually doing?

> > I think using the `;` is cleaner than your solution, but either should work,
> > and I'm fine with you landing this.  If I'm not understanding what's really
> > happening, can you test my proposed solution and explain where I'm wrong?
> 
> Yeah, as long as there is any recipe it will work. If there's no recipe,
> make doesn't update the timestamp cache, which breaks the install target
> rules. I'm fine with just making it a ';' (see also #c10 / #c11). I'll
> update the patch.

Ah, I see, glandium already said the same :)  I'd rather see fewer touch commands and more knowledge of Make's weird `;` disseminated.  r+ with that.

Comment 16

[Michael Shal [:mshal]]

(In reply to Nick Alexander :nalexander from comment #15)
> (In reply to Michael Shal [:mshal] from comment #14)
> > (In reply to Nick Alexander :nalexander from comment #13)
> > > I think this will address the issue, but I don't think your diagnosis is
> > > correct.  As in
> > > https://searchfox.org/mozilla-central/source/mobile/android/base/Makefile.
> > > in#371-375, there's a Make weirdness where-by `target: dep` is treated
> > > differently from `target: dep\n\trule`.  You can recover the empty rule
> > > semantics that make sense with `target: dep ;`.
> > 
> > What part is not correct? I can update the commit message (requesting ni?
> > for this).
> 
> Sorry, I wasn't clear: I meant that your conclusion about Make's timestamp
> cache is new to me.  Maybe it's correct, since I've just internalized that
> "no rule == bad things" and not the details of the cache.  Are you confident
> that's what "no rule" is actually doing?

I saw via 'make -d' that HandlerData.h was being checked by make, then created by the midl rule, and then when processing the dist/include install rule, make declares that HandlerData.h is older (even though it was just created, and in fact newer). I found this make bug that describes the timestamp caching issue: http://savannah.gnu.org/bugs/?46596

It's marked as a dupe of http://savannah.gnu.org/bugs/?41273, but the first one is much more similar to how the midl Makefiles are structured.

Comment 17

[Michael Shal [:mshal]]

Comment on attachment 8950945 [details]
Bug 1420119 - avoid issues with make's timestamp caching;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220206/diff/1-2/

Comment 18

[James Teh [:Jamie]]

Sorry for the slow response. I tested your latest patch (from comment 12). I applied your patch, built, reverted the patches from bug 1431264 (which change HandlerData.idl), built again, then re-applied the patches from bug 1431264 and built a third time. All builds were successful. Thanks so much for the fix.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4b14a5665b4
avoid issues with make's timestamp caching; r=nalexander

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c4b14a5665b4