Opened 10 years ago

Closed 10 years ago

#13113 closed defect (fixed)

sage -pkg can't handle its own options

Reported by: Keshav Kini Owned by: Leif Leonhardy
Priority: major Milestone: sage-5.2
Component: scripts Keywords: sd41 sage-pkg
Cc: John Palmieri Merged in: sage-5.2.beta0
Authors: Keshav Kini Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Keshav Kini)

sage -pkg -n dir1 fails because the main sage command line handler actually turns this into basically sage-pkg -n && sage-pkg dir1, which is stupid since sage-pkg is already using OptParse to parse arguments and is perfectly capable of handling all of its command line arguments at once. These patches make it do so.

Attachments (2)

trac_13113-scripts.patch (4.2 KB) - added by Keshav Kini 10 years ago.
apply to $SAGE_LOCAL/bin
trac_13113-root.patch (973 bytes) - added by Keshav Kini 10 years ago.
apply to $SAGE_ROOT

Download all attachments as: .zip

Change History (21)

comment:1 Changed 10 years ago by Keshav Kini

Status: newneeds_review

The second patch is kind of hard to read - all I did between the for statement I added and the if __name__ == '__main__' at the bottom was to indent everything by four spaces.

comment:2 Changed 10 years ago by Keshav Kini

Oops, I lied - another difference is that I moved the message about what package we're creating from $SAGE_ROOT/spkg/bin/sage to $SAGE_LOCAL/bin/sage-pkg, and also I removed line 55 for obvious reasons.

The first of these fixes this stupid situation:

[1] fs-boone@zhenghe /opt/sage/local/bin $ sage -pkg --help
Creating Sage package --help
Usage: sage-pkg [options]

Options:
  -h, --help            show this help message and exit
  -n, --no-compress, --no_compress, --nocompress
                        don't compress the spkg file

(Note the second line.)

comment:3 Changed 10 years ago by Keshav Kini

Description: modified (diff)

comment:4 Changed 10 years ago by Keshav Kini

Keywords: sd41 sage-pkg added

comment:5 Changed 10 years ago by Keshav Kini

Cc: John Palmieri added

CCing jhpalmieri since he might be interested... maybe... ? :)

comment:6 Changed 10 years ago by John Palmieri

You don't need leading spaces on lines 74-75 or 77. I think the same is true for selected lines in the range 102-126. Otherwise, the patches look good in principle. I haven't actually tested them, of course...

comment:7 Changed 10 years ago by Keshav Kini

Whoops, you're right, of course. New patch in a sec

comment:8 Changed 10 years ago by John Palmieri

Reviewers: John Palmieri
Status: needs_reviewpositive_review

Looks good to me, and works the way it should.

comment:9 Changed 10 years ago by Keshav Kini

Awesome, thanks!

comment:10 Changed 10 years ago by Keshav Kini

Status: positive_reviewneeds_work

Wait, the root repo patch needs work - I should handle --pkg_nc properly.

Changed 10 years ago by Keshav Kini

Attachment: trac_13113-scripts.patch added

apply to $SAGE_LOCAL/bin

comment:11 Changed 10 years ago by Keshav Kini

Status: needs_workneeds_review

Also replicated the message from --pkg_nc in sage-pkg (new line 53 in the scripts patch).

comment:12 Changed 10 years ago by John Palmieri

Status: needs_reviewpositive_review

Still looks good.

comment:13 Changed 10 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

What's the point of the two

if [ $? -ne 0 ]; then 
    exit 1 
fi

blocks?

They could just be removed, right?

comment:14 Changed 10 years ago by Keshav Kini

Well, they were there before, so I just left them there. If you think removing them is that important, I'll update the patches...

Changed 10 years ago by Keshav Kini

Attachment: trac_13113-root.patch added

apply to $SAGE_ROOT

comment:15 Changed 10 years ago by Keshav Kini

Status: needs_workneeds_review

New patch is up.

comment:16 Changed 10 years ago by John Palmieri

I still think this looks good. Jeroen?

comment:17 Changed 10 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

Looks good to me, and I like your commit message :-)

comment:18 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.1sage-5.2

comment:19 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.2.beta0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.