Bug 166810 – Avoid triggering rebuilds for minor changes of CSSProperties.json

• Component: New Bugs
• Status: REOPENED

[Simon Fraser (smfr)]

Avoid triggering rebulids for minor changes of CSSProperties.json

[Simon Fraser (smfr)]

Created attachment 298281 [details]
Patch

[Daniel Bates]

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.

[WebKit Commit Bot]

Comment on attachment 298281 [details]
Patch

Clearing flags on attachment: 298281

Committed r210493: <http://trac.webkit.org/changeset/210493>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

There's a hack involving md5 checksum files described here: https://www.cmcrossroads.com/article/rebuilding-when-files-checksum-changes

[Simon Fraser (smfr)]

Almost identical hack at http://blog.jgc.org/2006/04/rebuilding-when-hash-has-changed-not.html

[Simon Fraser (smfr)]

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.

[Darin Adler]

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.

[Simon Fraser (smfr)]

I'll roll it out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 166842

[Simon Fraser (smfr)]

Created attachment 298974 [details]
Non-working patch