IDL dependencies are always regenerated, even if nothing has changed
Created attachment 205228 [details] Patch
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)?
(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.
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.
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.
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).
(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.
(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.
(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.
Comment on attachment 205228 [details] Patch It seems this patch has gotten stale, clearing flags. Does the original issue still reproduce?