Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression using backslash in requirements file with version 7.x #2966

Closed
rodcloutier opened this issue Jul 8, 2015 · 4 comments
Closed
Labels
auto-locked Outdated issues that have been locked by automation

Comments

@rodcloutier
Copy link

The pull request #2697 introduced a regression in the way local file path in requirements are parsed.
Requirements, in requirement files, using backslash are not supported anymore on Windows

Context of requirements.txt

-e test\module
$ pip --version
$ pip 6.1.1 from F:\bin\Python\278\lib\site-packages (python 2.7)
$ pip install -e  test\module
You are using pip version 6.1.1, however version 7.1.0 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
Obtaining file:///R:/dev/experiments/pip/test/module
Installing collected packages: funniest
  Running setup.py develop for funniest
Successfully installed funniest-0.1
$ pip install -r requirements.txt
You are using pip version 6.1.1, however version 7.1.0 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
Obtaining file:///R:/dev/experiments/pip/test/module (from -r requirements.txt (line 1))
Installing collected packages: funniest
  Running setup.py develop for funniest
Successfully installed funniest-0.1

Now running version 7

$ pip --version
pip 7.0.0 from F:\bin\Python\278\lib\site-packages (python 2.7)
$ pip install -e test\module
You are using pip version 7.0.0, however version 7.1.0 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
Obtaining file:///R:/dev/experiments/pip/test/module
Installing collected packages: funniest
  Running setup.py develop for funniest
Successfully installed funniest-0.1
$ pip install -r requirements.txt
You are using pip version 7.0.0, however version 7.1.0 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
testmodule should either be a path to a local project or a VCS url beginning with svn+, git+, hg+, or bzr+

Git bisect gave me the following commit, from pull request #2697 as the cause,

764e468f429f5fdfdd65a103f1337910cca7fa3a is the first bad commit
commit 764e468f429f5fdfdd65a103f1337910cca7fa3a
Author: Marcus Smith <qwcode@gmail.com>
Date:   Thu Apr 16 22:10:46 2015 -0700

    refactor the requirements file parsing

    two major changes:

    1) re-use the optparse options in pip.cmdoptions instead of maintaining
       a custom parser

    2) as a result of #1, simplify the call stack
        from:  parse_requirements -> parse_content -> parse_line
          to:  parse_requirements -> process_line

    beyond #1/#2, minor cosmetics and adjusting the tests to match

:040000 040000 7e92207579adb8d66adf399a148ef662b4b46c9d 3e4f06cdc329fdbcd788dcf9a1a0b7a53051dc17 M      pip
:040000 040000 ceb2f89267779753381422ef24bc1897f861fe7f 2f0bab0e53f918e85e016d37b057e0288c1820b8 M      tests

I wrote these tests that expose the error (by the way, can we do this in a little cleaner way, with temp files?)

pip/tests/unit/test_req_file.py

class TestProcessLine(object):
    ...
    def test_requirement_with_backslash(self):
        url = 'local\\path'

        if os.path.exists(url):
            import shutil
            shutil.rmtree(url)
        os.makedirs(url)
        open(os.path.join(url, 'setup.py'), 'w')

        line = '-e %s' % url
        filename = 'filename'
        result = process_line(line, filename, 1, options=self.options)

        assert list(result)[0].link.url == 'file:///' + os.path.join(os.getcwd(),url).replace('\\', '/')

    def test_requirement_with_slash(self):
        url = 'local/path'

        if os.path.exists(url):
            import shutil
            shutil.rmtree(url)
        os.makedirs(url)
        open(os.path.join(url, 'setup.py'), 'w')

        line = '-e %s' % url
        filename = 'filename'
        result = process_line(line, filename, 1, options=self.options)
        assert list(result)[0].link.url == 'file:///' + os.path.join(os.getcwd(),url).replace('\\', '/')

Result of test run

$ py.test tests\unit\test_req_file.py
============================= test session starts =============================
platform win32 -- Python 2.7.8 -- py-1.4.30 -- pytest-2.7.2
rootdir: R:\dev\experiments\pip, inifile: setup.cfg
collected 22 items

tests\unit\test_req_file.py .............F........

================================== FAILURES ===================================
_______________ TestProcessLine.test_requirement_with_backslash _______________

self = <tests.unit.test_req_file.TestProcessLine object at 0x031DEA70>

    def test_requirement_with_backslash(self):
        url = 'local\\path'

        if os.path.exists(url):
            import shutil
            shutil.rmtree(url)
        os.makedirs(url)
        open(os.path.join(url, 'setup.py'), 'w')

        line = '-e %s' % url
        filename = 'filename'
        result = process_line(line, filename, 1, options=self.options)

>       assert list(result)[0].link.url == 'file:///' + os.path.join(os.getcwd(),url).replace('\\', '/')

tests\unit\test_req_file.py:150:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
pip\req\req_file.py:134: in process_line
    default_vcs=default_vcs, isolated=isolated
pip\req\req_install.py:127: in from_editable
    editable_req, default_vcs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

editable_req = 'localpath', default_vcs = None

    def parse_editable(editable_req, default_vcs=None):
        """Parses an editable requirement into:
            - a requirement name
            - an URL
            - extras
            - editable options
        Accepted requirements:
            svn+http://blahblah@rev#egg=Foobar[baz]&subdirectory=version_subdir
            .[some_extra]
        """

        url = editable_req
        extras = None

        # If a file path is specified with extras, strip off the extras.
        m = re.match(r'^(.+)(\[[^\]]+\])$', url)
        if m:
            url_no_extras = m.group(1)
            extras = m.group(2)
        else:
            url_no_extras = url

        if os.path.isdir(url_no_extras):
            if not os.path.exists(os.path.join(url_no_extras, 'setup.py')):
                raise InstallationError(
                    "Directory %r is not installable. File 'setup.py' not found." %
                    url_no_extras
                )
            # Treating it as code that has already been checked out
            url_no_extras = path_to_url(url_no_extras)

        if url_no_extras.lower().startswith('file:'):
            if extras:
                return (
                    None,
                    url_no_extras,
                    pkg_resources.Requirement.parse(
                        '__placeholder__' + extras
                    ).extras,
                    {},
                )
            else:
                return None, url_no_extras, None, {}

        for version_control in vcs:
            if url.lower().startswith('%s:' % version_control):
                url = '%s+%s' % (version_control, url)
                break

        if '+' not in url:
            if default_vcs:
                url = default_vcs + '+' + url
            else:
                raise InstallationError(
                    '%s should either be a path to a local project or a VCS url '
                    'beginning with svn+, git+, hg+, or bzr+' %
>                   editable_req
                )
E               InstallationError: localpath should either be a path to a local project or a VCS url beginning with svn+, git+, hg+, or bzr+

pip\req\req_install.py:1146: InstallationError
===================== 1 failed, 21 passed in 1.15 seconds =====================

The line that causes the error is the last line of the following code.

def process_line(line, filename, line_number, finder=None, comes_from=None,
                 options=None, session=None, cache_root=None):
    """
    Process a single requirements line; This can result in creating/yielding
    requirements, or updating the finder.
    """

    parser = build_parser()
    args = shlex.split(line)

shlex assumes a posix parsing which will remove the backslash. I tried naively to use the posix argument as such

args = shlex.split(line, posix=os.name != 'nt')

but this makes other tests fail

============================= test session starts =============================
platform win32 -- Python 2.7.8 -- py-1.4.30 -- pytest-2.7.2
rootdir: R:\dev\experiments\pip, inifile: setup.cfg
collected 22 items

tests\unit\test_req_file.py ..........F..........F

================================== FAILURES ===================================
_____________ TestProcessLine.test_options_on_a_requirement_line ______________

self = <tests.unit.test_req_file.TestProcessLine object at 0x03115550>

    def test_options_on_a_requirement_line(self):
        line = 'SomeProject --install-option=yo1 --install-option yo2 '\
               '--global-option="yo3" --global-option "yo4"'
        filename = 'filename'
        req = list(process_line(line, filename, 1))[0]
>       assert req.options == {
            'global_options': ['yo3', 'yo4'],
            'install_options': ['yo1', 'yo2']}
E       assert {'global_opti...'yo1', 'yo2']} == {'global_optio...'yo1', 'yo2']}
E         Omitting 1 identical items, use -v to show
E         Differing items:
E         {'global_options': ['"yo3"', '"yo4"']} != {'global_options': ['yo3', 'yo4']}
E         Use -v to get the full diff

tests\unit\test_req_file.py:117: AssertionError
________ TestParseRequirements.test_install_requirements_with_options _________

self = <tests.unit.test_req_file.TestParseRequirements object at 0x03115610>
tmpdir = Path(u'f:\\bin\\msys64\\tmp\\pytest-13\\test_install_requirements_with_options0')
finder = <pip.index.PackageFinder object at 0x03141070>
session = <pip.download.PipSession object at 0x03115F10>

    def test_install_requirements_with_options(self, tmpdir, finder, session):
        global_option = '--dry-run'
        install_option = '--prefix=/opt'

        content = '''
            INITools==2.0 --global-option="{global_option}" \
                            --install-option "{install_option}"
            '''.format(global_option=global_option, install_option=install_option)

        req_path = tmpdir.join('requirements.txt')
        with open(req_path, 'w') as fh:
            fh.write(content)

        req = next(parse_requirements(req_path, finder=finder,
                                      session=session))

        req.source_dir = os.curdir
        with patch.object(subprocess, 'Popen') as popen:
            try:
                req.install([])
            except:
                pass

            call = popen.call_args_list[0][0][0]
>           assert call.index(install_option) > \
                call.index('install') > \
                call.index(global_option) > 0
E           ValueError: '--prefix=/opt' is not in list

tests\unit\test_req_file.py:294: ValueError
===================== 2 failed, 20 passed in 1.16 seconds =====================

Any tips on how, where to solve this?

@qwcode
Copy link
Contributor

qwcode commented Jul 10, 2015

this should be fixed in v7.02, so try upgrading and retry.
closing for now. reopen if you still see an issue.

@qwcode qwcode closed this as completed Jul 10, 2015
@rodcloutier
Copy link
Author

Reopening since I have the same behavior with 7.1

$ pip --version
pip 7.1.0 from F:\bin\Python\278\lib\site-packages (python 2.7)
$ pip install -e test\module
Obtaining file:///R:/dev/experiments/pip/test/module
Installing collected packages: funniest
  Running setup.py develop for funniest
Successfully installed funniest-0.1
$ pip install -r requirements.txt
testmodule should either be a path to a local project or a VCS url beginning with svn+, git+, hg+,

@qwcode
Copy link
Contributor

qwcode commented Jul 10, 2015

ok, I can look further, but the line you reference as the problem doesn't exist in >=7.02

@qwcode
Copy link
Contributor

qwcode commented Jul 12, 2015

ok, I looked at this some more.

yes, this is a regression
(but you are aware that test\\module and test/module will work?)

as you saw, posix=False doesn't work well.

one idea that seemed to worked was to alter the escape list to '' (i.e. not '\'), when on windows (but I'm not sure of the implications)

like this:

> s = shlex.shlex('-e test/module', posix=True)
> s.whitespace_split = True
> s.escape = ''
> list(s)
['-e', 'test\\module']

@pfmoore ?, thoughts?

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

No branches or pull requests

2 participants