Fix lock idempotence

4baf68a2 · Sébastien Eustace · 46ad8072 · 4baf68a2 · 4baf68a2 · 4baf68a2
Commit 4baf68a2 authored Apr 09, 2019 by Sébastien Eustace
Showing with 170 additions and 63 deletions

poetry/installation/installer.py
+12 -39

tests/console/commands/test_add.py
+24 -24

tests/installation/fixtures/with-multiple-updates.test
+57 -0

tests/installation/test_installer.py
+77 -0

No files found.
--- a/poetry/installation/installer.py
+++ b/poetry/installation/installer.py
-import sys
 from typing import List
 from typing import Union
@@ -7,7 +5,6 @@ from poetry.io import NullIO
 from poetry.packages import Dependency
 from poetry.packages import Locker
 from poetry.packages import Package
-from poetry.packages.constraints import parse_constraint as parse_generic_constraint
 from poetry.puzzle import Solver
 from poetry.puzzle.operations import Install
 from poetry.puzzle.operations import Uninstall
@@ -17,7 +14,6 @@ from poetry.repositories import Pool
 from poetry.repositories import Repository
 from poetry.repositories.installed_repository import InstalledRepository
 from poetry.semver import parse_constraint
-from poetry.semver import Version
 from poetry.utils.helpers import canonicalize_name
 from .base_installer import BaseInstaller
@@ -143,7 +139,7 @@ class Installer:
    def _do_install(self, local_repo):
        locked_repository = Repository()
        if self._update:
-            if self._locker.is_locked():
+            if self._locker.is_locked() and not self._lock:
                locked_repository = self._locker.locked_repository(True)
                # If no packages have been whitelisted (The ones we want to update),
@@ -191,13 +187,14 @@ class Installer:
            # currently installed
            ops = self._get_operations_from_lock(locked_repository)
-        self._populate_local_repo(local_repo, ops, locked_repository)
+        self._populate_local_repo(local_repo, ops)
-        if self._lock:
+        if self._update:
-            # If we are only in lock mode, no need to go any further
            self._write_lock_file(local_repo)
-            return 0
+            if self._lock:
+                # If we are only in lock mode, no need to go any further
+                return 0
        root = self._package
        if not self.is_dev_mode():
@@ -289,9 +286,6 @@ class Installer:
                )
            )
-        # Writing lock before installing
-        self._write_lock_file(local_repo)
        self._io.writeln("")
        for op in ops:
            self._execute(op)
@@ -392,38 +386,17 @@ class Installer:
        self._installer.remove(operation.package)
-    def _populate_local_repo(self, local_repo, ops, locked_repository):
+    def _populate_local_repo(self, local_repo, ops):
-        # We walk through all operations and add/remove/update accordingly
        for op in ops:
-            if isinstance(op, Update):
+            if isinstance(op, Uninstall):
+                continue
+            elif isinstance(op, Update):
                package = op.target_package
            else:
                package = op.package
-            acted_on = False
+            if not local_repo.has_package(package):
-            for pkg in locked_repository.packages:
+                local_repo.add_package(package)
-                if pkg.name == package.name:
-                    # The package we operate on is in the local repo
-                    if op.job_type == "update":
-                        if pkg.version == package.version:
-                            break
-                        local_repo.remove_package(pkg)
-                        local_repo.add_package(op.target_package)
-                    elif op.job_type == "uninstall":
-                        local_repo.remove_package(op.package)
-                    else:
-                        # Even though the package already exists
-                        # in the lock file we will prefer the new one
-                        # to force updates
-                        local_repo.remove_package(pkg)
-                        local_repo.add_package(package)
-                    acted_on = True
-            if not acted_on:
-                if not local_repo.has_package(package):
-                    local_repo.add_package(package)
    def _get_operations_from_lock(
        self, locked_repository  # type: Repository

--- a/tests/console/commands/test_add.py
+++ b/tests/console/commands/test_add.py
@@ -21,10 +21,10 @@ Using version ^0.2.0 for cachy
 Updating dependencies
 Resolving dependencies...
+Writing lock file
-Package operations: 1 install, 0 updates, 0 removals
-Writing lock file
+Package operations: 1 install, 0 updates, 0 removals
  - Installing cachy (0.2.0)
 """
@@ -53,10 +53,10 @@ def test_add_constraint(app, repo, installer):
 Updating dependencies
 Resolving dependencies...
+Writing lock file
-Package operations: 1 install, 0 updates, 0 removals
-Writing lock file
+Package operations: 1 install, 0 updates, 0 removals
  - Installing cachy (0.1.0)
 """
@@ -85,11 +85,11 @@ def test_add_constraint_dependencies(app, repo, installer):
 Updating dependencies
 Resolving dependencies...
+Writing lock file
 Package operations: 2 installs, 0 updates, 0 removals
-Writing lock file
  - Installing msgpack-python (0.5.3)
  - Installing cachy (0.2.0)
 """
@@ -119,10 +119,10 @@ def test_add_git_constraint(app, repo, installer):
 Updating dependencies
 Resolving dependencies...
+Writing lock file
-Package operations: 2 installs, 0 updates, 0 removals
-Writing lock file
+Package operations: 2 installs, 0 updates, 0 removals
  - Installing pendulum (1.4.4)
  - Installing demo (0.1.2 9cf87a2)
@@ -159,10 +159,10 @@ def test_add_git_constraint_with_poetry(app, repo, installer):
 Updating dependencies
 Resolving dependencies...
+Writing lock file
-Package operations: 2 installs, 0 updates, 0 removals
-Writing lock file
+Package operations: 2 installs, 0 updates, 0 removals
  - Installing pendulum (1.4.4)
  - Installing demo (0.1.2 9cf87a2)
@@ -192,11 +192,11 @@ def test_add_file_constraint_wheel(app, repo, installer):
 Updating dependencies
 Resolving dependencies...
+Writing lock file
 Package operations: 2 installs, 0 updates, 0 removals
-Writing lock file
  - Installing pendulum (1.4.4)
  - Installing demo (0.1.0 ../distributions/demo-0.1.0-py2.py3-none-any.whl)
 """
@@ -232,10 +232,10 @@ def test_add_file_constraint_sdist(app, repo, installer):
 Updating dependencies
 Resolving dependencies...
+Writing lock file
-Package operations: 2 installs, 0 updates, 0 removals
-Writing lock file
+Package operations: 2 installs, 0 updates, 0 removals
  - Installing pendulum (1.4.4)
  - Installing demo (0.1.0 ../distributions/demo-0.1.0.tar.gz)
@@ -279,10 +279,10 @@ def test_add_constraint_with_extras(app, repo, installer):
 Updating dependencies
 Resolving dependencies...
+Writing lock file
-Package operations: 2 installs, 0 updates, 0 removals
-Writing lock file
+Package operations: 2 installs, 0 updates, 0 removals
  - Installing msgpack-python (0.5.3)
  - Installing cachy (0.2.0)
@@ -323,11 +323,11 @@ def test_add_constraint_with_python(app, repo, installer):
 Updating dependencies
 Resolving dependencies...
+Writing lock file
 Package operations: 1 install, 0 updates, 0 removals
-Writing lock file
  - Installing cachy (0.2.0)
 """
@@ -364,10 +364,10 @@ def test_add_constraint_with_platform(app, repo, installer):
 Updating dependencies
 Resolving dependencies...
+Writing lock file
-Package operations: 1 install, 0 updates, 0 removals
-Writing lock file
+Package operations: 1 install, 0 updates, 0 removals
  - Installing cachy (0.2.0)
 """
@@ -402,10 +402,10 @@ Using version ^0.2.0 for cachy
 Updating dependencies
 Resolving dependencies...
+Writing lock file
-Package operations: 1 install, 0 updates, 0 removals
-Writing lock file
+Package operations: 1 install, 0 updates, 0 removals
  - Installing cachy (0.2.0)
 """
@@ -435,11 +435,11 @@ Using version ^3.13 for pyyaml
 Updating dependencies
 Resolving dependencies...
+Writing lock file
 Package operations: 1 install, 0 updates, 0 removals
-Writing lock file
  - Installing pyyaml (3.13)
 """

--- a/tests/installation/fixtures/with-multiple-updates.test
+++ b/tests/installation/fixtures/with-multiple-updates.test
+[[package]]
+name = "A"
+version = "1.0"
+description = ""
+category = "main"
+optional = false
+python-versions = "*"
+[package.dependencies]
+B = ">=1.0.1"
+C = [
+    {version = "^1.0", python = ">=2.7,<2.8"},
+    {version = "^2.0", python = ">=3.4,<4.0"},
+]
+[[package]]
+name = "B"
+version = "1.0.1"
+description = ""
+category = "main"
+optional = false
+python-versions = ">=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*"
+[[package]]
+name = "B"
+version = "1.1.0"
+description = ""
+category = "main"
+optional = false
+python-versions = ">=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*"
+[[package]]
+name = "C"
+version = "1.0"
+description = ""
+category = "main"
+optional = false
+marker = "python_version >= \"2.7\" and python_version < \"2.8\""
+python-versions = "*"
+[[package]]
+name = "C"
+version = "2.0"
+description = ""
+category = "main"
+optional = false
+marker = "python_version >= \"3.4\" and python_version < \"4.0\""
+python-versions = "*"
+[metadata]
+python-versions = "~2.7 || ^3.4"
+content-hash = "123456789"
+[metadata.hashes]
+A = []
+B = []
+C = []
--- a/tests/installation/test_installer.py
+++ b/tests/installation/test_installer.py
@@ -1279,3 +1279,80 @@ def test_installer_test_solver_finds_compatible_package_for_dependency_python_no
        assert len(installs) == 1
    else:
        assert len(installs) == 0
+def test_update_multiple_times_with_split_dependencies_is_idempotent(
+    installer, locker, repo, package
+):
+    locker.locked(True)
+    locker.mock_lock_data(
+        {
+            "package": [
+                {
+                    "name": "A",
+                    "version": "1.0",
+                    "category": "main",
+                    "optional": False,
+                    "platform": "*",
+                    "python-versions": "*",
+                    "checksum": [],
+                    "dependencies": {"B": ">=1.0"},
+                },
+                {
+                    "name": "B",
+                    "version": "1.0.1",
+                    "category": "main",
+                    "optional": False,
+                    "platform": "*",
+                    "python-versions": ">=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*",
+                    "checksum": [],
+                    "dependencies": {},
+                },
+            ],
+            "metadata": {
+                "python-versions": "*",
+                "platform": "*",
+                "content-hash": "123456789",
+                "hashes": {"A": [], "B": []},
+            },
+        }
+    )
+    package.python_versions = "~2.7 || ^3.4"
+    package.add_dependency("A", "^1.0")
+    a = get_package("A", "1.0")
+    a.add_dependency("B", ">=1.0.1")
+    a.add_dependency("C", {"version": "^1.0", "python": "~2.7"})
+    a.add_dependency("C", {"version": "^2.0", "python": "^3.4"})
+    b101 = get_package("B", "1.0.1")
+    b101.python_versions = ">=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*"
+    b110 = get_package("B", "1.1.0")
+    b110.python_versions = ">=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*"
+    repo.add_package(a)
+    repo.add_package(b101)
+    repo.add_package(b110)
+    repo.add_package(get_package("C", "1.0"))
+    repo.add_package(get_package("C", "2.0"))
+    installer.update(True)
+    installer.run()
+    expected = fixture("with-multiple-updates")
+    import json
+    assert expected == locker.written_data
+    locker.mock_lock_data(locker.written_data)
+    installer.update(True)
+    installer.run()
+    assert expected == locker.written_data
+    locker.mock_lock_data(locker.written_data)
+    installer.update(True)
+    installer.run()
+    assert expected == locker.written_data