747398 - X-platform build scripts attempt to rebuild generated/*.abc when they are not-writeable

Status: VERIFIED FIXED

(Tamarin Graveyard :: Build Config, defect)

Description

[Dan Schaffer]

We could not try to rebuild the generated/*.abc scripts when they are not writeable.  Most likely if they are not writeable to user does not need to rebuild them.  We could display a warning.  When a user gets a p4 copy of the branch the files will not be writeable.

Comment 1

[Dan Schaffer]

Attachment: patch check generated files are writeable

What is the underlying problem causing the timestamps to tell make a rebuild of builtin.abc and shell_toplevel.abc are necessary?  This patch would give a warning and possibly hide the problem.

Comment 2

[Dan Schaffer]

Also should we update nativegen.py (in addition to builtin.py and shell_toplevel.py)?

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 617479 [details] [diff] [review]
patch check generated files are writeable

Review of attachment 617479 [details] [diff] [review]:
-----------------------------------------------------------------

R-.  Don't you need to do the check for all the generated files, namely foo.{h,cpp,abc}, and not just the foo.abc file?

Or am I missing something?

(But, +1 to putting the check into the Python script, rather than in the Makefile as I was thinking.)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Dan Schaffer from comment #1)
> What is the underlying problem causing the timestamps to tell make a rebuild
> of builtin.abc and shell_toplevel.abc are necessary?  This patch would give
> a warning and possibly hide the problem.

The underlying problem, I believe, is that when you do a checkout in Perforce (or if you sync to various CL numbers, etc), it will populate the workspace in some order where the generated files may be put in place *before* the input sources that were used to generate them, and thus the Makefile believes that it needs to regenerate the generated files.

(This is not just a Perforce issue; it arises with Mercurial and most other VCS'es; the difference is that Perforce is checking the files out read-only and thus makes the rebuild-attempt a fatal error rather than a one-time inefficiency.)

A way to test the resulting solution is to work within a Perforce client workspace and force the timestamps for the source code outside of generated/* to get updated.  The 'touch' command may work here, and I'm pretty sure a "p4 sync -f core/* shell/*" would probably also work.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Dan Schaffer from comment #2)
> Also should we update nativegen.py (in addition to builtin.py and
> shell_toplevel.py)?

Oh yes, that would probably address my review comment #3.

Comment 6

[Brent Baker]

> What is the underlying problem causing the timestamps to tell make a rebuild
> of builtin.abc and shell_toplevel.abc are necessary?

Underlying issue is that generated/shell.* files will be written to the file system before shell/* files when cloning a brand new repo. So make will see that it needs to recompile the shell generated code since the timestamps on the source file are newer than the output of the make call. I am not sure if changing the generated/* files in perforce to have +m (Preserve local modifications times) would resolve this issue but might be something to look into?

Comment 7

[Brent Baker]

Sounds like doing +m on the generated/* would be what we want:

http://www.perforce.com/perforce/doc.current/manuals/p4guide/ab_filetypes.html
+m "Preserve original modification time"
The file's timestamp on the local file system is preserved upon submission and restored upon sync. Useful for third-party DLLs in Windows environments, because the operating system relies on the file's timestamp. By default, the modification time is set to the time you synced the file.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #5)
> (In reply to Dan Schaffer from comment #2)
> > Also should we update nativegen.py (in addition to builtin.py and
> > shell_toplevel.py)?
> 
> Oh yes, that would probably address my review comment #3.

(in other words: putting an analogous check into nativegen.py would probably suffice to get an R+ from me.  But it would be a good idea to test your patch before posting for review; i.e. duplicate the problem via one of the mechanisms described in comment 4, and then check that your patch fixes it.)

Comment 9

[Dan Schaffer]

Attachment: patch for write access check (attempt #2)

added warning for no write access in util/nativegen.py.

Comment 10

[Tamarin Bot]

changeset: 7362:d17397690ae5
user:      Dan Schaffer <[e-mail]>
summary:   bug 747398: display warning if generated/ files do not have write access and continue the build (r=flockii)

http://hg.mozilla.org/tamarin-redux/rev/d17397690ae5

Comment 11

[Tamarin Bot]

changeset: 7365:dfaf5c6d8885
user:      Felix Klock II <[e-mail]>
summary:   Bug 747398: backout CL 1055933 as it appears to have broken the Runtime CB in AVM branch (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/dfaf5c6d8885

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

changeset:   7368:39f08686bb10
user:        Dan Schaffer <[e-mail]>
date:        Tue Apr 24 11:13:24 2012 -0700
summary:     bug 7474398: check write status of generated files to avoid write errors

http://hg.mozilla.org/tamarin-redux/rev/39f08686bb10