Opened 11 years ago

Closed 11 years ago

#12018 closed defect (fixed)

sage-list-packages will fail if user can't write to SAGE_ROOT

Reported by: John Palmieri Owned by: Leif Leonhardy
Priority: minor Milestone: sage-5.0
Component: scripts Keywords:
Cc: Merged in: sage-5.0.beta10
Authors: R. Andrew Ohana Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by John Palmieri)

As the summary says. The script sage-list-packages writes a temporary file to

file = "%s/tmp/list"%SAGE_ROOT

This should use a temporary file instead.


Apply trac12018-notempfile.patch to the scripts repo.

Attachments (2)

trac_12018-tempfile.patch (1.2 KB) - added by John Palmieri 11 years ago.
trac12018-notempfile.patch (3.1 KB) - added by R. Andrew Ohana 11 years ago.
patch that avoids the use of a tempfile (and uses os.path instead of string arithmetic)

Download all attachments as: .zip

Change History (14)

Changed 11 years ago by John Palmieri

Attachment: trac_12018-tempfile.patch added

comment:1 Changed 11 years ago by John Palmieri

Description: modified (diff)
Status: newneeds_review

comment:2 Changed 11 years ago by John Palmieri

Summary: sage-location will fail if user can't write to SAGE_ROOTsage-list-packages will fail if user can't write to SAGE_ROOT

I should point out that sage-list-packages is used by the commands "sage --standard", "sage --optional", and "sage --experimental".

comment:3 Changed 11 years ago by R. Andrew Ohana

Description: modified (diff)

I've added a patch that avoids the use of a tempfile and cleans up the script (os.path should be used instead of string arithmetic).

comment:4 Changed 11 years ago by John Palmieri

In your patch, I appreciate the use of os.path.join for local paths, but is this the right thing to do for urls? If the local system has some funny path separator, the url may be misformed.

comment:5 in reply to:  4 Changed 11 years ago by R. Andrew Ohana

Replying to jhpalmieri:

In your patch, I appreciate the use of os.path.join for local paths, but is this the right thing to do for urls? If the local system has some funny path separator, the url may be misformed.

Good point, it seems like the urlparse library is what I should have used. I'll update my patch.

comment:6 Changed 11 years ago by R. Andrew Ohana

ok, patch updated

comment:7 Changed 11 years ago by John Palmieri

I'm happy to use your approach and not to create a file. Some more comments: I think using urlparse.urljoin will clean some things up:

  • sage-list-packages

    diff --git a/sage-list-packages b/sage-list-packages
    a b if not os.environ.has_key("SAGE_SERVER") 
    1111     print "The environment variable SAGE_SERVER must be set"
    1212     sys.exit(1)
    1313
    14 url_list = list(urlparse.urlsplit(os.environ['SAGE_SERVER']))
    15 url_path = os.path.join(url_list[2], 'packages')
    16 url_list[2] = urllib.pathname2url(url_path)
    17 
    18 PKG_SERVER = urlparse.urlunsplit(url_list)
     14PKG_SERVER = urlparse.urljoin(os.environ['SAGE_SERVER'], 'packages')
    1915print "Using SAGE Server %s"%PKG_SERVER
    2016
    21 url_path = os.path.join(url_path, sys.argv[1], 'list')
    22 url_list[2] = urllib.pathname2url(url_path)
    23 url = urlparse.urlunsplit(url_list)
     17url_path = os.path.join('packages', sys.argv[1], 'list')
     18url = urlparse.urljoin(PKG_SERVER, urllib.pathname2url(url_path))
    2419
    2520try:
    2621     installed = set(os.listdir(os.path.join(SPKG_ROOT, 'installed')))

Also, according to the Python documentation, urllib.urlopen is deprecated in favor of urllib2.urlopen. Maybe we should use that instead?

comment:8 Changed 11 years ago by John Palmieri

Any updates on this?

comment:9 Changed 11 years ago by R. Andrew Ohana

I think we should do what you suggest, sorry, got caught up in other stuff.

Changed 11 years ago by R. Andrew Ohana

Attachment: trac12018-notempfile.patch added

patch that avoids the use of a tempfile (and uses os.path instead of string arithmetic)

comment:10 Changed 11 years ago by R. Andrew Ohana

Authors: John PalmieriJohn Palmieri, R. Andrew Ohana

ok, updated

comment:11 Changed 11 years ago by John Palmieri

Authors: John Palmieri, R. Andrew OhanaR. Andrew Ohana
Description: modified (diff)
Reviewers: John Palmieri
Status: needs_reviewpositive_review

Looks good to me.

comment:12 Changed 11 years ago by Jeroen Demeyer

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