From 025f66899b2c53fd0abf6dea9808eabdedee3a69 Mon Sep 17 00:00:00 2001 From: Dan Ryan Date: Thu, 7 Jun 2018 01:07:28 -0400 Subject: [PATCH 1/5] Refactor and cleanup some locking code - This has been driving me mad. Signed-off-by: Dan Ryan --- pipenv/core.py | 186 ++++++++++++++++++++---------------------------- pipenv/utils.py | 68 ++++++++++-------- 2 files changed, 116 insertions(+), 138 deletions(-) diff --git a/pipenv/core.py b/pipenv/core.py index 51d21d65..e4a94d31 100644 --- a/pipenv/core.py +++ b/pipenv/core.py @@ -49,6 +49,7 @@ from .utils import ( split_argument, extract_uri_from_vcs_dep, fs_str, + clean_resolved_dep, ) from ._compat import ( TemporaryDirectory, @@ -1009,8 +1010,6 @@ def do_lock( ): """Executes the freeze functionality.""" from .utils import get_vcs_deps - from notpip._vendor.distlib.markers import DEFAULT_CONTEXT as marker_context - allowed_marker_keys = ['markers'] + [k for k in marker_context.keys()] cached_lockfile = {} if not pre: pre = project.settings.get('allow_prereleases') @@ -1023,16 +1022,6 @@ def do_lock( ) sys.exit(1) cached_lockfile = project.lockfile_content - if write: - # Alert the user of progress. - click.echo( - u'{0} {1} {2}'.format( - crayons.normal('Locking'), - crayons.red('[dev-packages]'), - crayons.normal('dependencies…'), - ), - err=True, - ) # Create the lockfile. lockfile = project._lockfile # Cleanup lockfile. @@ -1049,104 +1038,85 @@ def do_lock( pip_freeze = delegator.run( '{0} freeze'.format(escape_grouped_arguments(which_pip(allow_global=system))) ).out - deps = convert_deps_to_pip( - dev_packages, project, r=False, include_index=True - ) - results = venv_resolve_deps( - deps, - which=which, - verbose=verbose, - project=project, - clear=clear, - pre=pre, - allow_global=system, - pypi_mirror=pypi_mirror, - ) - # Add develop dependencies to lockfile. - for dep in results: - # Add version information to lockfile. - lockfile['develop'].update( - {dep['name']: {'version': '=={0}'.format(dep['version'])}} - ) - # Add Hashes to lockfile - lockfile['develop'][dep['name']]['hashes'] = dep['hashes'] - # Add index metadata to lockfile. - if 'index' in dep: - lockfile['develop'][dep['name']]['index'] = dep['index'] - # Add PEP 508 specifier metadata to lockfile if dep isnt top level - # or top level dep doesn't itself have markers - if 'markers' in dep: - if dep['name'] in dev_packages and not any(key in dev_packages[dep['name']] for key in allowed_marker_keys): - continue - lockfile['develop'][dep['name']]['markers'] = dep['markers'] - # Add refs for VCS installs. - # TODO: be smarter about this. - vcs_dev_lines, vcs_dev_lockfiles = get_vcs_deps(project, pip_freeze, which=which, verbose=verbose, clear=clear, pre=pre, allow_global=system, dev=True, pypi_mirror=pypi_mirror) - for lf in vcs_dev_lockfiles: - try: - name = first(lf.keys()) - except AttributeError: - continue - if hasattr(lf[name], 'keys'): - lockfile['develop'].update(lf) - if write: - # Alert the user of progress. - click.echo( - u'{0} {1} {2}'.format( - crayons.normal('Locking'), - crayons.red('[packages]'), - crayons.normal('dependencies…'), - ), - err=True, - ) - # Resolve package dependencies, with pip-tools. - deps = convert_deps_to_pip( - project.packages, project, r=False, include_index=True - ) - results = venv_resolve_deps( - deps, - which=which, - verbose=verbose, - project=project, - clear=False, - pre=pre, - allow_global=system, - pypi_mirror=pypi_mirror, - ) - # Add default dependencies to lockfile. - for dep in results: - # Add version information to lockfile. - pipfile_version = project.packages[dep['name']] if dep['name'] in project.packages else None - if pipfile_version and hasattr(pipfile_version, 'keys') and any(k for k in ['file', 'path'] if k in pipfile_version): - lockfile['default'].update( - {dep['name']: dict(pipfile_version)} + sections = { + 'dev': { + 'packages': project.dev_packages, + 'vcs': project.vcs_dev_packages, + 'pipfile_key': 'dev_packages', + 'lockfile_key': 'develop', + 'log_string': 'dev-packages', + 'dev': True + }, + 'default': { + 'packages': project.packages, + 'vcs': project.vcs_packages, + 'pipfile_key': 'packages', + 'lockfile_key': 'default', + 'log_string': 'packages', + 'dev': False + } + } + for section_name, settings in sections.items(): + if write: + # Alert the user of progress. + click.echo( + u'{0} {1} {2}'.format( + crayons.normal('Locking'), + crayons.red('[{0}]'.format(settings['log_string'])), + crayons.normal('dependencies…'), + ), + err=True, ) - continue - lockfile['default'].update( - {dep['name']: {'version': '=={0}'.format(dep['version'])}} - ) - # Add Hashes to lockfile - lockfile['default'][dep['name']]['hashes'] = dep['hashes'] - # Add index metadata to lockfile. - if 'index' in dep: - lockfile['default'][dep['name']]['index'] = dep['index'] - # Add PEP 508 specifier metadata to lockfile if dep isn't top level - # or top level dep has no specifiers itself - if 'markers' in dep: - if dep['name'] in project.packages and not any(key in project.packages[dep['name']] for key in allowed_marker_keys): - continue - lockfile['default'][dep['name']]['markers'] = dep['markers'] - # Add refs for VCS installs. - # TODO: be smarter about this. - _vcs_deps, vcs_lockfiles = get_vcs_deps(project, pip_freeze, which=which, verbose=verbose, clear=clear, pre=pre, allow_global=system, dev=False, pypi_mirror=pypi_mirror) - for lf in vcs_lockfiles: - try: - name = first(lf.keys()) - except AttributeError: - continue - if hasattr(lf[name], 'keys'): - lockfile['default'].update(lf) + deps = convert_deps_to_pip( + settings['packages'], project, r=False, include_index=True + ) + results = venv_resolve_deps( + deps, + which=which, + verbose=verbose, + project=project, + clear=clear, + pre=pre, + allow_global=system, + pypi_mirror=pypi_mirror, + ) + # Add develop dependencies to lockfile. + for dep in results: + is_top_level = dep['name'] in settings['packages'] + pipfile_entry = settings['packages'][dep['name']] if is_top_level else None + dep_lockfile = clean_resolved_dep(dep, is_top_level=is_top_level, pipfile_entry=pipfile_entry) + lockfile[settings['lockfile_key']].update(dep_lockfile) + # Add refs for VCS installs. + # TODO: be smarter about this. + vcs_lines, vcs_lockfile = get_vcs_deps( + project, + pip_freeze, + which=which, + verbose=verbose, + clear=clear, + pre=pre, + allow_global=system, + dev=settings['dev'] + ) + vcs_results = venv_resolve_deps( + vcs_lines, + which=which, + verbose=verbose, + project=project, + clear=clear, + pre=pre, + allow_global=system, + pypi_mirror=pypi_mirror, + ) + for dep in vcs_results: + if not hasattr(dep, 'keys') or not hasattr(dep['name'], 'keys'): + continue + is_top_level = dep['name'] in vcs_lockfile + pipfile_entry = vcs_lockfile[dep['name']] if is_top_level else None + dep_lockfile = clean_resolved_dep(dep, is_top_level=is_top_level, pipfile_entry=pipfile_entry) + vcs_lockfile.update(dep_lockfile) + lockfile[settings['lockfile_key']].update(vcs_lockfile) # Support for --keep-outdated… if keep_outdated: for section_name, section in ( diff --git a/pipenv/utils.py b/pipenv/utils.py index 1e765b0a..34140c81 100644 --- a/pipenv/utils.py +++ b/pipenv/utils.py @@ -1164,7 +1164,7 @@ def get_vcs_deps( section = "vcs_dev_packages" if dev else "vcs_packages" lines = [] - lockfiles = [] + lockfile = {} try: packages = getattr(project, section) except AttributeError: @@ -1208,37 +1208,45 @@ def get_vcs_deps( ) if installed.is_vcs: installed.req.ref = locked_rev - lockfiles.append({pipfile_name: installed.pipfile_entry[1]}) - pipfile_srcdir = (src_dir / pipfile_name).as_posix() - lockfile_srcdir = (src_dir / installed.normalized_name).as_posix() + lockfile[pipfile_name] = installed.pipfile_entry[1] lines.append(line) - if os.path.exists(pipfile_srcdir): - lockfiles.extend( - venv_resolve_deps( - ["-e {0}".format(pipfile_srcdir)], - which=which, - verbose=verbose, - project=project, - clear=clear, - pre=pre, - allow_global=allow_global, - pypi_mirror=pypi_mirror, - ) - ) + return lines, lockfile + + +def clean_resolved_dep(dep, is_top_level=False, pipfile_entry=None): + from notpip._vendor.distlib.markers import DEFAULT_CONTEXT as marker_context + allowed_marker_keys = ['markers'] + [k for k in marker_context.keys()] + name = dep['name'] + # We use this to determine if there are any markers on top level packages + # So we can make sure those win out during resolution if the packages reoccur + dep_keys = [k for k in getattr(pipfile_entry, 'keys', list)()] if is_top_level else [] + lockfile = { + 'version': '=={0}'.format(dep['version']), + } + for key in ['hashes', 'index']: + if key in dep: + lockfile[key] = dep[key] + # In case we lock a uri or a file when the user supplied a path + if pipfile_entry and any(k in pipfile_entry for k in ['file', 'path']): + fs_key = next((k for k in ['path', 'file'] if k in pipfile_entry), None) + lockfile_key = next((k for k in ['uri', 'file', 'path'] if k in lockfile), None) + if fs_key != lockfile_key: + del lockfile[lockfile_key] + lockfile[fs_key] = pipfile_entry[fs_key] + + # If a package is **PRESENT** in the pipfile but has no markers, make sure we + # **NEVER** include markers in the lockfile + if 'markers' in dep: + # First, handle the case where there is no top level dependency in the pipfile + if not is_top_level: + lockfile['markers'] = dep['markers'] + # otherwise make sure we are prioritizing whatever the pipfile says about the markers + # If the pipfile says nothing, then we should put nothing in the lockfile else: - lockfiles.extend( - venv_resolve_deps( - ["-e {0}".format(lockfile_srcdir)], - which=which, - verbose=verbose, - project=project, - clear=clear, - pre=pre, - allow_global=allow_global, - pypi_mirror=pypi_mirror, - ) - ) - return lines, lockfiles + pipfile_marker = next((k for k in dep_keys if k in allowed_marker_keys), None) + if pipfile_marker: + lockfile['markers'] = "{0}{1}".format(pipfile_marker, pipfile_entry[pipfile_marker]) + return {name: lockfile} def fs_str(string): From 469db93005b111bcb50f1ea91503e16776777199 Mon Sep 17 00:00:00 2001 From: Dan Ryan Date: Thu, 7 Jun 2018 01:33:20 -0400 Subject: [PATCH 2/5] Put a space between marker name and value Signed-off-by: Dan Ryan --- pipenv/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pipenv/utils.py b/pipenv/utils.py index 34140c81..a333bb85 100644 --- a/pipenv/utils.py +++ b/pipenv/utils.py @@ -1245,7 +1245,7 @@ def clean_resolved_dep(dep, is_top_level=False, pipfile_entry=None): else: pipfile_marker = next((k for k in dep_keys if k in allowed_marker_keys), None) if pipfile_marker: - lockfile['markers'] = "{0}{1}".format(pipfile_marker, pipfile_entry[pipfile_marker]) + lockfile['markers'] = "{0} {1}".format(pipfile_marker, pipfile_entry[pipfile_marker]) return {name: lockfile} From 168c93cfc15cc533a95415ea0f6665b562473005 Mon Sep 17 00:00:00 2001 From: Dan Ryan Date: Thu, 7 Jun 2018 01:49:44 -0400 Subject: [PATCH 3/5] Check marker key before generating string Signed-off-by: Dan Ryan --- pipenv/utils.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pipenv/utils.py b/pipenv/utils.py index a333bb85..46c237cf 100644 --- a/pipenv/utils.py +++ b/pipenv/utils.py @@ -1245,7 +1245,10 @@ def clean_resolved_dep(dep, is_top_level=False, pipfile_entry=None): else: pipfile_marker = next((k for k in dep_keys if k in allowed_marker_keys), None) if pipfile_marker: - lockfile['markers'] = "{0} {1}".format(pipfile_marker, pipfile_entry[pipfile_marker]) + entry = "{0}".format(pipfile_entry[pipfile_marker]) + if pipfile_marker != 'markers': + entry = "{0}{1}".format(pipfile_entry, entry) + lockfile['markers'] = entry return {name: lockfile} From 0eb9c4c165fa9a993f8370b9bb30652a02511c81 Mon Sep 17 00:00:00 2001 From: Dan Ryan Date: Thu, 7 Jun 2018 01:55:28 -0400 Subject: [PATCH 4/5] Typo... we want the key, not the whole entry Signed-off-by: Dan Ryan --- pipenv/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pipenv/utils.py b/pipenv/utils.py index 46c237cf..44ace3ce 100644 --- a/pipenv/utils.py +++ b/pipenv/utils.py @@ -1247,7 +1247,7 @@ def clean_resolved_dep(dep, is_top_level=False, pipfile_entry=None): if pipfile_marker: entry = "{0}".format(pipfile_entry[pipfile_marker]) if pipfile_marker != 'markers': - entry = "{0}{1}".format(pipfile_entry, entry) + entry = "{0}{1}".format(pipfile_marker, entry) lockfile['markers'] = entry return {name: lockfile} From be75afb2a906a80d3689ed54e30b1f6bfc730c66 Mon Sep 17 00:00:00 2001 From: Dan Ryan Date: Thu, 7 Jun 2018 11:04:44 -0400 Subject: [PATCH 5/5] Add a space for markers that we transform - Cleanup stale comments - Deterministic ordering for locking Signed-off-by: Dan Ryan --- pipenv/core.py | 5 +++-- pipenv/utils.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pipenv/core.py b/pipenv/core.py index e4a94d31..4d06a66c 100644 --- a/pipenv/core.py +++ b/pipenv/core.py @@ -1056,7 +1056,8 @@ def do_lock( 'dev': False } } - for section_name, settings in sections.items(): + for section_name in ['dev', 'default']: + settings = sections[section_name] if write: # Alert the user of progress. click.echo( @@ -1081,7 +1082,7 @@ def do_lock( allow_global=system, pypi_mirror=pypi_mirror, ) - # Add develop dependencies to lockfile. + # Add dependencies to lockfile. for dep in results: is_top_level = dep['name'] in settings['packages'] pipfile_entry = settings['packages'][dep['name']] if is_top_level else None diff --git a/pipenv/utils.py b/pipenv/utils.py index 44ace3ce..56a387c8 100644 --- a/pipenv/utils.py +++ b/pipenv/utils.py @@ -1247,7 +1247,7 @@ def clean_resolved_dep(dep, is_top_level=False, pipfile_entry=None): if pipfile_marker: entry = "{0}".format(pipfile_entry[pipfile_marker]) if pipfile_marker != 'markers': - entry = "{0}{1}".format(pipfile_marker, entry) + entry = "{0} {1}".format(pipfile_marker, entry) lockfile['markers'] = entry return {name: lockfile}