Opened 9 years ago

Closed 9 years ago

#13113 closed defect (fixed)

sage -pkg can't handle its own options

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

Description (last modified by 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 kini 9 years ago.
apply to $SAGE_LOCAL/bin
trac_13113-root.patch (973 bytes) - added by kini 9 years ago.
apply to $SAGE_ROOT

Download all attachments as: .zip

Change History (21)

comment:1 Changed 9 years ago by kini

  • Status changed from new to needs_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 9 years ago by 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 9 years ago by kini

  • Description modified (diff)

comment:4 Changed 9 years ago by kini

  • Keywords sd41 sage-pkg added

comment:5 Changed 9 years ago by kini

  • Cc jhpalmieri added

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

comment:6 Changed 9 years ago by jhpalmieri

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 9 years ago by kini

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

comment:8 Changed 9 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

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

comment:9 Changed 9 years ago by kini

Awesome, thanks!

comment:10 Changed 9 years ago by kini

  • Status changed from positive_review to needs_work

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

Changed 9 years ago by kini

apply to $SAGE_LOCAL/bin

comment:11 Changed 9 years ago by kini

  • Status changed from needs_work to needs_review

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

comment:12 Changed 9 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

Still looks good.

comment:13 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_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 9 years ago by 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 9 years ago by kini

apply to $SAGE_ROOT

comment:15 Changed 9 years ago by kini

  • Status changed from needs_work to needs_review

New patch is up.

comment:16 Changed 9 years ago by jhpalmieri

I still think this looks good. Jeroen?

comment:17 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to positive_review

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

comment:18 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:19 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.2.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.