339201 - [client] How come atlantia shows up green with a failure?

Status: RESOLVED INVALID

(Webtools Graveyard :: Tinderbox, defect)

Description

[Allan Beaufour]

atlantia has not been building xforms.xpi since May 4th, but it still shows up green. Why is that?
http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&errorparser=unix&logfile=1148541960.8376.gz&buildtime=1148541960&buildname=MacOSX%20Darwin%207.9.0%20atlantia%20Depend%20release&fulltext=1

(Getting it to build again is bug 339198)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Presumably a bug in post-mozilla-rel.pl's error handling.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In particular, there are no error checks here, but the results of run_shell_command should probably be checked (and probably the presence of the xpi as well):

    if ($Settings::BuildXForms) {
      TinderUtils::run_shell_command "cd $builddir/extensions/xforms; $builddir/build/autoconf/make-makefile -t $builddir -d ../..";
      TinderUtils::run_shell_command "make -C $builddir/extensions/xforms";

      @xforms_xpi = grep { -f $_ } <${builddir}/dist/xpi-stage/xforms.xpi>;
    }

Comment 3

[Allan Beaufour]

Attachment: Patch

Here's a patch for it. Not sure who to ask for review, trying preed.

Comment 4

[J. Paul Reed [:preed]]

Comment on attachment 223683 [details] [diff] [review]
Patch

You *probably* don't want to return() if there's an error; just set the status to 0, and let the function continue, returning the $status at the end.

(Also, if you wanted to indicate failure this way, you'd return 1; see the comment at the end of the function.)

Comment 5

[Allan Beaufour]

Comment on attachment 223683 [details] [diff] [review]
Patch

(In reply to comment #4)
> (From update of attachment 223683 [details] [diff] [review] [edit])
> You *probably* don't want to return() if there's an error; just set the status
> to 0, and let the function continue, returning the $status at the end.

It does not make much sense to continue with the function, since it only involves 1) handling the xforms.xpi (which is not built on error) and 2) updating stuff when there is no error. If you really want I can keep it running, but if somebody later adds a command and overrides the $status, then the error is possibly lost.

> (Also, if you wanted to indicate failure this way, you'd return 1; see the
> comment at the end of the function.)

No. Returning 0 is signaling an error condition. packit() returns true/false, the comment talks about $status which return 0 on "no error" and something else on "error". Maybe we should change the comment, it confused me too :)

Re-requesting review.

Comment 6

[J. Paul Reed [:preed]]

(In reply to comment #5)
> (From update of attachment 223683 [details] [diff] [review] [edit])
> (In reply to comment #4)
> > (From update of attachment 223683 [details] [diff] [review] [edit] [edit])
> > You *probably* don't want to return() if there's an error; just set the status
> > to 0, and let the function continue, returning the $status at the end.
> 
> It does not make much sense to continue with the function, since it only
> involves 1) handling the xforms.xpi (which is not built on error) and 2)
> updating stuff when there is no error. If you really want I can keep it
> running, but if somebody later adds a command and overrides the $status, then
> the error is possibly lost.

I'm more concerned about people adding stuff at the bottom of the function that has nothing to do with xforms that they think will always get run. The function is packit(), not xforms_packit().

(You're welcome to break it out into an xforms_packit(), though. ;-)

> No. Returning 0 is signaling an error condition. packit() returns true/false,
> the comment talks about $status which return 0 on "no error" and something else
> on "error". Maybe we should change the comment, it confused me too :)

Yah, it probably should be... I didn't work through the logic because I was in a hurry, I just glanced at the comment; if the comment is wrong...

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 223683 [details] [diff] [review] [edit] [edit])
> > (In reply to comment #4)
> > > (From update of attachment 223683 [details] [diff] [review] [edit] [edit] [edit])
> > > You *probably* don't want to return() if there's an error; just set the status
> > > to 0, and let the function continue, returning the $status at the end.
> > 
> > It does not make much sense to continue with the function, since it only
> > involves 1) handling the xforms.xpi (which is not built on error) and 2)
> > updating stuff when there is no error. If you really want I can keep it
> > running, but if somebody later adds a command and overrides the $status, then
> > the error is possibly lost.
> 
> I'm more concerned about people adding stuff at the bottom of the function that
> has nothing to do with xforms that they think will always get run. The function
> is packit(), not xforms_packit().

Well, I also missed the fact that a lot other than xforms is going on there... sorry for that. New patch coming up.

Comment 8

[Allan Beaufour]

Attachment: Patch v2

Comment 9

[J. Paul Reed [:preed]]

Comment on attachment 224027 [details] [diff] [review]
Patch v2

I still don't think this is exactly what we want.

I talked through this with rhelmer, and the problem is that this patch clobbers the previous valueof $status, which has implications for update_create_package() not running.

I think you want to get the status of the xforms commands, and only if they're false, then set status to them. What I'm worried about is status up above being set to false, then these commands running, and clobbering the value with true, and then update_package_create() getting called when it shouldn't.

Comment 10

[Jesse Ruderman]

I don't think we're building xforms any more.