search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.47k stars 487 forks source link

py3: fix pip_installed_packages #27001

Closed fchapoton closed 5 years ago

fchapoton commented 5 years ago

by using pip3 when appropriate

CC: @embray @jdemeyer

Component: python3

Author: Frédéric Chapoton

Branch/Commit: b3722ac

Reviewer: Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/27001

fchapoton commented 5 years ago

New commits:

a06f005fix choice of pip in pip_installed_packages
fchapoton commented 5 years ago

Commit: a06f005

fchapoton commented 5 years ago

Branch: u/chapoton/27001

embray commented 5 years ago
comment:2

This should really be

diff --git a/src/sage/misc/package.py b/src/sage/misc/package.py
index 689e5a2..f7bfbc4 100644
--- a/src/sage/misc/package.py
+++ b/src/sage/misc/package.py
@@ -48,6 +48,7 @@ from sage.env import SAGE_ROOT, SAGE_PKGS
 import json
 import os
 import subprocess
+import sys
 try:
     # Python 3.3+
     from urllib.request import urlopen
@@ -142,9 +143,12 @@ def pip_installed_packages():
         sage: d['beautifulsoup']   # optional - beautifulsoup
         u'...'
     """
-    proc = subprocess.Popen(["pip", "list", "--no-index", "--format", "json"],
+    proc = subprocess.Popen([sys.executable, "-m", "pip", "list", "--no-index",
+                             "--format", "json"], stdout=subprocess.PIPE)
     stdout = proc.communicate()[0].decode()
-    return {package['name'].lower():package['version'] for package in json.load
+    return {package['name'].lower(): package['version']
+            for package in json.loads(stdout)}
+

 def list_packages(*pkg_types, **opts):
     r"""

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from a06f005 to d5040c0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

d5040c0fix choice of pip in pip_installed_packages
fchapoton commented 5 years ago
comment:4

done

embray commented 5 years ago

Reviewer: Erik Bray

embray commented 5 years ago
comment:5

Ok thanks!

embray commented 5 years ago
comment:6

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.

vbraun commented 5 years ago
comment:7
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
----------------------------------------------------------------------
fchapoton commented 5 years ago
comment:8

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

fchapoton commented 5 years ago
comment:9

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

embray commented 5 years ago
comment:10

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

fchapoton commented 5 years ago
comment:11

so what should we do here ?

jdemeyer commented 5 years ago
comment:12

Replying to @fchapoton:

so what should we do here ?

(edited because original answer was wrong)

It seems that the doctest framework is using pip_installed_packages which is now causing Python to be run from an artificially unsafe directory. In this case, it's fine to just change the doctest output.

jhpalmieri commented 5 years ago
comment:13

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: ...).

jhpalmieri commented 5 years ago

Changed branch from u/chapoton/27001 to u/jhpalmieri/27001

jhpalmieri commented 5 years ago
comment:15

I see two options: either

diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
index df938c8211..a5e2e6fbbf 100644
--- a/src/sage/tests/cmdline.py
+++ b/src/sage/tests/cmdline.py
@@ -409,12 +409,21 @@ def test_executable(args, input="", timeout=100.0, **kwds):
         sage: os.mkdir(d)
         sage: os.chmod(d, 0o777)
         sage: (out, err, ret) = test_executable(["sage", "-t", "nonexisting.py"], cwd=d)
-        sage: print(err)
+        sage: print(err) # py2
+        ...
+        Traceback (most recent call last):
+        ...
+        sage: print(err) # py3
         Traceback (most recent call last):
         ...
         RuntimeError: refusing to run doctests...
         sage: (out, err, ret) = test_executable(["sage", "-tp", "1", "nonexisting.py"], cwd=d)
-        sage: print(err)
+        sage: print(err) # py2
+        ...
+        Traceback (most recent call last):
+        ...
+        RuntimeError: refusing to run doctests...
+        sage: print(err) # py3
         Traceback (most recent call last):
         ...
         RuntimeError: refusing to run doctests...

or

diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
index df938c8211..08bda7c435 100644
--- a/src/sage/tests/cmdline.py
+++ b/src/sage/tests/cmdline.py
@@ -410,12 +410,10 @@ def test_executable(args, input="", timeout=100.0, **kwds):
         sage: os.chmod(d, 0o777)
         sage: (out, err, ret) = test_executable(["sage", "-t", "nonexisting.py"], cwd=d)
         sage: print(err)
-        Traceback (most recent call last):
         ...
         RuntimeError: refusing to run doctests...
         sage: (out, err, ret) = test_executable(["sage", "-tp", "1", "nonexisting.py"], cwd=d)
         sage: print(err)
-        Traceback (most recent call last):
         ...
         RuntimeError: refusing to run doctests...

Here is a branch using the second version.

New commits:

cfefd2efix choice of pip in pip_installed_packages
b3722actrac 27001: fix doctest
jhpalmieri commented 5 years ago

Changed commit from d5040c0 to b3722ac

fchapoton commented 5 years ago
comment:16

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

embray commented 5 years ago
comment:17

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?

jhpalmieri commented 5 years ago
comment:18

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

embray commented 5 years ago
comment:19

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

embray commented 5 years ago
comment:20

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

fchapoton commented 5 years ago
comment:21

I am slightly worried by comment:17 too.

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

jdemeyer commented 5 years ago
comment:22

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.

fchapoton commented 5 years ago
comment:23

ok, then let's go.

jdemeyer commented 5 years ago
comment:24

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.

jhpalmieri commented 5 years ago
comment:25

Replying to @fchapoton:

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.

vbraun commented 5 years ago

Changed branch from u/jhpalmieri/27001 to b3722ac