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: |
Description (last modified by )
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)
Change History (21)
comment:1 Changed 10 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 10 years ago by
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
Description: | modified (diff) |
---|
comment:4 Changed 10 years ago by
Keywords: | sd41 sage-pkg added |
---|
comment:5 Changed 10 years ago by
Cc: | John Palmieri added |
---|
CCing jhpalmieri since he might be interested... maybe... ? :)
comment:6 Changed 10 years ago by
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:8 Changed 10 years ago by
Reviewers: | → John Palmieri |
---|---|
Status: | needs_review → positive_review |
Looks good to me, and works the way it should.
comment:10 Changed 10 years ago by
Status: | positive_review → needs_work |
---|
Wait, the root repo patch needs work - I should handle --pkg_nc properly.
comment:11 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
Also replicated the message from --pkg_nc
in sage-pkg
(new line 53 in the scripts patch).
comment:13 Changed 10 years ago by
Status: | positive_review → 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 10 years ago by
Well, they were there before, so I just left them there. If you think removing them is that important, I'll update the patches...
comment:17 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
Looks good to me, and I like your commit message :-)
comment:18 Changed 10 years ago by
Milestone: | sage-5.1 → sage-5.2 |
---|
comment:19 Changed 10 years ago by
Merged in: | → sage-5.2.beta0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
The second patch is kind of hard to read - all I did between the
for
statement I added and theif __name__ == '__main__'
at the bottom was to indent everything by four spaces.