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

RESOLVED INVALID

Status

Webtools Graveyard
Tinderbox
RESOLVED INVALID
12 years ago
4 years ago

People

(Reporter: Allan Beaufour, Assigned: Allan Beaufour)

Tracking

Trunk
PowerPC
All

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
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)
Presumably a bug in post-mozilla-rel.pl's error handling.
Assignee: build → morgamic
Component: Tinderbox Platforms → Tinderbox
Product: mozilla.org → Webtools
QA Contact: dbaron → timeless
Summary: How come atlantia shows up green with a failure? → [client] How come atlantia shows up green with a failure?
Version: other → Trunk
Assignee: morgamic → build
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>;
    }
(Assignee)

Comment 3

12 years ago
Created attachment 223683 [details] [diff] [review]
Patch

Here's a patch for it. Not sure who to ask for review, trying preed.
Assignee: build → allan
Status: NEW → ASSIGNED
Attachment #223683 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #223683 - Flags: review? → review?(preed)

Comment 4

12 years ago
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.)
Attachment #223683 - Flags: review?(preed) → review-
(Assignee)

Comment 5

12 years ago
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.
Attachment #223683 - Flags: review- → review?(preed)

Comment 6

12 years ago
(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...
(Assignee)

Comment 7

12 years ago
(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.
(Assignee)

Comment 8

12 years ago
Created attachment 224027 [details] [diff] [review]
Patch v2
Attachment #223683 - Attachment is obsolete: true
Attachment #224027 - Flags: review?(preed)
Attachment #223683 - Flags: review?(preed)

Comment 9

12 years ago
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.
Attachment #224027 - Flags: review?(preed) → review-
QA Contact: timeless → tinderbox

Comment 10

8 years ago
I don't think we're building xforms any more.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → INVALID
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.