Opened 8 years ago

Closed 8 years ago

#12018 closed defect (fixed)

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

Reported by: jhpalmieri Owned by: leif
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:

Description (last modified by jhpalmieri)

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 jhpalmieri 8 years ago.
trac12018-notempfile.patch (3.1 KB) - added by ohanar 8 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 8 years ago by jhpalmieri

comment:1 Changed 8 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by jhpalmieri

  • Summary changed from sage-location will fail if user can't write to SAGE_ROOT to sage-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 8 years ago by ohanar

  • 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 follow-up: Changed 8 years ago by 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.

comment:5 in reply to: ↑ 4 Changed 8 years ago by ohanar

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 8 years ago by ohanar

ok, patch updated

comment:7 Changed 8 years ago by jhpalmieri

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 8 years ago by jhpalmieri

Any updates on this?

comment:9 Changed 8 years ago by ohanar

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

Changed 8 years ago by ohanar

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

comment:10 Changed 8 years ago by ohanar

  • Authors changed from John Palmieri to John Palmieri, R. Andrew Ohana

ok, updated

comment:11 Changed 8 years ago by jhpalmieri

  • Authors changed from John Palmieri, R. Andrew Ohana to R. Andrew Ohana
  • Description modified (diff)
  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

Looks good to me.

comment:12 Changed 8 years ago by jdemeyer

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