Last Comment Bug 772186 - Support return values from pymake native commands
: Support return values from pymake native commands
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks: 680636
  Show dependency treegraph
 
Reported: 2012-07-09 12:53 PDT by Gregory Szorc [:gps]
Modified: 2012-07-12 09:36 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.14 KB, patch)
2012-07-11 02:56 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
handle return values per python sys.exit (and also fix python sys.exit handling) (4.36 KB, patch)
2012-07-11 05:53 PDT, Siddharth Agarwal [:sid0] (inactive)
khuey: review+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2012-07-09 12:53:27 PDT
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 Siddharth Agarwal [:sid0] (inactive) 2012-07-11 02:56:12 PDT
Created attachment 640988 [details] [diff] [review]
patch

I also added tests for failing sys.exit and return calls.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-07-11 05:03:05 PDT
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 Siddharth Agarwal [:sid0] (inactive) 2012-07-11 05:07:48 PDT
(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 Siddharth Agarwal [:sid0] (inactive) 2012-07-11 05:53:51 PDT
Created attachment 641023 [details] [diff] [review]
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 Siddharth Agarwal [:sid0] (inactive) 2012-07-11 08:18:19 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-11 09:32:51 PDT
Comment on attachment 640988 [details] [diff] [review]
patch

I'm assuming the other patch supersedes this one.
Comment 7 Gregory Szorc [:gps] 2012-07-11 11:01:20 PDT
(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 Siddharth Agarwal [:sid0] (inactive) 2012-07-11 21:08:24 PDT
(Sorry for getting your nick wrong, gps. Gah, two three-letter nicks with two letters common :( )
Comment 10 Ed Morley [:emorley] 2012-07-12 09:36:35 PDT
https://hg.mozilla.org/mozilla-central/rev/e60f929e7d6b

Note You need to log in before you can comment on or make changes to this bug.