332535 - don't update nsBuildID.h timestamp if build id is the same

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brian Ryner (not reading)]

Updating the timestamp on this header causes all sorts of things to recompile, even if the build id hasn't changed.  We should compare to the old file when generating the build id, and only replace it if there's a difference.

Comment 1

[Brian Ryner (not reading)]

Attachment: fix

The fix is actually for build_number, which nsBuildID.h is generated from.

Comment 2

[Benjamin Smedberg]

Comment on attachment 216991 [details] [diff] [review]
fix

If diff -q is portable we should probably use that.

Comment 3

[Brian Ryner (not reading)]

(In reply to comment #2)
> (From update of attachment 216991 [details] [diff] [review] [edit])
> If diff -q is portable we should probably use that.
> 

We don't use it anywhere else, I'm guessing that it's not.

Comment 4

[Brian Ryner (not reading)]

checked in

Comment 5

[Brian Ryner (not reading)]

Attachment: fix non-bash shells

Apparently, non-bash shells don't have "!".  This patch avoids using it.

Comment 6

[jag (Peter Annema)]

Attachment: Alternative fix

No need for the shell stuff, let's do this in perl.

Comment 7

[jag (Peter Annema)]

Attachment: diff -uw of above

Comment 8

[jag (Peter Annema)]

I also have this in a local tree, though I guess that's unneeded now:

@@ -39,6 +39,7 @@

 use strict;
 use IO::File;
+use File::Compare;

 BEGIN {
     use Exporter ();
@@ -158,8 +157,13 @@
     close $INFILE;
     close $OUTFILE;

-    unlink $outfile;
-    rename "${outfile}.old", "$outfile";
+    if (compare("$outfile", "${outfile}.old") == 0) {
+        unlink "${outfile}.old";
+    }
+    else {
+        unlink $outfile;
+        rename "${outfile}.old", "$outfile";
+    }
 }

 sub SetMilestone($) {

Comment 9

[jag (Peter Annema)]

I've checked in attachment 217128 [details] [diff] [review].

Comment 10

[Brian Ryner (not reading)]

(In reply to comment #9)
> I've checked in attachment 217128 [details] [diff] [review] [edit].
> 

Who reviewed it?

I considered doing it in the perl script to start with, but I figured let's keep the perl script simple -- it outputs the build id to a file.  Anything else we want to do is more in the realm of how we generate that file at build time.

Comment 11

[Benjamin Smedberg]

Comment on attachment 217124 [details] [diff] [review]
fix non-bash shells

bryner's fix is better. Let's back out the perl hackery which should not have been checked in without review anyway.

Comment 12

[jag (Peter Annema)]

No review (though I did have it looked at before checking in) since it was a simple bustage fix.

I went with the perl solution because it was already avoiding when possible to overwrite the target file in the unofficial builds case, and making it do the same for official builds was trivial and seemed simpler than always writing it out and dealing with not changing the modification time in the Makefile.

I hear you on keeping tools simple, though I'd argue my change hasn't made this tool more complicated. The output of the tool is still a file (if so desired) with the requested string (YYYYMMDDHH or 10 zeroes).

Let me know if you want me to back this out, or feel free to do so yourself.

Comment 13

[jag (Peter Annema)]

(In reply to comment #11)
> (From update of attachment 217124 [details] [diff] [review] [edit])
> Let's back out the perl hackery

You make it sound like hacking perl is something awful! ;-)

And I respectfully disagree that bryner's fix is better, but I guess that's a matter of taste.

Comment 14

[jag (Peter Annema)]

Attachment: If you're intent on undoing the perl "hackery"

Then you probably would also like to remove the (then) redundant check in the unofficial build case.

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 217128 [details] [diff] [review]
Alternative fix

Should have read closer, I didn't realize it already had this behavior for unofficial builds.  r=me.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

+    # Only overwrite file if contents are not already set to 0

that comment is now wrong... it should say something like that it only overwrites it if the contents are different.

Comment 17

[jag (Peter Annema)]

Attachment: Fix comment