Avoid triggering rebulids for minor changes of CSSProperties.json
Created attachment 298281 [details] Patch
Comment on attachment 298281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298281&action=review r=me > Source/WebCore/css/makeprop.pl:184 > + if (compare($tempFile, $file) != 0) { I take it you feel the "!= 0" improves the readability of this code.
Comment on attachment 298281 [details] Patch Clearing flags on attachment: 298281 Committed r210493: <http://trac.webkit.org/changeset/210493>
All reviewed patches have been landed. Closing bug.
Comment on attachment 298281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298281&action=review Here’s a problem that will slow down builds that I think we have after this patch: Lets say we edit CSSProperties.json so that it is newer than CSSPropertyNames.h, but our edit does not affect the contents of CSSPropertyNames.h. We build and the new version of this script will correctly leave CSSPropertyNames.h unmodified. Now we build again without editing CSSProperties.json. Unfortunately, the make file sees that CSSPropertyNames.h is older than CSSProperties.json so it runs makeprop.pl again. That means we are now going to run makeprop.pl every time we build WebKit, once this happens. I don’t know how to make correct rules in a makefile for case like this; but I am pretty sure the problem is real. I would love to find a good solution for the problem that we can use in this case and that we can use in other similar cases. > Source/WebCore/css/makeprop.pl:185 > + copy($tempFile, $file) or die "Failed to copy $tempFile to $file: $!"; This should be a move, rather than a copy followed by an unlink. > Source/WebCore/css/makeprop.pl:192 > +open GPERF, ">$gperfTempFile" || die "Could not open $gperfTempFile for writing"; These problems both existed before this patch: It’s cleaner to write: open GPERF, ">", $gperfTempFile or die ... Including the ">" in the filename was needed in old versions of perl, but in new ones we don’t need to do that. I also suspect that "|| die" is wrong and this needs to use "or die" because of operator precedence. I’m pretty sure that the "||" will be part of evaluating the argument to open, not something evaluated based on the return value of open. This same thing applies to code below. > Source/WebCore/css/makeprop.pl:351 > +my $properyNamesHeaderTempFile = "CSSPropertyNames.h.tmp"; There’s a typo in this variable name and all the uses of it, "propery" without the "t".
(In reply to comment #5) > Comment on attachment 298281 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298281&action=review > > Here’s a problem that will slow down builds that I think we have after this > patch: > > Lets say we edit CSSProperties.json so that it is newer than > CSSPropertyNames.h, but our edit does not affect the contents of > CSSPropertyNames.h. We build and the new version of this script will > correctly leave CSSPropertyNames.h unmodified. Now we build again without > editing CSSProperties.json. Unfortunately, the make file sees that > CSSPropertyNames.h is older than CSSProperties.json so it runs makeprop.pl > again. > > That means we are now going to run makeprop.pl every time we build WebKit, > once this happens. True. Maybe the fix is to keep the intermediate files around and involve them in make dependencies, or use timestamp files. > I don’t know how to make correct rules in a makefile for case like this; but > I am pretty sure the problem is real. I would love to find a good solution > for the problem that we can use in this case and that we can use in other > similar cases. > > > Source/WebCore/css/makeprop.pl:185 > > + copy($tempFile, $file) or die "Failed to copy $tempFile to $file: $!"; > > This should be a move, rather than a copy followed by an unlink. > > > Source/WebCore/css/makeprop.pl:192 > > +open GPERF, ">$gperfTempFile" || die "Could not open $gperfTempFile for writing"; > > These problems both existed before this patch: > > It’s cleaner to write: > > open GPERF, ">", $gperfTempFile or die ... > > Including the ">" in the filename was needed in old versions of perl, but in > new ones we don’t need to do that. > > I also suspect that "|| die" is wrong and this needs to use "or die" because > of operator precedence. I’m pretty sure that the "||" will be part of > evaluating the argument to open, not something evaluated based on the return > value of open. > > This same thing applies to code below. > > > Source/WebCore/css/makeprop.pl:351 > > +my $properyNamesHeaderTempFile = "CSSPropertyNames.h.tmp"; > > There’s a typo in this variable name and all the uses of it, "propery" > without the "t". I'll clean those up.
There's a hack involving md5 checksum files described here: https://www.cmcrossroads.com/article/rebuilding-when-files-checksum-changes
Almost identical hack at http://blog.jgc.org/2006/04/rebuilding-when-hash-has-changed-not.html
A side effect of always running makeprops.pl is that CSSPropertyNames.cpp is regenerated each time. I think this step needs to be moved into the Makefile.
I’m really concerned about leaving this patch in. It’s nice to make the single build after modifying CSSProperties.json faster, but doing it this way makes all the builds from then on slower; we should turn this behavior off or roll it out until we solve the problem.
I'll roll it out.
Re-opened since this is blocked by bug 166842
Created attachment 298974 [details] Non-working patch