Opened 2 years ago
Closed 2 years ago
#27001 closed defect (fixed)
py3: fix pip_installed_packages
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-8.7 |
Component: | python3 | Keywords: | |
Cc: | embray, jdemeyer | 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: |
Description
by using pip3 when appropriate
Change History (26)
comment:1 Changed 2 years ago by
- Branch set to u/chapoton/27001
- Commit set to a06f005ecdf70d790b104c314aaee5cf8427a216
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
- Status changed from needs_review to needs_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 48 48 import json 49 49 import os 50 50 import subprocess 51 import sys 51 52 try: 52 53 # Python 3.3+ 53 54 from urllib.request import urlopen … … def pip_installed_packages(): 142 143 sage: d['beautifulsoup'] # optional - beautifulsoup 143 144 u'...' 144 145 """ 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) 146 148 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 148 152 149 153 def list_packages(*pkg_types, **opts): 150 154 r"""
Apparently I already had this fix in my python3 branch: I apologize for forgetting to make a ticket for it.
comment:3 Changed 2 years ago by
- Commit changed from a06f005ecdf70d790b104c314aaee5cf8427a216 to d5040c0b4ad10323909d0642143d3a16bea41b54
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d5040c0 | fix choice of pip in pip_installed_packages
|
comment:5 Changed 2 years ago by
- Priority changed from major to minor
- Reviewers set to Erik Bray
- Status changed from needs_review to positive_review
- Type changed from enhancement to defect
Ok thanks!
comment:6 Changed 2 years ago by
- Milestone changed from sage-8.6 to sage-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 2 years ago by
- Status changed from positive_review to needs_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 2 years ago by
Erik or Jeroen, any idea on how to fix that ?
comment:9 Changed 2 years ago by
should I just add ...
before the actual doctest ?
comment:10 Changed 2 years ago by
#26457 has discussion about rewriting, if not entirely removing that test.
comment:11 follow-up: ↓ 12 Changed 2 years ago by
so what should we do here ?
comment:12 in reply to: ↑ 11 Changed 2 years ago by
Replying to chapoton:
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.
comment:13 follow-up: ↓ 24 Changed 2 years ago by
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 2 years ago by
- Branch changed from u/chapoton/27001 to u/jhpalmieri/27001
comment:15 Changed 2 years ago by
- Commit changed from d5040c0b4ad10323909d0642143d3a16bea41b54 to b3722ac58a78ff4ab30111da4d6a895c18b63211
- Status changed from needs_work to needs_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): 409 409 sage: os.mkdir(d) 410 410 sage: os.chmod(d, 0o777) 411 411 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 413 417 Traceback (most recent call last): 414 418 ... 415 419 RuntimeError: refusing to run doctests... 416 420 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 418 427 Traceback (most recent call last): 419 428 ... 420 429 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): 410 410 sage: os.chmod(d, 0o777) 411 411 sage: (out, err, ret) = test_executable(["sage", "-t", "nonexisting.py"], cwd=d) 412 412 sage: print(err) 413 Traceback (most recent call last):414 413 ... 415 414 RuntimeError: refusing to run doctests... 416 415 sage: (out, err, ret) = test_executable(["sage", "-tp", "1", "nonexisting.py"], cwd=d) 417 416 sage: print(err) 418 Traceback (most recent call last):419 417 ... 420 418 RuntimeError: refusing to run doctests... 421 419
Here is a branch using the second version.
New commits:
cfefd2e | fix choice of pip in pip_installed_packages
|
b3722ac | trac 27001: fix doctest
|
comment:16 Changed 2 years ago by
ok, looks good to me. Jeroen, do you approve ?
comment:17 follow-up: ↓ 22 Changed 2 years ago by
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 2 years ago by
It passes tests for me with Python 2 and Python 3, so I guess we fixed it.
comment:19 Changed 2 years ago by
- Status changed from needs_review to positive_review
I don't remember that, but I guess so!
comment:20 Changed 2 years ago by
(I'm not Jeroen, obviously, but I think this seems to reflect his suggestion)
comment:21 follow-up: ↓ 25 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
ok, then let's go.
comment:24 in reply to: ↑ 13 Changed 2 years ago by
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 linesys: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 2 years ago by
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 2 years ago by
- Branch changed from u/jhpalmieri/27001 to b3722ac58a78ff4ab30111da4d6a895c18b63211
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
fix choice of pip in pip_installed_packages