772186 - Support return values from pymake native commands

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

This is an offshoot of bug 770702 to have pymake native commands support return values.

I think we want to interpret int return values as exit codes and all other values as whatever behavior we do today.

Comment 1

[Rain [:rain1] (inactive)]

Attachment: patch

I also added tests for failing sys.exit and return calls.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 640988 [details] [diff] [review]
patch

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

After further review, we might also consider following what Python does for sys.exit:
http://docs.python.org/library/sys.html#sys.exit
"The optional argument arg can be an integer giving the exit status (defaulting to zero), or another type of object."
"If another type of object is passed, None is equivalent to passing zero, and any other object is printed to stderr and results in an exit code of 1."

::: pymake/process.py
@@ +214,5 @@
> +            if isinstance(rv, int) and rv != 0:
> +                print >>sys.stderr, (
> +                    "Native command '%s %s' returned value '%s'" %
> +                    (self.module, self.method, rv))
> +                return -127

Do we want to propogate the return code here? If you look at PopenJob.run, we return the value of .wait() for actual shell commands.

Comment 3

[Rain [:rain1] (inactive)]

(In reply to Ted Mielczarek [:ted] from comment #2)
> Comment on attachment 640988 [details] [diff] [review]
> patch
> 
> Review of attachment 640988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After further review, we might also consider following what Python does for
> sys.exit:
> http://docs.python.org/library/sys.html#sys.exit
> "The optional argument arg can be an integer giving the exit status
> (defaulting to zero), or another type of object."
> "If another type of object is passed, None is equivalent to passing zero,
> and any other object is printed to stderr and results in an exit code of 1."

That is pretty much what I originally wanted to do, but gcp thought we might run into trouble with functions that already exist and return objects even on success. On the other hand, I don't think finding and fixing such cases is going to be an issue.

Comment 4

[Rain [:rain1] (inactive)]

Attachment: handle return values per python sys.exit (and also fix python sys.exit handling)

I prefer this one (plus going in and fixing any native commands that need to be fixed), but I'm going to leave both up and let you decide.

Comment 5

[Rain [:rain1] (inactive)]

I just did a full pymake build using the second patch (attachment 641023 [details] [diff] [review]) and had no issues. I think we should go with that.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 640988 [details] [diff] [review]
patch

I'm assuming the other patch supersedes this one.

Comment 7

[Gregory Szorc [:gps]]

(In reply to Siddharth Agarwal [:sid0] from comment #3)
> (In reply to Ted Mielczarek [:ted] from comment #2)
> > Comment on attachment 640988 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 640988 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > After further review, we might also consider following what Python does for
> > sys.exit:
> > http://docs.python.org/library/sys.html#sys.exit
> > "The optional argument arg can be an integer giving the exit status
> > (defaulting to zero), or another type of object."
> > "If another type of object is passed, None is equivalent to passing zero,
> > and any other object is printed to stderr and results in an exit code of 1."
> 
> That is pretty much what I originally wanted to do, but gcp thought we might
> run into trouble with functions that already exist and return objects even
> on success. On the other hand, I don't think finding and fixing such cases
> is going to be an issue.

I didn't realize sys.exit's behavior was what it is. I think we should copy that behavior.

Comment 8

[Rain [:rain1] (inactive)]

(Sorry for getting your nick wrong, gps. Gah, two three-letter nicks with two letters common :( )

Comment 9

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/rev/f1f9d039748c
http://hg.mozilla.org/integration/mozilla-inbound/rev/e60f929e7d6b

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e60f929e7d6b