From 069b110c866ecfd652a041d735ca6d261b8d41ba Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Mon, 2 Dec 2019 11:02:23 +0000 Subject: lint: Remove unnecessary list comprehensions Newer version of pylint detect when a comprehension would not be needed. Let's remove all the ones that are indeed extraneous --- src/buildstream/_profile.py | 2 +- src/buildstream/_stream.py | 2 +- src/buildstream/element.py | 2 +- src/buildstream/source.py | 2 +- tests/frontend/workspace.py | 2 +- tests/remoteexecution/workspace.py | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/buildstream/_profile.py b/src/buildstream/_profile.py index fdde04ab7..b3182b630 100644 --- a/src/buildstream/_profile.py +++ b/src/buildstream/_profile.py @@ -118,7 +118,7 @@ class _Profiler: self._valid_topics = False if settings: - self.enabled_topics = {topic for topic in settings.split(":")} + self.enabled_topics = set(settings.split(":")) @contextlib.contextmanager def profile(self, topic, key, message=None): diff --git a/src/buildstream/_stream.py b/src/buildstream/_stream.py index 4b8409323..20e073110 100644 --- a/src/buildstream/_stream.py +++ b/src/buildstream/_stream.py @@ -678,7 +678,7 @@ class Stream: continue if isinstance(obj, ArtifactElement): obj.name = ref - files = [f for f in obj._walk_artifact_files()] + files = list(obj._walk_artifact_files()) elements_to_files[obj.name] = files return elements_to_files diff --git a/src/buildstream/element.py b/src/buildstream/element.py index ffce257bf..7b96e8a8b 100644 --- a/src/buildstream/element.py +++ b/src/buildstream/element.py @@ -1965,7 +1965,7 @@ class Element(Plugin): sandbox._set_mount_source(mount.path, mount.host_path) if command: - argv = [arg for arg in command] + argv = command else: argv = shell_command diff --git a/src/buildstream/source.py b/src/buildstream/source.py index dbe113409..f49cdb493 100644 --- a/src/buildstream/source.py +++ b/src/buildstream/source.py @@ -1004,7 +1004,7 @@ class Source(Plugin): # If we're synthesising missing list entries, we know we're # doing this for project.refs so synthesise empty dicts for the # intervening entries too - lpath = [step for step in path] + lpath = path.copy() lpath.append("") # We know the last step will be a string key for step, next_step in zip(lpath, lpath[1:]): if type(step) is str: # pylint: disable=unidiomatic-typecheck diff --git a/tests/frontend/workspace.py b/tests/frontend/workspace.py index 80db8cc98..eabaca68c 100644 --- a/tests/frontend/workspace.py +++ b/tests/frontend/workspace.py @@ -42,7 +42,7 @@ from buildstream._workspaces import BST_WORKSPACE_FORMAT_VERSION from tests.testutils import create_artifact_share, create_element_size, wait_for_cache_granularity -repo_kinds = [(kind) for kind in ALL_REPO_KINDS] +repo_kinds = ALL_REPO_KINDS # Project directory DATA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), "project",) diff --git a/tests/remoteexecution/workspace.py b/tests/remoteexecution/workspace.py index 4ac743490..83480b42e 100644 --- a/tests/remoteexecution/workspace.py +++ b/tests/remoteexecution/workspace.py @@ -91,7 +91,7 @@ def _get_mtimes(root): def get_mtimes(root): - return {k: v for (k, v) in set(_get_mtimes(root))} + return dict(set(_get_mtimes(root))) def check_buildtree( -- cgit v1.2.1 From 1064374cf19ef5492c3b6b4a23ae14c49ac8ea86 Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Mon, 2 Dec 2019 11:12:42 +0000 Subject: lint: remove all unecessary elif/else after break/continue Newer pylint versions detect and complain about unnecessary elif/else after a continue/break/return clause. Let's remove them --- src/buildstream/_artifactcache.py | 4 ++-- src/buildstream/_basecache.py | 4 ++-- src/buildstream/_stream.py | 2 +- src/buildstream/plugins/sources/tar.py | 2 +- src/buildstream/plugins/sources/zip.py | 2 +- src/buildstream/sandbox/_sandboxbwrap.py | 18 +++++++++--------- src/buildstream/storage/_casbaseddirectory.py | 4 ++-- src/buildstream/utils.py | 6 +++--- 8 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/buildstream/_artifactcache.py b/src/buildstream/_artifactcache.py index 03c47b906..10ccf1527 100644 --- a/src/buildstream/_artifactcache.py +++ b/src/buildstream/_artifactcache.py @@ -311,8 +311,8 @@ class ArtifactCache(BaseCache): if artifact: element.info("Pulled artifact {} <- {}".format(display_key, remote)) break - else: - element.info("Remote ({}) does not have artifact {} cached".format(remote, display_key)) + + element.info("Remote ({}) does not have artifact {} cached".format(remote, display_key)) except CASError as e: element.warn("Could not pull from remote {}: {}".format(remote, e)) errors.append(e) diff --git a/src/buildstream/_basecache.py b/src/buildstream/_basecache.py index 516119cd1..15b1d5389 100644 --- a/src/buildstream/_basecache.py +++ b/src/buildstream/_basecache.py @@ -315,8 +315,8 @@ class BaseCache: if on_failure: on_failure(remote_spec, str(err)) continue - else: - raise + + raise # Finally, we can instantiate the remote. Note that # NamedTuples are hashable, so we can use them as pretty diff --git a/src/buildstream/_stream.py b/src/buildstream/_stream.py index 20e073110..c2945a2f6 100644 --- a/src/buildstream/_stream.py +++ b/src/buildstream/_stream.py @@ -647,7 +647,7 @@ class Stream: if not obj._cached(): self._message(MessageType.WARN, "{} is not cached".format(ref)) continue - elif not obj._cached_logs(): + if not obj._cached_logs(): self._message(MessageType.WARN, "{} is cached without log files".format(ref)) continue diff --git a/src/buildstream/plugins/sources/tar.py b/src/buildstream/plugins/sources/tar.py index e35b52f85..fc4f5736a 100644 --- a/src/buildstream/plugins/sources/tar.py +++ b/src/buildstream/plugins/sources/tar.py @@ -235,7 +235,7 @@ class TarSource(DownloadableFileSource): # Avoid considering the '.' directory, if any is included in the archive # this is to avoid the default 'base-dir: *' value behaving differently # depending on whether the tarball was encoded with a leading '.' or not - elif member_name == ".": + if member_name == ".": continue yield member_name diff --git a/src/buildstream/plugins/sources/zip.py b/src/buildstream/plugins/sources/zip.py index 47933c8eb..47823dfde 100644 --- a/src/buildstream/plugins/sources/zip.py +++ b/src/buildstream/plugins/sources/zip.py @@ -162,7 +162,7 @@ class ZipSource(DownloadableFileSource): # Avoid considering the '.' directory, if any is included in the archive # this is to avoid the default 'base-dir: *' value behaving differently # depending on whether the archive was encoded with a leading '.' or not - elif member.filename == "." or member.filename == "./": + if member.filename == "." or member.filename == "./": continue yield member.filename diff --git a/src/buildstream/sandbox/_sandboxbwrap.py b/src/buildstream/sandbox/_sandboxbwrap.py index 1405611bc..433b0f754 100644 --- a/src/buildstream/sandbox/_sandboxbwrap.py +++ b/src/buildstream/sandbox/_sandboxbwrap.py @@ -493,17 +493,17 @@ class SandboxBwrap(Sandbox): tries += 1 time.sleep(1 / 100) continue - else: - # We've reached the upper limit of tries, bail out now - # because something must have went wrong - # - raise - elif e.errno == errno.ENOENT: + + # We've reached the upper limit of tries, bail out now + # because something must have went wrong + # + raise + if e.errno == errno.ENOENT: # Bubblewrap cleaned it up for us, no problem if we cant remove it break - else: - # Something unexpected, reraise this error - raise + + # Something unexpected, reraise this error + raise else: # Successfully removed the symlink break diff --git a/src/buildstream/storage/_casbaseddirectory.py b/src/buildstream/storage/_casbaseddirectory.py index df28dc591..f90d5c503 100644 --- a/src/buildstream/storage/_casbaseddirectory.py +++ b/src/buildstream/storage/_casbaseddirectory.py @@ -221,12 +221,12 @@ class CasBasedDirectory(Directory): else: if path == ".": continue - elif path == "..": + if path == "..": if current_dir.parent is not None: current_dir = current_dir.parent # In POSIX /.. == / so just stay at the root dir continue - elif create: + if create: current_dir = current_dir._add_directory(path) else: error = "'{}' not found in {}" diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py index 7f7bf67b2..0307b8470 100644 --- a/src/buildstream/utils.py +++ b/src/buildstream/utils.py @@ -765,7 +765,7 @@ def _pretty_size(size, dec_places=0): for unit in units: if psize < 1024: break - elif unit != units[-1]: + if unit != units[-1]: psize /= 1024 return "{size:g}{unit}".format(size=round(psize, dec_places), unit=unit) @@ -914,8 +914,8 @@ def _process_list( # Skip this missing file if ignore_missing: continue - else: - raise UtilError("Source file is missing: {}".format(srcpath)) from e + + raise UtilError("Source file is missing: {}".format(srcpath)) from e if stat.S_ISDIR(mode): # Ensure directory exists in destination -- cgit v1.2.1 From e4ed02fbfcea355d6432e7a2e389af84a94120dd Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Mon, 2 Dec 2019 11:34:31 +0000 Subject: tests: Use pytest.raise() instead of checking for return code This gives a potentially more explicit understanding of what went wrong, and pytest can give better information about that exception than just us asserting the return code. --- tests/external_plugins.py | 5 +++-- tests/sources/git.py | 8 ++++---- tests/testutils/python_repo.py | 3 +-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/external_plugins.py b/tests/external_plugins.py index 2123b846b..9a5e04501 100644 --- a/tests/external_plugins.py +++ b/tests/external_plugins.py @@ -31,12 +31,13 @@ class ExternalPluginRepo: def clone(self, location): self._clone_location = os.path.join(location, self.name) subprocess.run( - ["git", "clone", "--single-branch", "--branch", self.ref, "--depth", "1", self.url, self._clone_location,] + ["git", "clone", "--single-branch", "--branch", self.ref, "--depth", "1", self.url, self._clone_location,], + check=True, ) return self._clone_location def install(self): - subprocess.run(["pip3", "install", self._clone_location]) + subprocess.run(["pip3", "install", self._clone_location], check=True) def test(self, args): test_files = self._match_test_patterns() diff --git a/tests/sources/git.py b/tests/sources/git.py index fbb1394ec..e4252fe51 100644 --- a/tests/sources/git.py +++ b/tests/sources/git.py @@ -779,8 +779,8 @@ def test_git_describe(cli, tmpdir, datafiles, ref_storage, tag_type): tags = set(tags.splitlines()) assert tags == set(["tag1", "tag2"]) - p = subprocess.run(["git", "log", repo.rev_parse("uselesstag")], cwd=checkout) - assert p.returncode != 0 + with pytest.raises(subprocess.CalledProcessError): + subprocess.run(["git", "log", repo.rev_parse("uselesstag")], cwd=checkout, check=True) @pytest.mark.skipif(HAVE_GIT is False, reason="git is not available") @@ -888,8 +888,8 @@ def test_git_describe_head_is_tagged(cli, tmpdir, datafiles, ref_storage, tag_ty assert set(rev_list.splitlines()) == set([tagged_ref]) - p = subprocess.run(["git", "log", repo.rev_parse("uselesstag")], cwd=checkout) - assert p.returncode != 0 + with pytest.raises(subprocess.CalledProcessError): + subprocess.run(["git", "log", repo.rev_parse("uselesstag")], cwd=checkout, check=True) @pytest.mark.skipif(HAVE_GIT is False, reason="git is not available") diff --git a/tests/testutils/python_repo.py b/tests/testutils/python_repo.py index 7d9ae4e47..07efa376a 100644 --- a/tests/testutils/python_repo.py +++ b/tests/testutils/python_repo.py @@ -88,8 +88,7 @@ def generate_pip_package(tmpdir, pypi, name, version="0.1", dependencies=None): os.chmod(main_file, 0o644) # Run sdist with a fresh process - p = subprocess.run([sys.executable, "setup.py", "sdist"], cwd=tmpdir) - assert p.returncode == 0 + subprocess.run([sys.executable, "setup.py", "sdist"], cwd=tmpdir, check=True) # create directory for this package in pypi resulting in a directory # tree resembling the following structure: -- cgit v1.2.1 From ff6f6d0c38218d954814847f2a25b6d056347691 Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Mon, 2 Dec 2019 11:35:32 +0000 Subject: Update all python dependencies This updates all dependencies on the project, which is mainly needed by python3.8 but can be done independentely. This also disables multiple false positive lint errors and disable a new check that we don't need. --- .pylintrc | 2 ++ requirements/cov-requirements.txt | 21 ++++++++++----------- requirements/dev-requirements.txt | 27 +++++++++++++-------------- requirements/plugin-requirements.txt | 1 + requirements/requirements.txt | 16 ++++++++-------- src/buildstream/testing/_utils/junction.py | 2 +- tests/testutils/repo/git.py | 2 +- 7 files changed, 36 insertions(+), 35 deletions(-) diff --git a/.pylintrc b/.pylintrc index 422406417..25d5647b0 100644 --- a/.pylintrc +++ b/.pylintrc @@ -106,6 +106,8 @@ disable=##################################### ####################################################### # Messages that we would like to enable at some point # ####################################################### + # We have many circular imports that need breaking + import-outside-toplevel, duplicate-code, diff --git a/requirements/cov-requirements.txt b/requirements/cov-requirements.txt index bf2b5112a..c700ababf 100644 --- a/requirements/cov-requirements.txt +++ b/requirements/cov-requirements.txt @@ -1,16 +1,15 @@ coverage==4.4 -pytest-cov==2.7.1 -Cython==0.29.13 +pytest-cov==2.8.1 +Cython==0.29.14 ## The following requirements were added by pip freeze: -atomicwrites==1.3.0 -attrs==19.1.0 -importlib-metadata==0.20 -more-itertools==7.2.0 -packaging==19.1 -pluggy==0.12.0 +attrs==19.3.0 +importlib-metadata==1.1.0 +more-itertools==8.0.0 +packaging==19.2 +pluggy==0.13.1 py==1.8.0 -pyparsing==2.4.2 -pytest==5.1.2 -six==1.12.0 +pyparsing==2.4.5 +pytest==5.3.1 +six==1.13.0 wcwidth==0.1.7 zipp==0.6.0 diff --git a/requirements/dev-requirements.txt b/requirements/dev-requirements.txt index bd8449978..a9c265c85 100644 --- a/requirements/dev-requirements.txt +++ b/requirements/dev-requirements.txt @@ -1,27 +1,26 @@ -pylint==2.3.1 -pytest==5.1.2 +pylint==2.4.4 +pytest==5.3.1 pytest-datafiles==2.0 pytest-env==0.6.2 -pytest-xdist==1.29.0 +pytest-xdist==1.30.0 pytest-timeout==1.3.3 pyftpdlib==1.5.5 ## The following requirements were added by pip freeze: apipkg==1.5 -astroid==2.2.5 -atomicwrites==1.3.0 -attrs==19.1.0 +astroid==2.3.3 +attrs==19.3.0 execnet==1.7.1 -importlib-metadata==0.20 +importlib-metadata==1.1.0 isort==4.3.21 -lazy-object-proxy==1.4.2 +lazy-object-proxy==1.4.3 mccabe==0.6.1 -more-itertools==7.2.0 -packaging==19.1 -pluggy==0.12.0 +more-itertools==8.0.0 +packaging==19.2 +pluggy==0.13.1 py==1.8.0 -pyparsing==2.4.2 -pytest-forked==1.0.2 -six==1.12.0 +pyparsing==2.4.5 +pytest-forked==1.1.3 +six==1.13.0 typed-ast==1.4.0 wcwidth==0.1.7 wrapt==1.11.2 diff --git a/requirements/plugin-requirements.txt b/requirements/plugin-requirements.txt index ac984746d..950062f73 100644 --- a/requirements/plugin-requirements.txt +++ b/requirements/plugin-requirements.txt @@ -1 +1,2 @@ arpy==1.1.1 +## The following requirements were added by pip freeze: diff --git a/requirements/requirements.txt b/requirements/requirements.txt index 7d3d5d6c7..962090823 100644 --- a/requirements/requirements.txt +++ b/requirements/requirements.txt @@ -1,14 +1,14 @@ Click==7.0 -grpcio==1.23.0 -Jinja2==2.10.1 +grpcio==1.25.0 +Jinja2==2.10.3 pluginbase==1.0.0 -protobuf==3.9.1 -psutil==5.6.3 +protobuf==3.11.0 +psutil==5.6.7 ruamel.yaml==0.16.5 -setuptools==40.8.0 -pyroaring==0.2.8 +setuptools==39.0.1 +pyroaring==0.2.9 ujson==1.35 ## The following requirements were added by pip freeze: MarkupSafe==1.1.1 -ruamel.yaml.clib==0.1.2 -six==1.12.0 +ruamel.yaml.clib==0.2.0 +six==1.13.0 diff --git a/src/buildstream/testing/_utils/junction.py b/src/buildstream/testing/_utils/junction.py index cfc5898a9..e4dbd5984 100644 --- a/src/buildstream/testing/_utils/junction.py +++ b/src/buildstream/testing/_utils/junction.py @@ -70,4 +70,4 @@ class _SimpleGit(Repo): kwargs["env"] = dict(GIT_ENV, PWD=self.repo) kwargs.setdefault("cwd", self.repo) kwargs.setdefault("check", True) - return subprocess.run(argv, **kwargs) + return subprocess.run(argv, **kwargs) # pylint: disable=subprocess-run-check diff --git a/tests/testutils/repo/git.py b/tests/testutils/repo/git.py index b9360e9cd..1deca3ff7 100644 --- a/tests/testutils/repo/git.py +++ b/tests/testutils/repo/git.py @@ -27,7 +27,7 @@ class Git(Repo): kwargs["env"] = dict(self.env, PWD=self.repo) kwargs.setdefault("cwd", self.repo) kwargs.setdefault("check", True) - return subprocess.run(argv, **kwargs) + return subprocess.run(argv, **kwargs) # pylint: disable=subprocess-run-check def create(self, directory): self.copy_directory(directory, self.repo) -- cgit v1.2.1