Defend against parallel package builds via a file lock

.codeclimate.yml

@@ -1,20 +1,37 @@
-engines:
-  duplication:
-    enabled: true
+version: "2"
+checks:
+  argument-count:
     config:
-      languages:
-        python:
-          python_version: 3
-  pep8:
-    enabled: true
-  radon:
-    enabled: true
+      threshold: 6
+  complex-logic:
+    config:
+      threshold: 10
+  file-lines:
+    config:
+      threshold: 500
+  method-complexity:
+    config:
+      threshold: 10
+  method-count:
+    config:
+      threshold: 20
+  method-lines:
+    config:
+      threshold: 50
+  nested-control-flow:
+    config:
+      threshold: 4
+  return-statements:
+    config:
+      threshold: 10
+
+plugins:
   sonar-python:
     enabled: true
     config:
       tests_patterns:
       - tests/**
-
-ratings:
-  paths:
-  - "**.py"
+  pep8:
+    enabled: false
+  radon:
+    enabled: false

.pre-commit-config.yaml

@@ -1,6 +1,6 @@
 repos:
 -   repo: https://github.com/ambv/black
-    rev: 18.6b4
+    rev: 18.9b0
     hooks:
     -   id: black
         args: [--line-length=99, --safe]

azure-pipelines.yml

@@ -5,8 +5,9 @@ variables:
   CI_NAME: Azure Pipelines
   CI_BUILD_ID: $(Build.BuildId)
   CI_BUILD_URL: "https://toxdev.visualstudio.com/tox/_build/results?buildId=$(Build.BuildId)"
-  GIT_BRANCH: $(Build.SourceBranch)
+  GIT_BRANCH: $(Build.SourceBranchName)
   GIT_COMMIT_SHA: $(Build.SourceVersion)
+  PYTEST_ADDOPTS: "-vv -ra --showlocals"
 
 trigger:
   batch: true

changelog/1026.bugfix.rst

@@ -0,0 +1 @@
+intermittent failures with ``--parallel--safe-build``, instead of mangling with the file paths now uses a lock to make the package build operation thread safe and is now on by default (``--parallel--safe-build`` is now deprecated) - by :user:`gaborbernat`

changelog/1026.feature.rst

@@ -0,0 +1,7 @@
+Added ``temp_dir`` folder configuration (defaults to ``{toxworkdir}/.tmp``) that contains tox
+temporary files. Package builds now create a hard link (if possible, otherwise copy - notably in
+case of Windows Python 2.7) to the built file, and feed that file downstream (e.g. for pip to
+install it). The hard link is removed at the end of the run (what it points though is kept
+inside ``distdir``). This ensures that a tox session operates on the same package it built, even
+if a parallel tox run builds another version. Note ``distdir`` will contain only the last built
+package in such cases. - by :user:`gaborbernat`

doc/config.rst

@@ -43,6 +43,13 @@ Global settings are defined under the ``tox`` section as:
 
    Directory for tox to generate its environments into, will be created if it does not exist.
 
+.. conf:: temp_dir ^ PATH ^ {toxinidir}/.tmp
+
+   Directory where to put tox temporary files. For example: we create a hard link (if possible,
+   otherwise new copy) in this directory for the project package. This ensures tox works correctly
+   when having parallel runs (as each session will have its own copy of the project package - e.g.
+   the source distribution).
+
 .. conf:: skipsdist ^ true|false ^ false
 
    Flag indicating to perform the packaging operation or not. Set it to ``true`` when using tox for
@@ -53,16 +60,21 @@ Global settings are defined under the ``tox`` section as:
    Indicates where the packaging root file exists (historically the ``setup.py`` for ``setuptools``).
    This will be the working directory when performing the packaging.
 
-
 .. conf:: distdir ^ PATH ^ {toxworkdir}/dist
 
    Directory where the packaged source distribution should be put. Note this is cleaned at the start of
    every packaging invocation.
 
-.. conf:: distshare ^ PATH ^ {toxworkdir}/distshare
+.. conf:: sdistsrc ^ PATH ^ {toxworkdir}/dist
+
+   Do not build the package, but instead use teh latest package available under this path.
+   You can override it via the command line flag ``--installpkg``.
+
+.. conf:: distshare ^ PATH ^ {homedir}/.tox/distshare
 
    Folder where the packaged source distribution will be moved, this is not cleaned between packaging
-   invocations.
+   invocations. On Jenkins (exists ``JENKINS_URL`` or ``HUDSON_URL`` environment variable)
+   the default path is ``{toxworkdir}/distshare``.
 
 .. conf:: envlist ^ comma separated values
 

setup.py

@@ -80,6 +80,7 @@ def main():
             "six >= 1.0.0, <2",
             "virtualenv >= 1.11.2",
             "toml >=0.9.4",
+            "filelock >= 3.0.0, <4",
         ],
         extras_require={
             "testing": [

src/tox/_pytestplugin.py

@@ -4,6 +4,7 @@
 import sys
 import textwrap
 import time
+from collections import OrderedDict
 from fnmatch import fnmatch
 
 import py
@@ -408,12 +409,30 @@ def mock_venv(monkeypatch):
 
     # object to collect some data during the execution
     class Result(object):
-        def __init__(self):
+        def __init__(self, session):
             self.popens = []
-            self.cwd = None
-            self.session = None
-
-    res = Result()
+            self._popen = session.popen
+
+            # collect all popen calls
+            def popen(cmd, **kwargs):
+                # we don't want to perform installation of new packages,
+                # just replace with an always ok cmd
+                if "pip" in cmd and "install" in cmd:
+                    cmd = ["python", "-c", "print({!r})".format(cmd)]
+                activity_id = session._actions[-1].id
+                activity_name = session._actions[-1].activity
+                try:
+                    ret = self._popen(cmd, **kwargs)
+                except tox.exception.InvocationError as exception:  # pragma: no cover
+                    ret = exception  # pragma: no cover
+                finally:
+                    self.popens.append((activity_id, activity_name, kwargs.get("env"), ret, cmd))
+                return ret
+
+            monkeypatch.setattr(session, "popen", popen)
+            self.session = session
+
+    res = OrderedDict()
 
     # convince tox that the current running virtual environment is already the env we would create
     class ProxyCurrentPython:
@@ -468,28 +487,11 @@ def tox_runenvreport(venv, action):
     prev_build = tox.session.build_session
 
     def build_session(config):
-        res.session = prev_build(config)
-        res._popen = res.session.popen
-        monkeypatch.setattr(res.session, "popen", popen)
-        return res.session
+        session = prev_build(config)
+        res[id(session)] = Result(session)
+        return session
 
     monkeypatch.setattr(tox.session, "build_session", build_session)
-
-    # collect all popen calls
-    def popen(cmd, **kwargs):
-        # we don't want to perform installation of new packages, just replace with an always ok cmd
-        if "pip" in cmd and "install" in cmd:
-            cmd = ["python", "-c", "print({!r})".format(cmd)]
-        activity_id = res.session._actions[-1].id
-        activity_name = res.session._actions[-1].activity
-        try:
-            ret = res._popen(cmd, **kwargs)
-        except tox.exception.InvocationError as exception:  # pragma: no cover
-            ret = exception  # pragma: no cover
-        finally:
-            res.popens.append((activity_id, activity_name, kwargs.get("env"), ret, cmd))
-        return ret
-
     return res
 
 

src/tox/config.py

@@ -8,7 +8,6 @@
 import shlex
 import string
 import sys
-import uuid
 import warnings
 from collections import OrderedDict
 from fnmatch import fnmatchcase
@@ -419,7 +418,8 @@ def tox_addoption(parser):
         "--parallel--safe-build",
         action="store_true",
         dest="parallel_safe_build",
-        help="ensure two tox builds can run in parallel",
+        help="(deprecated) ensure two tox builds can run in parallel "
+        "(uses a lock file in the tox workdir with .lock extension)",
     )
     parser.add_argument(
         "--installpkg",
@@ -1018,20 +1018,17 @@ def __init__(self, config, ini_path, ini_data):  # noqa
         if override:
             for name in config.indexserver:
                 config.indexserver[name] = IndexServerConfig(name, override)
-        unique_id = str(uuid.uuid4())
 
         reader.addsubstitutions(toxworkdir=config.toxworkdir)
         config.distdir = reader.getpath("distdir", "{toxworkdir}/dist")
-        self._make_thread_safe_path(config, "distdir", unique_id)
 
         reader.addsubstitutions(distdir=config.distdir)
         config.distshare = reader.getpath("distshare", dist_share_default)
-        self._make_thread_safe_path(config, "distshare", unique_id)
+        config.temp_dir = reader.getpath("temp_dir", "{toxworkdir}/.tmp")
         reader.addsubstitutions(distshare=config.distshare)
         config.sdistsrc = reader.getpath("sdistsrc", None)
         config.setupdir = reader.getpath("setupdir", "{toxinidir}")
         config.logdir = config.toxworkdir.join("log")
-        self._make_thread_safe_path(config, "logdir", unique_id)
 
         self.parse_build_isolation(config, reader)
         config.envlist, all_envs = self._getenvdata(reader, config)
@@ -1076,12 +1073,6 @@ def parse_build_isolation(self, config, reader):
                     name, "{}{}".format(testenvprefix, name), reader._subs, config
                 )
 
-    def _make_thread_safe_path(self, config, attr, unique_id):
-        if config.option.parallel_safe_build:
-            path = getattr(config, attr)
-            value = py.path.local(path.dirname).join("{}-{}".format(path.basename, unique_id))
-            setattr(config, attr, value)
-
     @staticmethod
     def ensure_requires_satisfied(specified):
         missing_requirements = []

src/tox/package.py

@@ -1,11 +1,14 @@
 import json
+import os
 import sys
 import textwrap
 from collections import namedtuple
+from itertools import chain
 
 import pkg_resources
 import py
 import six
+from filelock import FileLock, Timeout
 
 import tox
 from tox.config import DepConfig, get_py_project_toml
@@ -17,7 +20,7 @@
 def tox_package(session, venv):
     """Build an sdist at first call return that for all calls"""
     if not hasattr(session, "package"):
-        session.package = get_package(session)
+        session.package, session.dist = get_package(session)
     return session.package
 
 
@@ -27,12 +30,62 @@ def get_package(session):
     if config.skipsdist:
         report.info("skipping sdist step")
         return None
+    lock_file = str(
+        session.config.toxworkdir.join("{}.lock".format(session.config.isolated_build_env))
+    )
+    lock = FileLock(lock_file)
+    try:
+        try:
+            lock.acquire(0.0001)
+        except Timeout:
+            report.verbosity0("lock file {} present, will block until released".format(lock_file))
+            lock.acquire()
+        package = acquire_package(config, report, session)
+        session_package = create_session_view(package, config.temp_dir, report)
+        return session_package, package
+    finally:
+        lock.release(force=True)
+
+
+def create_session_view(package, temp_dir, report):
+    """once we build a package we cannot return that directly, as a subsequent call
+    might delete that package (in order to do its own build); therefore we need to
+    return a view of the file that it's not prone to deletion and can be removed when the
+    session ends
+    """
+    if not package:
+        return package
+    temp_dir.ensure(dir=True)
+
+    # we'll prefix it with a unique number, note adding as suffix can cause conflicts
+    # with tools that check the files extension (e.g. pip)
+    exists = [
+        i.basename[: -len(package.basename) - 1]
+        for i in temp_dir.listdir(fil="*-{}".format(package.basename))
+    ]
+    file_id = max(chain((0,), (int(i) for i in exists if six.text_type(i).isnumeric())))
+    session_package = temp_dir.join("{}-{}".format(file_id + 1, package.basename))
+
+    # if we can do hard links do that, otherwise just copy
+    operation = "links"
+    if hasattr(os, "link"):
+        os.link(str(package), str(session_package))
+    else:
+        operation = "copied"
+        package.copy(session_package)
+    common = session_package.common(package)
+    report.verbosity1(
+        "package {} {} to {} ({})".format(
+            common.bestrelpath(session_package), operation, common.bestrelpath(package), common
+        )
+    )
+    return session_package
+
+
+def acquire_package(config, report, session):
+    """acquire a source distribution (either by loading a local file or triggering a build)"""
     if not config.option.sdistonly and (config.sdistsrc or config.option.installpkg):
-        path = config.option.installpkg
-        if not path:
-            path = config.sdistsrc
-        path = session._resolve_package(path)
-        report.info("using package {!r}, skipping 'sdist' activity ".format(str(path)))
+        path = get_local_package(config, report, session)
     else:
         try:
             path = build_package(config, report, session)
@@ -51,6 +104,15 @@ def get_package(session):
     return path
 
 
+def get_local_package(config, report, session):
+    path = config.option.installpkg
+    if not path:
+        path = config.sdistsrc
+    path = session._resolve_package(path)
+    report.info("using package {!r}, skipping 'sdist' activity ".format(str(path)))
+    return path
+
+
 def build_package(config, report, session):
     if not config.isolated_build:
         return make_sdist_legacy(report, config, session)