Opened 4 years ago

Closed 4 years ago

#27001 closed defect (fixed)

py3: fix pip_installed_packages

Reported by: Frédéric Chapoton Owned by:
Priority: minor Milestone: sage-8.7
Component: python3 Keywords:
Cc: Erik Bray, Jeroen Demeyer Merged in:
Authors: Frédéric Chapoton Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: b3722ac (Commits, GitHub, GitLab) Commit: b3722ac58a78ff4ab30111da4d6a895c18b63211
Dependencies: Stopgaps:

Status badges

Description

by using pip3 when appropriate

Change History (26)

comment:1 Changed 4 years ago by Frédéric Chapoton

Branch: u/chapoton/27001
Commit: a06f005ecdf70d790b104c314aaee5cf8427a216
Status: newneeds_review

New commits:

a06f005fix choice of pip in pip_installed_packages

comment:2 Changed 4 years ago by Erik Bray

Status: needs_reviewneeds_work

This should really be

  • src/sage/misc/package.py

    diff --git a/src/sage/misc/package.py b/src/sage/misc/package.py
    index 689e5a2..f7bfbc4 100644
    a b from sage.env import SAGE_ROOT, SAGE_PKGS 
    4848import json
    4949import os
    5050import subprocess
     51import sys
    5152try:
    5253    # Python 3.3+
    5354    from urllib.request import urlopen
    def pip_installed_packages(): 
    142143        sage: d['beautifulsoup']   # optional - beautifulsoup
    143144        u'...'
    144145    """
    145     proc = subprocess.Popen(["pip", "list", "--no-index", "--format", "json"],
     146    proc = subprocess.Popen([sys.executable, "-m", "pip", "list", "--no-index",
     147                             "--format", "json"], stdout=subprocess.PIPE)
    146148    stdout = proc.communicate()[0].decode()
    147     return {package['name'].lower():package['version'] for package in json.load
     149    return {package['name'].lower(): package['version']
     150            for package in json.loads(stdout)}
     151
    148152
    149153def list_packages(*pkg_types, **opts):
    150154    r"""

Apparently I already had this fix in my python3 branch: I apologize for forgetting to make a ticket for it.

comment:3 Changed 4 years ago by git

Commit: a06f005ecdf70d790b104c314aaee5cf8427a216d5040c0b4ad10323909d0642143d3a16bea41b54

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d5040c0fix choice of pip in pip_installed_packages

comment:4 Changed 4 years ago by Frédéric Chapoton

Status: needs_workneeds_review

done

comment:5 Changed 4 years ago by Erik Bray

Priority: majorminor
Reviewers: Erik Bray
Status: needs_reviewpositive_review
Type: enhancementdefect

Ok thanks!

comment:6 Changed 4 years ago by Erik Bray

Milestone: sage-8.6sage-8.7

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

comment:7 Changed 4 years ago by Volker Braun

Status: positive_reviewneeds_work
sage -t --long src/sage/tests/cmdline.py
**********************************************************************
File "src/sage/tests/cmdline.py", line 412, in sage.tests.cmdline.test_executable
Failed example:
    print(err)
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: refusing to run doctests...
Got:
    sys:1: RuntimeWarning: not adding directory '' to sys.path since everybody can write to it.
    Untrusted users could put files in this directory which might then be imported by your Python code. As a general precaution from similar exploits, you should not execute Python code from this directory
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage_git/build/src/bin/sage-runtests", line 163, in <module>
        err = DC.run()
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/control.py", line 1200, in run
        self.test_safe_directory()
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/control.py", line 643, in test_safe_directory
        .format(os.getcwd()))
    RuntimeError: refusing to run doctests from the current directory '/var/lib/buildbot/.sage/temp/kucalc/160488/dir_OqlYHZ/test' since untrusted users could put files in this directory, making it unsafe to run Sage code from
    <BLANKLINE>
**********************************************************************
File "src/sage/tests/cmdline.py", line 417, in sage.tests.cmdline.test_executable
Failed example:
    print(err)
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: refusing to run doctests...
Got:
    sys:1: RuntimeWarning: not adding directory '' to sys.path since everybody can write to it.
    Untrusted users could put files in this directory which might then be imported by your Python code. As a general precaution from similar exploits, you should not execute Python code from this directory
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage_git/build/src/bin/sage-runtests", line 163, in <module>
        err = DC.run()
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/control.py", line 1200, in run
        self.test_safe_directory()
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/control.py", line 643, in test_safe_directory
        .format(os.getcwd()))
    RuntimeError: refusing to run doctests from the current directory '/var/lib/buildbot/.sage/temp/kucalc/160488/dir_OqlYHZ/test' since untrusted users could put files in this directory, making it unsafe to run Sage code from
    <BLANKLINE>
**********************************************************************
1 item had failures:
   2 of 251 in sage.tests.cmdline.test_executable
    [250 tests, 2 failures, 41.92 s]
----------------------------------------------------------------------
sage -t --long src/sage/tests/cmdline.py  # 2 doctests failed
----------------------------------------------------------------------

comment:8 Changed 4 years ago by Frédéric Chapoton

Erik or Jeroen, any idea on how to fix that ?

comment:9 Changed 4 years ago by Frédéric Chapoton

should I just add ... before the actual doctest ?

comment:10 Changed 4 years ago by Erik Bray

#26457 has discussion about rewriting, if not entirely removing that test.

comment:11 Changed 4 years ago by Frédéric Chapoton

so what should we do here ?

comment:12 in reply to:  11 Changed 4 years ago by Jeroen Demeyer

Replying to chapoton:

so what should we do here ?

The warning says that what you're doing is probably insecure, so use that advice and don't run that test in an unsafe way!

Version 0, edited 4 years ago by Jeroen Demeyer (next)

comment:13 Changed 4 years ago by John Palmieri

If I add ... at the beginning of the doctest output, then it passes with Python 2 but not Python 3 (which doesn't print the extra line sys:1: RuntimeWarning: ...).

comment:14 Changed 4 years ago by John Palmieri

Branch: u/chapoton/27001u/jhpalmieri/27001

comment:15 Changed 4 years ago by John Palmieri

Commit: d5040c0b4ad10323909d0642143d3a16bea41b54b3722ac58a78ff4ab30111da4d6a895c18b63211
Status: needs_workneeds_review

I see two options: either

  • src/sage/tests/cmdline.py

    diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
    index df938c8211..a5e2e6fbbf 100644
    a b def test_executable(args, input="", timeout=100.0, **kwds): 
    409409        sage: os.mkdir(d)
    410410        sage: os.chmod(d, 0o777)
    411411        sage: (out, err, ret) = test_executable(["sage", "-t", "nonexisting.py"], cwd=d)
    412         sage: print(err)
     412        sage: print(err) # py2
     413        ...
     414        Traceback (most recent call last):
     415        ...
     416        sage: print(err) # py3
    413417        Traceback (most recent call last):
    414418        ...
    415419        RuntimeError: refusing to run doctests...
    416420        sage: (out, err, ret) = test_executable(["sage", "-tp", "1", "nonexisting.py"], cwd=d)
    417         sage: print(err)
     421        sage: print(err) # py2
     422        ...
     423        Traceback (most recent call last):
     424        ...
     425        RuntimeError: refusing to run doctests...
     426        sage: print(err) # py3
    418427        Traceback (most recent call last):
    419428        ...
    420429        RuntimeError: refusing to run doctests...

or

  • src/sage/tests/cmdline.py

    diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
    index df938c8211..08bda7c435 100644
    a b def test_executable(args, input="", timeout=100.0, **kwds): 
    410410        sage: os.chmod(d, 0o777)
    411411        sage: (out, err, ret) = test_executable(["sage", "-t", "nonexisting.py"], cwd=d)
    412412        sage: print(err)
    413         Traceback (most recent call last):
    414413        ...
    415414        RuntimeError: refusing to run doctests...
    416415        sage: (out, err, ret) = test_executable(["sage", "-tp", "1", "nonexisting.py"], cwd=d)
    417416        sage: print(err)
    418         Traceback (most recent call last):
    419417        ...
    420418        RuntimeError: refusing to run doctests...
    421419

Here is a branch using the second version.


New commits:

cfefd2efix choice of pip in pip_installed_packages
b3722actrac 27001: fix doctest

comment:16 Changed 4 years ago by Frédéric Chapoton

ok, looks good to me. Jeroen, do you approve ?

comment:17 Changed 4 years ago by Erik Bray

I thought that the doctest parser got confused if we started a line with ... (it would get confused with a continuation prompt like ....:). Or did we fix that?

comment:18 Changed 4 years ago by John Palmieri

It passes tests for me with Python 2 and Python 3, so I guess we fixed it.

comment:19 Changed 4 years ago by Erik Bray

Status: needs_reviewpositive_review

I don't remember that, but I guess so!

comment:20 Changed 4 years ago by Erik Bray

(I'm not Jeroen, obviously, but I think this seems to reflect his suggestion)

comment:21 Changed 4 years ago by Frédéric Chapoton

I am slightly worried by comment:17 too.

Maybe we should make sure that this is fixed and the doctests are run ?

comment:22 in reply to:  17 Changed 4 years ago by Jeroen Demeyer

Replying to embray:

I thought that the doctest parser got confused if we started a line with ... (it would get confused with a continuation prompt like ....:). Or did we fix that?

I somehow seem to recall that this was fixed and that now a ....: continuation is required after a sage: prompt.

comment:23 Changed 4 years ago by Frédéric Chapoton

ok, then let's go.

comment:24 in reply to:  13 Changed 4 years ago by Jeroen Demeyer

Replying to jhpalmieri:

If I add ... at the beginning of the doctest output, then it passes with Python 2 but not Python 3 (which doesn't print the extra line sys:1: RuntimeWarning: ...).

I just posted a new topic on sage-devel What to do with the sys.path security patch? referring this.

comment:25 in reply to:  21 Changed 4 years ago by John Palmieri

Replying to chapoton:

I am slightly worried by comment:17 too.

Maybe we should make sure that this is fixed and the doctests are run ?

I will confirm that the tests are run: they fail if I change the text.

comment:26 Changed 4 years ago by Volker Braun

Branch: u/jhpalmieri/27001b3722ac58a78ff4ab30111da4d6a895c18b63211
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.