Opened 7 months ago

Closed 5 months 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) Commit: b3722ac58a78ff4ab30111da4d6a895c18b63211
Dependencies: Stopgaps:

Description

by using pip3 when appropriate

Change History (26)

comment:1 Changed 7 months ago by chapoton

  • Branch set to u/chapoton/27001
  • Commit set to a06f005ecdf70d790b104c314aaee5cf8427a216
  • Status changed from new to needs_review

New commits:

a06f005fix choice of pip in pip_installed_packages

comment:2 Changed 7 months ago by embray

  • 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 
    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 7 months ago by git

  • Commit changed from a06f005ecdf70d790b104c314aaee5cf8427a216 to d5040c0b4ad10323909d0642143d3a16bea41b54

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 7 months ago by chapoton

  • Status changed from needs_work to needs_review

done

comment:5 Changed 7 months ago by embray

  • 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 6 months ago by embray

  • 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 6 months ago by vbraun

  • 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 6 months ago by chapoton

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

comment:9 Changed 6 months ago by chapoton

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

comment:10 Changed 6 months ago by embray

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

comment:11 follow-up: Changed 5 months ago by chapoton

so what should we do here ?

comment:12 in reply to: ↑ 11 Changed 5 months ago by jdemeyer

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.

Last edited 5 months ago by jdemeyer (previous) (diff)

comment:13 follow-up: Changed 5 months ago by 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: ...).

comment:14 Changed 5 months ago by jhpalmieri

  • Branch changed from u/chapoton/27001 to u/jhpalmieri/27001

comment:15 Changed 5 months ago by jhpalmieri

  • 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): 
    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 5 months ago by chapoton

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

comment:17 follow-up: Changed 5 months ago by 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?

comment:18 Changed 5 months ago by jhpalmieri

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

comment:19 Changed 5 months ago by embray

  • Status changed from needs_review to positive_review

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

comment:20 Changed 5 months ago by embray

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

comment:21 follow-up: Changed 5 months ago by 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 5 months ago by jdemeyer

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 5 months ago by chapoton

ok, then let's go.

comment:24 in reply to: ↑ 13 Changed 5 months ago by jdemeyer

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 5 months ago by jhpalmieri

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 5 months ago by vbraun

  • Branch changed from u/jhpalmieri/27001 to b3722ac58a78ff4ab30111da4d6a895c18b63211
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.