Bug 117900 – IDL dependencies are always regenerated, even if nothing has changed

• Component: New Bugs
• Status: NEW

[Maciej Stachowiak]

IDL dependencies are always regenerated, even if nothing has changed

[Maciej Stachowiak]

Created attachment 205228 [details]
Patch

[Chris Dumez]

Comment on attachment 205228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205228&action=review

I cannot test this on mac but I did similar tests with CMake and I had issues. Could you please try the following?
- Do a full build
- Edit Source/WebCore/page/History.idl and add a [NoInterfaceObject] extended attribute to the IDL interface
- Do an incremental build

What should happen is that:
a) JSHistory.* are regenerated.
b) JSDOMWindow.* should be regenerated so that the Window object no longer has a 'History' property.

On CMake, a) was happening but not b), which is a real issue.
It may be that I did it wrong or differently with CMake so it would be good if you could double-check on mac that your patch doesn't do the same.

> Source/WebCore/DerivedSources.make:1026
> +JS%.h : %.idl $(JS_BINDINGS_SCRIPTS) $(IDL_ATTRIBUTES_FILE) $(PLATFORM_FEATURE_DEFINES)

Why doesn't this depend on $(SUPPLEMENTAL_DEPENDENCY_FILE)?

[Maciej Stachowiak]

(In reply to comment #2)
> (From update of attachment 205228 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205228&action=review
> 
> I cannot test this on mac but I did similar tests with CMake and I had issues. Could you please try the following?
> - Do a full build
> - Edit Source/WebCore/page/History.idl and add a [NoInterfaceObject] extended attribute to the IDL interface
> - Do an incremental build
> 
> What should happen is that:
> a) JSHistory.* are regenerated.
> b) JSDOMWindow.* should be regenerated so that the Window object no longer has a 'History' property.
> 
> On CMake, a) was happening but not b), which is a real issue.
> It may be that I did it wrong or differently with CMake so it would be good if you could double-check on mac that your patch doesn't do the same.

It worked as expected.

> 
> > Source/WebCore/DerivedSources.make:1026
> > +JS%.h : %.idl $(JS_BINDINGS_SCRIPTS) $(IDL_ATTRIBUTES_FILE) $(PLATFORM_FEATURE_DEFINES)
> 
> Why doesn't this depend on $(SUPPLEMENTAL_DEPENDENCY_FILE)?

It hasn't since I fixed IDLs to not rebuild every time in the first place. The reason is that SupplementalDependencies.dep already captures the dependencies in a fine-grained way. DerivedSources.make includes it. So for each header, it knows the actual IDL files it depends on. If the dependency graph changes, then the dependency IDL files will also change. So if you add Supplemental to an IDL file the correct file that became supplemented will rebuild, but not others. Or if you change a file that is already supplemental, the interface supplemented will rebuild.

Unfortunately I am not sure any build systems besides 'make' can do this trick.

[Chris Dumez]

Ok, the only issue is that since you revert r151675, some ports will regress and start regenerating all the bindings whenever an IDL is touched. I'll take a look at CMake tomorrow and see if I can match what you're doing for Mac.

Does it hurt to keep r151675 in for now? I understand this is not the nicest approach but at least it still improves the situation a lot for other build systems.

[Mark Rowe (bdash)]

Comment on attachment 205228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205228&action=review

> Source/WebCore/bindings/scripts/preprocess-idls.pl:135
> +    my @all_dependencies = [];

Is all_dependencies used anywhere? It seems to be new in this change but I don't see any other references to it.

[Chris Dumez]

Comment on attachment 205228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205228&action=review

>> Source/WebCore/bindings/scripts/preprocess-idls.pl:135
>> +    my @all_dependencies = [];
> 
> Is all_dependencies used anywhere? It seems to be new in this change but I don't see any other references to it.

Right, it is unused which is why I removed it as well in r151675 (that this patch reverts).

[Maciej Stachowiak]

(In reply to comment #4)
> Ok, the only issue is that since you revert r151675, some ports will regress and start regenerating all the bindings whenever an IDL is touched. I'll take a look at CMake tomorrow and see if I can match what you're doing for Mac.
> 
> Does it hurt to keep r151675 in for now? I understand this is not the nicest approach but at least it still improves the situation a lot for other build systems.

It makes things worse for Mac, though, since it causes files to always be regenerated. The point of this change is to avoid doing that.

And for other ports, correct IDL dependencies never worked. In fact, to some extent they still don't; if you add a new supplemental source, all IDL files will be recompiled for build systems that don't use DerivedSources.make.

I think the best solution is for other ports to use DerivedSources.make to build generated dependencies. None of the higher level build systems are flexible enough to do it really right, and creating new kinds of derived sources is super hard if you have to code it for multiple build systems. I'll start a webkit-dev thread on that topic.

(In reply to comment #6)
> (From update of attachment 205228 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205228&action=review
> 
> >> Source/WebCore/bindings/scripts/preprocess-idls.pl:135
> >> +    my @all_dependencies = [];
> > 
> > Is all_dependencies used anywhere? It seems to be new in this change but I don't see any other references to it.
> 
> Right, it is unused which is why I removed it as well in r151675 (that this patch reverts).

I can re-remove it. I just reverted the patch blind.

[Chris Dumez]

(In reply to comment #7)
> (In reply to comment #4)
> > Ok, the only issue is that since you revert r151675, some ports will regress and start regenerating all the bindings whenever an IDL is touched. I'll take a look at CMake tomorrow and see if I can match what you're doing for Mac.
> > 
> > Does it hurt to keep r151675 in for now? I understand this is not the nicest approach but at least it still improves the situation a lot for other build systems.
> 
> It makes things worse for Mac, though, since it causes files to always be regenerated. The point of this change is to avoid doing that.
> 
> And for other ports, correct IDL dependencies never worked. In fact, to some extent they still don't; if you add a new supplemental source, all IDL files will be recompiled for build systems that don't use DerivedSources.make.
> 
> I think the best solution is for other ports to use DerivedSources.make to build generated dependencies. None of the higher level build systems are flexible enough to do it really right, and creating new kinds of derived sources is super hard if you have to code it for multiple build systems. I'll start a webkit-dev thread on that topic.

Ok, I did not realize that r151675 made things worse on Mac, sorry.

One possibility would be to add a --writeOnlyIfChanged argument to preprocess-idls.pl so that some build systems (such as CMake) can choose to use r151675, at least for now. So far, I haven't noticed any issue with r151675 and CMake.

[Maciej Stachowiak]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #4)

> 
> Ok, I did not realize that r151675 made things worse on Mac, sorry.

Well, worse in one way and better in another. 

> 
> One possibility would be to add a --writeOnlyIfChanged argument to preprocess-idls.pl so that some build systems (such as CMake) can choose to use r151675, at least for now. So far, I haven't noticed any issue with r151675 and CMake.

That sounds like an ok way to go for now. I'll try to rework the patch that way.

[Joseph Pecoraro]

Comment on attachment 205228 [details]
Patch

It seems this patch has gotten stale, clearing flags. Does the original issue still reproduce?