The default bug view has changed. See this FAQ.

Support return values from pymake native commands

RESOLVED FIXED in mozilla16

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: sid0)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

5 years ago
Created attachment 640988 [details] [diff] [review]
patch

I also added tests for failing sys.exit and return calls.
Assignee: nobody → sagarwal
Status: NEW → ASSIGNED
Attachment #640988 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
Blocks: 680636
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.
(Assignee)

Comment 3

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

Comment 4

5 years ago
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.
Attachment #641023 - Flags: review?(khuey)
(Assignee)

Comment 5

5 years ago
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.
Attachment #641023 - Flags: review?(khuey) → review+
Comment on attachment 640988 [details] [diff] [review]
patch

I'm assuming the other patch supersedes this one.
Attachment #640988 - Flags: review?(khuey)
(Reporter)

Comment 7

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

Updated

5 years ago
Attachment #640988 - Attachment is obsolete: true
(Assignee)

Comment 8

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

Comment 9

5 years ago
http://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/rev/f1f9d039748c
http://hg.mozilla.org/integration/mozilla-inbound/rev/e60f929e7d6b
https://hg.mozilla.org/mozilla-central/rev/e60f929e7d6b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.