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

Bugfix/3681 #3704

Merged
merged 38 commits into from May 26, 2016

Conversation

Projects
None yet
3 participants
@AndydeCleyre
Contributor

AndydeCleyre commented May 20, 2016

See #3681

In case of failure to parse as a valid requirement, warn and move along to the next one.

@RonnyPfannschmidt

This comment has been minimized.

Show comment
Hide comment
@RonnyPfannschmidt

RonnyPfannschmidt May 20, 2016

Contributor

the implementation looks good, can you try to also add an acceptance test that shows its doing whats expected

Contributor

RonnyPfannschmidt commented May 20, 2016

the implementation looks good, can you try to also add an acceptance test that shows its doing whats expected

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft May 20, 2016

Member

Agreed, this look good. A test would be great though so we can make sure that we don't run into this again. Is that something you're able to do?

Member

dstufft commented May 20, 2016

Agreed, this look good. A test would be great though so we can make sure that we don't run into this again. Is that something you're able to do?

@AndydeCleyre

This comment has been minimized.

Show comment
Hide comment
@AndydeCleyre

AndydeCleyre May 20, 2016

Contributor

I was unable to run the existing freeze tests with 3.5.1, 3.4.4., 3.3.6, and 2.6.9, similar to the issue described at #2574, especially:

RuntimeError: Unknown test type . . . test_basic.py

and

. . ./_pytest/assertion/rewrite.py:700: in visit_Name
    locs = ast.Call(self.builtin("locals"), [], [], None, None)
E   TypeError: Call constructor takes either 0 or 3 positional arguments

I was using py.test -k 'test_freeze'. Please let me know if I'm missing something here; as it is I cannot successfully run the tests.

I tried to write one anyway, and am adding it to this pull request, but it will probably need some adjustment once I can actually run it and see what happens. Alternatively, someone can just pick things up from here.

Is it correct to assume that within the context of running this test, no other packages will be polluting the freeze output?

Contributor

AndydeCleyre commented May 20, 2016

I was unable to run the existing freeze tests with 3.5.1, 3.4.4., 3.3.6, and 2.6.9, similar to the issue described at #2574, especially:

RuntimeError: Unknown test type . . . test_basic.py

and

. . ./_pytest/assertion/rewrite.py:700: in visit_Name
    locs = ast.Call(self.builtin("locals"), [], [], None, None)
E   TypeError: Call constructor takes either 0 or 3 positional arguments

I was using py.test -k 'test_freeze'. Please let me know if I'm missing something here; as it is I cannot successfully run the tests.

I tried to write one anyway, and am adding it to this pull request, but it will probably need some adjustment once I can actually run it and see what happens. Alternatively, someone can just pick things up from here.

Is it correct to assume that within the context of running this test, no other packages will be polluting the freeze output?

@AndydeCleyre

This comment has been minimized.

Show comment
Hide comment
@AndydeCleyre

AndydeCleyre May 20, 2016

Contributor

Looking at the travis results coming in, it's clear that this is not complete yet.

At the very least, pip is failing when I try to install an invalid package, which I figured would be a prerequisite to testing freeze. Maybe someone more familiar with the setup here knows a way to get past that?

Contributor

AndydeCleyre commented May 20, 2016

Looking at the travis results coming in, it's clear that this is not complete yet.

At the very least, pip is failing when I try to install an invalid package, which I figured would be a prerequisite to testing freeze. Maybe someone more familiar with the setup here knows a way to get past that?

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft May 20, 2016

Member

@AndydeCleyre You'll probably need to fake install something by manually writing a .egg-info directory out.

Member

dstufft commented May 20, 2016

@AndydeCleyre You'll probably need to fake install something by manually writing a .egg-info directory out.

@AndydeCleyre

This comment has been minimized.

Show comment
Hide comment
@AndydeCleyre

AndydeCleyre May 20, 2016

Contributor

I created some dummy egg-info files on my system to test with, and see that even if we faked the installs, and this test ran completely, it would fail.

For one thing, names starting with digits are parsed without complaint by pip, though they are said to be invalid in PEP 426.

Should I remove that case from the test, or should we make pip name parsing more strict at the same time?

Contributor

AndydeCleyre commented May 20, 2016

I created some dummy egg-info files on my system to test with, and see that even if we faked the installs, and this test ran completely, it would fail.

For one thing, names starting with digits are parsed without complaint by pip, though they are said to be invalid in PEP 426.

Should I remove that case from the test, or should we make pip name parsing more strict at the same time?

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft May 20, 2016

Member

PEP 426 allows digits:

Distribution names MUST start and end with an ASCII letter or digit.

Member

dstufft commented May 20, 2016

PEP 426 allows digits:

Distribution names MUST start and end with an ASCII letter or digit.

@AndydeCleyre

This comment has been minimized.

Show comment
Hide comment
@AndydeCleyre

AndydeCleyre May 20, 2016

Contributor

Ah, thanks, will correct.

Contributor

AndydeCleyre commented May 20, 2016

Ah, thanks, will correct.

@AndydeCleyre

This comment has been minimized.

Show comment
Hide comment
@AndydeCleyre

AndydeCleyre May 23, 2016

Contributor

I'd appreciate it if someone else could take a look at the state of this pull request. It seems that when running the test on travis, my fake_install function does not work as expected, though it works locally on my machine (I have been unable to run the tests locally, as described above).

So if someone could either figure out what's going on during the test with my fake_install function, or give me a way to run the test locally, I can move forward with this. Otherwise this (working) fix will remain in purgatory indefinitely.

Here's the fake_install code:

import os
import inspect


def fake_install(pkgname):
    egg_info_path = os.path.join(
        # Workaround for virtualenv issue #355 (no site.getsitepackages):
        os.path.dirname(inspect.getfile(pytest)),
        '{0}-1.0-py{1}.{2}.egg-info'.format(
            pkgname.replace('-', '_'),
            sys.version_info[0],
            sys.version_info[1]
        )
    )
    with open(egg_info_path, 'w') as egg_info_file:
        egg_info_file.write(
            'Metadata-Version: 1.0\nName: {0}\nVersion: 1.0\n'.format(
                pkgname
            )
        )
Contributor

AndydeCleyre commented May 23, 2016

I'd appreciate it if someone else could take a look at the state of this pull request. It seems that when running the test on travis, my fake_install function does not work as expected, though it works locally on my machine (I have been unable to run the tests locally, as described above).

So if someone could either figure out what's going on during the test with my fake_install function, or give me a way to run the test locally, I can move forward with this. Otherwise this (working) fix will remain in purgatory indefinitely.

Here's the fake_install code:

import os
import inspect


def fake_install(pkgname):
    egg_info_path = os.path.join(
        # Workaround for virtualenv issue #355 (no site.getsitepackages):
        os.path.dirname(inspect.getfile(pytest)),
        '{0}-1.0-py{1}.{2}.egg-info'.format(
            pkgname.replace('-', '_'),
            sys.version_info[0],
            sys.version_info[1]
        )
    )
    with open(egg_info_path, 'w') as egg_info_file:
        egg_info_file.write(
            'Metadata-Version: 1.0\nName: {0}\nVersion: 1.0\n'.format(
                pkgname
            )
        )
@AndydeCleyre

This comment has been minimized.

Show comment
Hide comment
@AndydeCleyre

AndydeCleyre May 24, 2016

Contributor

I've confirmed that fake_install is using, in the case of the Python 2.6 tests run, /home/travis/build/pypa/pip/.tox/py26/lib/python2.6/site-packages.

However, it seems that this is not the directory used when calling script.pip_install_local and script.pip('freeze'), as the simple and simple2 packages, installed during the same test, are not found in that location.

Does anyone know where those are actually installed to, or how to determine that?

Contributor

AndydeCleyre commented May 24, 2016

I've confirmed that fake_install is using, in the case of the Python 2.6 tests run, /home/travis/build/pypa/pip/.tox/py26/lib/python2.6/site-packages.

However, it seems that this is not the directory used when calling script.pip_install_local and script.pip('freeze'), as the simple and simple2 packages, installed during the same test, are not found in that location.

Does anyone know where those are actually installed to, or how to determine that?

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft May 24, 2016

Member

Ah, these might be using ``dist-packages` because we're running on a Debuntu derivative. I think that the code in

pip/pip/locations.py

Lines 124 to 182 in ce76f2d

def distutils_scheme(dist_name, user=False, home=None, root=None,
isolated=False, prefix=None):
"""
Return a distutils install scheme
"""
from distutils.dist import Distribution
scheme = {}
if isolated:
extra_dist_args = {"script_args": ["--no-user-cfg"]}
else:
extra_dist_args = {}
dist_args = {'name': dist_name}
dist_args.update(extra_dist_args)
d = Distribution(dist_args)
d.parse_config_files()
i = d.get_command_obj('install', create=True)
# NOTE: setting user or home has the side-effect of creating the home dir
# or user base for installations during finalize_options()
# ideally, we'd prefer a scheme class that has no side-effects.
assert not (user and prefix), "user={0} prefix={1}".format(user, prefix)
i.user = user or i.user
if user:
i.prefix = ""
i.prefix = prefix or i.prefix
i.home = home or i.home
i.root = root or i.root
i.finalize_options()
for key in SCHEME_KEYS:
scheme[key] = getattr(i, 'install_' + key)
# install_lib specified in setup.cfg should install *everything*
# into there (i.e. it takes precedence over both purelib and
# platlib). Note, i.install_lib is *always* set after
# finalize_options(); we only want to override here if the user
# has explicitly requested it hence going back to the config
if 'install_lib' in d.get_option_dict('install'):
scheme.update(dict(purelib=i.install_lib, platlib=i.install_lib))
if running_under_virtualenv():
scheme['headers'] = os.path.join(
sys.prefix,
'include',
'site',
'python' + sys.version[:3],
dist_name,
)
if root is not None:
path_no_drive = os.path.splitdrive(
os.path.abspath(scheme["headers"]))[1]
scheme["headers"] = os.path.join(
root,
path_no_drive[1:],
)
return scheme
should tell you where it would install.

Member

dstufft commented May 24, 2016

Ah, these might be using ``dist-packages` because we're running on a Debuntu derivative. I think that the code in

pip/pip/locations.py

Lines 124 to 182 in ce76f2d

def distutils_scheme(dist_name, user=False, home=None, root=None,
isolated=False, prefix=None):
"""
Return a distutils install scheme
"""
from distutils.dist import Distribution
scheme = {}
if isolated:
extra_dist_args = {"script_args": ["--no-user-cfg"]}
else:
extra_dist_args = {}
dist_args = {'name': dist_name}
dist_args.update(extra_dist_args)
d = Distribution(dist_args)
d.parse_config_files()
i = d.get_command_obj('install', create=True)
# NOTE: setting user or home has the side-effect of creating the home dir
# or user base for installations during finalize_options()
# ideally, we'd prefer a scheme class that has no side-effects.
assert not (user and prefix), "user={0} prefix={1}".format(user, prefix)
i.user = user or i.user
if user:
i.prefix = ""
i.prefix = prefix or i.prefix
i.home = home or i.home
i.root = root or i.root
i.finalize_options()
for key in SCHEME_KEYS:
scheme[key] = getattr(i, 'install_' + key)
# install_lib specified in setup.cfg should install *everything*
# into there (i.e. it takes precedence over both purelib and
# platlib). Note, i.install_lib is *always* set after
# finalize_options(); we only want to override here if the user
# has explicitly requested it hence going back to the config
if 'install_lib' in d.get_option_dict('install'):
scheme.update(dict(purelib=i.install_lib, platlib=i.install_lib))
if running_under_virtualenv():
scheme['headers'] = os.path.join(
sys.prefix,
'include',
'site',
'python' + sys.version[:3],
dist_name,
)
if root is not None:
path_no_drive = os.path.splitdrive(
os.path.abspath(scheme["headers"]))[1]
scheme["headers"] = os.path.join(
root,
path_no_drive[1:],
)
return scheme
should tell you where it would install.

@AndydeCleyre

This comment has been minimized.

Show comment
Hide comment
@AndydeCleyre

AndydeCleyre May 24, 2016

Contributor

Thanks @dstufft, got the right directory now, turns out the script passed into the test has a site_packages_path attribute.

The tests are passing now. The last problems to solve were accounting for extra packages installed in the pypy test, and python-version-dependent ordering of the stderr lines.

Is the appropriate package parsing order for pip freeze well-defined for each python version? I made the test account for current results, but could these orders change in the future for existing versions? If they could, we should probably break up the test into one for each invalid name, so the order is no longer a factor. If not, please go ahead and review this.

Thanks,
Andy

Contributor

AndydeCleyre commented May 24, 2016

Thanks @dstufft, got the right directory now, turns out the script passed into the test has a site_packages_path attribute.

The tests are passing now. The last problems to solve were accounting for extra packages installed in the pypy test, and python-version-dependent ordering of the stderr lines.

Is the appropriate package parsing order for pip freeze well-defined for each python version? I made the test account for current results, but could these orders change in the future for existing versions? If they could, we should probably break up the test into one for each invalid name, so the order is no longer a factor. If not, please go ahead and review this.

Thanks,
Andy

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft May 24, 2016

Member

I don't think the order is well defined no.

Member

dstufft commented May 24, 2016

I don't think the order is well defined no.

@AndydeCleyre

This comment has been minimized.

Show comment
Hide comment
@AndydeCleyre

AndydeCleyre May 24, 2016

Contributor

In that case, I'll update this in a few hours to check the stderr and stdout as many times as there are expected lines, one line per check. The only downside to this approach is that it won't catch any extra unexpected output.

Contributor

AndydeCleyre commented May 24, 2016

In that case, I'll update this in a few hours to check the stderr and stdout as many times as there are expected lines, one line per check. The only downside to this approach is that it won't catch any extra unexpected output.

AndydeCleyre added some commits May 24, 2016

Instead of matching exact outputs, just check for presence of expecte…
…d lines individually (circumventing portential unpredictable sequencing problems going forward).
only use fake_install, only test package names with '_', '-', '.', ad…
…d invalid cases with those chars at the end and valid cases with those chars in the middle.
add newlines to match strings to prevent greedy ellipsis matching fro…
…m bungling the task when the package name ends with a dot
@AndydeCleyre

This comment has been minimized.

Show comment
Hide comment
@AndydeCleyre

AndydeCleyre May 25, 2016

Contributor

Alright, I'm personally satisfied with the state of this. Please take a look when you get a chance.

Contributor

AndydeCleyre commented May 25, 2016

Alright, I'm personally satisfied with the state of this. Please take a look when you get a chance.

@dstufft dstufft merged commit e445dba into pypa:develop May 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft May 26, 2016

Member

Thanks!

Member

dstufft commented May 26, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment