diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index dd95dcba0175..2c51d7d6562c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -52,7 +52,7 @@ jobs: permissions: {} runs-on: ubuntu-latest container: - image: ghcr.io/cockpit-project/unit-tests + image: quay.io/cockpit/tasks:latest options: --user root steps: - name: Checkout website repository diff --git a/.github/workflows/unit-tests-refresh.yml b/.github/workflows/unit-tests-refresh.yml deleted file mode 100644 index 37f69c1a361d..000000000000 --- a/.github/workflows/unit-tests-refresh.yml +++ /dev/null @@ -1,57 +0,0 @@ -name: unit-test-refresh -on: - schedule: - # auto-refresh every Sunday evening - - cron: '0 22 * * 0' - # can be run manually on https://github.com/cockpit-project/cockpit/actions - workflow_dispatch: -jobs: - # we do both builds and all tests in a single run, so that we only upload the containers on success - refresh: - runs-on: ubuntu-22.04 - permissions: - contents: read - packages: write - timeout-minutes: 60 - steps: - - name: Clone repository - uses: actions/checkout@v4 - with: - # need this to also fetch tags - fetch-depth: 0 - - - name: Build fresh containers - timeout-minutes: 10 - run: containers/unit-tests/build - - - name: Run amd64 gcc check-memory test - timeout-minutes: 20 - run: containers/unit-tests/start --verbose --env=CC=gcc --image-tag=latest --make check-memory - - - name: Run amd64 clang distcheck test - timeout-minutes: 15 - run: containers/unit-tests/start --verbose --env=CC=clang --image-tag=latest --make distcheck - - - name: Run i386 gcc distcheck test - timeout-minutes: 15 - run: containers/unit-tests/start --verbose --env=CC=clang --image-tag=i386 --make distcheck - - - name: Run amd64 gcc distcheck test for C bridge - timeout-minutes: 15 - run: containers/unit-tests/start --verbose --env=CC=gcc --env=EXTRA_DISTCHECK_CONFIGURE_FLAGS=--enable-old-bridge --image-tag=latest --make distcheck - - - name: Run amd64 gcc check test - timeout-minutes: 15 - run: containers/unit-tests/start --verbose --env=CC=gcc --image-tag=latest --make check - - - name: Run pytest-cov test - timeout-minutes: 15 - run: containers/unit-tests/start --verbose --env=CC=gcc --image-tag=latest --make pytest-cov - - - name: Log into container registry - run: podman login -u ${{ github.actor }} -p ${{ secrets.GITHUB_TOKEN }} ghcr.io - - - name: Push containers to registry - run: | - podman push ghcr.io/cockpit-project/unit-tests:latest - podman push ghcr.io/cockpit-project/unit-tests:i386 diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 335f4f62b4ed..470159c908ab 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -4,48 +4,33 @@ jobs: unit-tests: runs-on: ubuntu-22.04 permissions: {} + container: + image: quay.io/cockpit/tasks:latest + options: --user 1001 strategy: matrix: - startarg: - # avoid check-memory on i386, it has literally thousands of uninteresting/wrong errors - - { make: 'check-memory', cc: 'gcc', tag: 'latest' } - # with default Python bridge - - { make: 'distcheck', cc: 'clang', tag: 'latest' } - - { make: 'distcheck', cc: 'gcc', tag: 'i386' } - # with old C bridge - - { make: 'distcheck', cc: 'gcc', distcheck_flags: '--enable-old-bridge', tag: 'latest' } + target: + - check-memory + - distcheck # this runs static code checks, unlike distcheck - - { make: 'check', cc: 'gcc', tag: 'latest' } - - { make: 'pytest-cov', cc: 'gcc', tag: 'latest' } + - check + - pytest-cov fail-fast: false timeout-minutes: 60 + env: + FORCE_COLOR: 1 + TEST_BROWSER: firefox + CFLAGS: '-O2' steps: - name: Clone repository uses: actions/checkout@v4 with: # need this to also fetch tags fetch-depth: 0 + submodules: true - - name: Build unit test container if it changed - run: | - changes=$(git diff --name-only origin/${{ github.event.pull_request.base.ref }}..HEAD -- containers/unit-tests/) - if [ -n "${changes}" ]; then - case '${{ matrix.startarg.tag }}' in - i386) arch=i386;; - latest) arch=amd64;; - esac - containers/unit-tests/build $arch - fi - - - name: Run unit-tests container + - name: Run unit test timeout-minutes: 30 - # HACK: -gdwarf-4 is for clang: https://bugs.kde.org/show_bug.cgi?id=452758 run: | - containers/unit-tests/start \ - --verbose \ - --env=FORCE_COLOR=1 \ - --env=CC='${{ matrix.startarg.cc }}' \ - --env=CFLAGS='-O2 -gdwarf-4' \ - --env=EXTRA_DISTCHECK_CONFIGURE_FLAGS='${{ matrix.startarg.distcheck_flags }}' \ - --image-tag='${{ matrix.startarg.tag }}' \ - --make '${{ matrix.startarg.make }}' + ./autogen.sh + make -j$(nproc) '${{ matrix.target }}' diff --git a/containers/unit-tests/Dockerfile b/containers/unit-tests/Dockerfile deleted file mode 100644 index 19d8200b8a8c..000000000000 --- a/containers/unit-tests/Dockerfile +++ /dev/null @@ -1,26 +0,0 @@ -ARG debian_arch=amd64 -FROM docker.io/${debian_arch}/debian:testing - -ARG personality=linux64 -ENV personality=${personality} - -COPY setup.sh / -RUN ${personality} /setup.sh ${personality} && rm -rf /setup.sh - -# 'builder' user created in setup.sh -USER builder -WORKDIR /home/builder - -ENV LANG=C.UTF-8 - -# HACK: unbreak distcheck on Debian: https://bugs.debian.org/1035546 -ENV DEB_PYTHON_INSTALL_LAYOUT=deb - -VOLUME /source - -COPY entrypoint / -ENTRYPOINT ["/entrypoint"] -CMD ["/bin/bash"] - -# for filtering from our 'exec' script -LABEL org.cockpit-project.container=unit-tests diff --git a/containers/unit-tests/README.md b/containers/unit-tests/README.md deleted file mode 100644 index 743559397028..000000000000 --- a/containers/unit-tests/README.md +++ /dev/null @@ -1,76 +0,0 @@ -# Cockpit unit test container - -This container has all build dependencies and toolchains (GCC and clang) that we -want to exercise Cockpit with, mostly for `make distcheck` and `make check-memory`. -This container runs on [GitHub](.github/workflows/unit-tests.yml), but can be easily -run locally too. - -It assumes that the Cockpit source git checkout is available in `/source`. It -will not modify that directory or take uncommitted changes into account, but it -will re-use an already existing `node_modules/` directory. - -The scripts can use either podman (preferred) or docker. If you use docker, you -need to run all commands as root. With podman the containers work as either user -or root. - -## Building - -The `build` script will build the `cockpit/unit-tests` and -`cockpit/unit-tests:i386` containers. Call it with an architecture to only -build one variant, e.g. `build i386`. - -## Running tests - -You need to disable SELinux with `sudo setenforce 0` for this. There is no -other way for the container to access the files in your build tree (do *not* -use the `--volume` `:Z` option, as that will destroy the file labels on the -host). - -Tests in that container get started with the `start` script. By default, this -script runs the unit tests on amd64. The script accepts a number of arguments -to modify its behaviour: - - - `--env CC=othercc` to set the `CC` environment variable inside the container (ie: - to build with a different compiler) - - `--image-tag` to specify a different tag to use for the `cockpit/unit-tests` image - (eg: `--image-tag=i386`) - -Additionally, a testing scenario can be provided with specifying a `make` target. -Supported scenarios are: - - - `check-memory`: runs 'make check-memory' (ie: run the unit tests under valgrind) - - `distcheck`: runs 'make distcheck' and some related checks - - `pycheck`: runs browser unit tests against the Python bridge - -Some examples: - - $ ./start --make check-memory # run the valgrind tests on amd64 - - $ ./start --env=CC=clang --make check-memory # run the valgrind tests, compiled with clang - - $ ./start --image-tag=i386 distcheck # run the distcheck tests on i386 - -## Debugging tests - -For interactive debugging, run a shell in the container: - - $ ./start - -You will find the cockpit source tree (from the host) mounted at `/source` in -the container. Run - - $ /source/autogen.sh - -to create a build tree, then you can run any make or other debugging command -interactively. - -You can also attach to another container using the provided `exec` script. For example: - - $ ./exec uname -a # run a command as the "builder" user - - $ ./exec --root # start a shell as root - -## More Info - - * [Cockpit Project](https://cockpit-project.org) - * [Cockpit Development](https://github.com/cockpit-project/cockpit) diff --git a/containers/unit-tests/build b/containers/unit-tests/build deleted file mode 100755 index 03359eb27d06..000000000000 --- a/containers/unit-tests/build +++ /dev/null @@ -1,12 +0,0 @@ -#!/bin/sh -set -eu - -dir=$(dirname "$0") - -if [ -z "${1:-}" ] || [ "${1:-}" = amd64 ]; then - podman build --build-arg debian_arch=amd64 --build-arg personality=linux64 -t ghcr.io/cockpit-project/unit-tests ${dir} -fi - -if [ -z "${1:-}" ] || [ "${1:-}" = i386 ]; then - podman build --build-arg debian_arch=i386 --build-arg personality=linux32 -t ghcr.io/cockpit-project/unit-tests:i386 ${dir} -fi diff --git a/containers/unit-tests/entrypoint b/containers/unit-tests/entrypoint deleted file mode 100755 index 8a53d1074499..000000000000 --- a/containers/unit-tests/entrypoint +++ /dev/null @@ -1,12 +0,0 @@ -#!/bin/sh -e - -export TEST_BROWSER=firefox - -printf "Host: " && uname -srvm - -. /usr/lib/os-release -printf "Container: \${NAME} \${VERSION} / " && ${personality} uname -nrvm -echo - -set -ex -exec ${personality} -- "$@" diff --git a/containers/unit-tests/setup.sh b/containers/unit-tests/setup.sh deleted file mode 100755 index 24764675438d..000000000000 --- a/containers/unit-tests/setup.sh +++ /dev/null @@ -1,79 +0,0 @@ -#!/bin/sh -ex - -dependencies="\ - appstream-util \ - autoconf \ - automake \ - build-essential \ - clang \ - curl \ - dbus \ - firefox-esr \ - flake8 \ - gcc-multilib \ - gdb \ - git \ - glib-networking \ - glib-networking-dbgsym\ - gtk-doc-tools \ - gettext \ - libc6-dbg \ - libfontconfig1 \ - libglib2.0-0-dbgsym \ - libglib2.0-dev \ - libgnutls28-dev \ - libjavascript-minifier-xs-perl \ - libjson-glib-dev \ - libjson-perl \ - libkrb5-dev \ - libpam0g-dev \ - libpcp-import1-dev \ - libpcp-pmda3-dev \ - libpcp3-dev \ - libpolkit-agent-1-dev \ - libpolkit-gobject-1-dev \ - libssh-4-dbgsym \ - libssh-dev \ - libsystemd-dev \ - mypy \ - npm \ - nodejs \ - pkg-config \ - python3 \ - python3-mypy \ - python3-pip \ - python3-pytest-asyncio \ - python3-pytest-cov \ - python3-pytest-timeout \ - ssh \ - strace \ - valgrind \ - vulture \ - xmlto \ - xsltproc \ -" - -echo "deb http://deb.debian.org/debian-debug/ testing-debug main" > /etc/apt/sources.list.d/ddebs.list -echo "deb http://deb.debian.org/debian-debug/ testing-proposed-updates-debug main" >> /etc/apt/sources.list.d/ddebs.list -apt-get update -apt-get install -y --no-install-recommends eatmydata -DEBIAN_FRONTEND=noninteractive eatmydata apt-get install -y --no-install-recommends ${dependencies} - -# https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1057968 -echo "deb http://deb.debian.org/debian unstable main" > /etc/apt/sources.list.d/unstable.list -apt-get update -apt-get install -y --no-install-recommends python3-flake8 -rm /etc/apt/sources.list.d/unstable.list - -adduser --gecos "Builder" builder - -if [ "$(uname -m)" = "x86_64" ] ; then - # See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1030835 - pip install --break-system-packages ruff -fi - -# minimize image -# useful command: dpkg-query --show -f '${package} ${installed-size}\n' | sort -k2n -dpkg -P --force-depends libgl1-mesa-dri libglx-mesa0 perl - -rm -rf /var/cache/apt /var/lib/apt /var/log/* /usr/share/doc/ /usr/share/man/ /usr/share/help /usr/share/info diff --git a/containers/unit-tests/start b/containers/unit-tests/start deleted file mode 100755 index e01cb66370ad..000000000000 --- a/containers/unit-tests/start +++ /dev/null @@ -1,190 +0,0 @@ -#!/usr/bin/python3 - -# This file is part of Cockpit. -# -# Copyright (C) 2022 Red Hat, Inc. -# -# This program is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program. If not, see . - -import argparse -import os -import shlex -import sys -import tempfile -from subprocess import run - - -def logged(func): - def wrapper(args, **kwargs): - print('+', shlex.join(args)) - return func(args, **kwargs) - return wrapper - - -def git(*args): - run(['git', *args], check=True) - - -def git_output(*args): - return run(['git', *args], check=True, capture_output=True, text=True).stdout.strip() - - -def podman(*args, check=True): - if os.path.exists('/run/.toolboxenv'): - cmd = ['flatpak-spawn', '--host', 'podman', *args] - else: - cmd = ['podman', *args] - - return run(cmd, check=check) - - -class PodmanTemporaryDirectory(tempfile.TemporaryDirectory): - """TemporaryDirectory subclass capable of removing files owned by subuids""" - @classmethod - def _rmtree(cls, name, ignore_errors=False): # noqa: FBT002 - del ignore_errors # can't remove or rename this kwarg - podman('unshare', 'rm', '-r', name) - - def __enter__(self): - # Override the TemporaryDirectory behaviour of returning its name here - return self - - -class SourceDirectory(PodmanTemporaryDirectory): - def __init__(self): - super().__init__(prefix='cockpit-source.') - - def prepare(self, args): - if args.branch: - opts = ['-c', 'advice.detachedHead=false', '-b', args.branch] - else: - opts = [] - - git('clone', '--recurse-submodule=vendor/*', *opts, '.', self.name) - - if not args.head and not args.branch: - if stash := git_output('stash', 'create'): - git('-C', self.name, 'fetch', '--quiet', '--no-write-fetch-head', 'origin', stash) - git('-C', self.name, 'stash', 'apply', stash) - - if not args.no_node_modules: - run([f'{self.name}/tools/node-modules', 'checkout'], check=True) - - -class ResultsDirectory(PodmanTemporaryDirectory): - def __init__(self): - super().__init__(prefix='cockpit-results.') - - def copy_out(self, destination): - podman('unshare', 'cp', '-rT', self.name, destination) - - -def main(): - parser = argparse.ArgumentParser() - parser.add_argument('--verbose', '-v', action='store_true', help='Show commands when running them') - parser.add_argument('--results', metavar='DIRECTORY', help="Copy container /results to the given host directory") - - group = parser.add_argument_group(title='Container options') - group.add_argument('--image', default='ghcr.io/cockpit-project/unit-tests', help='Container image to use') - group.add_argument('--image-tag', default='latest', help='Container image tag to use') - group.add_argument('--env', metavar='NAME=VAL', action='append', default=[], - help='Set an environment variable in the container') - group.add_argument('--network', action="store_true", - help="Enable network in the container (default: disabled)") - group.add_argument('--interactive', '-i', action="store_true", - help="Interactive mode (implied by no command or script)") - group.add_argument('--tty', '-t', action="store_true", - help="Allocate a pseudoterminal (implied by no command or script)") - group.add_argument('--user', help="Pass through the --user flag to podman") - group.add_argument('--entrypoint', metavar='CMD', help="Provide the --entrypoint flag to podman") - group.add_argument('--workdir', help="Provide the --workdir flag to podman") - - group = parser.add_argument_group(title='What to build').add_mutually_exclusive_group() - group.add_argument('--head', action='store_true', help='Build the HEAD commit') - group.add_argument('-b', dest='branch', metavar='NAME', help='Build the named branch or tag') - group.add_argument('--work-tree', action='store_true', - help='Build the HEAD commit, plus changes on the filesystem (default)') - - group = parser.add_argument_group(title='Preparation').add_mutually_exclusive_group() - group.add_argument('--no-node-modules', action='store_true', - help='Disable checking out node_modules/ during preparation') - - group = parser.add_argument_group(title='Command to run').add_mutually_exclusive_group() - group.add_argument('-c', metavar='SCRIPT', dest='script', help="Run the provided shell script") - group.add_argument('--make-dist', action='store_true', help='Run `make dist`. Requires --results.') - group.add_argument('--make', metavar='TARGET', help='Run `make` on the given target') - # re: default=[]: https://github.com/python/cpython/issues/86020 - group.add_argument('command', metavar='CMD', nargs='*', default=[], help="Run a normal command, with arguments") - - args = parser.parse_args() - - if args.results and os.path.exists(args.results): - parser.error(f'--results directory `{args.results}` already exists') - - if args.make_dist and not args.results: - parser.error('--make-dist requires --results directory') - - if args.verbose: - global run - run = logged(run) - - with SourceDirectory() as source_dir, ResultsDirectory() as results_dir: - options = { - '--rm', - '--log-driver=none', - f'--volume={source_dir.name}:/source:Z,U', - } - - if args.results: - options.add(f'--volume={results_dir.name}:/results:Z,U') - - if not args.network: - options.add('--network=none') - if args.user: - options.add(f'--user={args.user}') - if args.entrypoint: - options.add(f'--entrypoint={args.entrypoint}') - if args.workdir: - options.add(f'--workdir={args.workdir}') - if args.interactive: - options.add('--interactive') - if args.tty: - options.add('--tty') - for keyval in args.env: - options.add(f'--env={keyval}') - - command = [] - if args.command: - command = args.command - elif args.script: - command = ['sh', '-c', args.script] - elif args.make: - command = ['sh', '-c', '/source/autogen.sh; exec make -j$(nproc) ' + shlex.quote(args.make)] - elif args.make_dist: - command = ['sh', '-c', 'cp -t /results $(/source/tools/make-dist)'] - else: - options.update(['--tty', '--interactive']) - - source_dir.prepare(args) - - result = podman('run', *options, f'{args.image}:{args.image_tag}', *command) - - if result.returncode == 0 and args.results: - results_dir.copy_out(args.results) - - return result.returncode - - -if __name__ == '__main__': - sys.exit(main()) diff --git a/packit.yaml b/packit.yaml deleted file mode 100644 index 8f77cd7cab3c..000000000000 --- a/packit.yaml +++ /dev/null @@ -1,108 +0,0 @@ -upstream_project_url: https://github.com/cockpit-project/cockpit -specfile_path: cockpit.spec -actions: - post-upstream-clone: - # build patched spec - - tools/node-modules make_package_lock_json - - cp tools/cockpit.spec . - # packit will compute and set the version by itself - - tools/fix-spec ./cockpit.spec 0 - - create-archive: - - tools/make-dist - -srpm_build_deps: - - automake - - gcc - - gettext - - glib2-devel - - make - - nodejs - - npm - - systemd-devel -# use the nicely formatted release NEWS from our upstream release, instead of git shortlog -copy_upstream_release_description: true -jobs: - - job: tests - identifier: self - trigger: pull_request - targets: - - fedora-38 - - fedora-39 - - fedora-latest-aarch64 - - fedora-development - - centos-stream-8-x86_64 - - centos-stream-9-x86_64 - - centos-stream-9-aarch64 - - # current Fedora runs reverse dependency testing against https://copr.fedorainfracloud.org/coprs/g/cockpit/main-builds/ - - job: tests - identifier: revdeps - trigger: pull_request - targets: - - fedora-latest-stable - tf_extra_params: - environments: - - artifacts: - - type: repository-file - id: https://copr.fedorainfracloud.org/coprs/g/cockpit/main-builds/repo/fedora-$releasever/group_cockpit-main-builds-fedora-$releasever.repo - tmt: - context: - revdeps: "yes" - - # run build/unit tests on some interesting architectures - - job: copr_build - trigger: pull_request - targets: - # 32 bit - - fedora-development-i386 - # big-endian - - fedora-development-s390x - - # for cross-project testing - - job: copr_build - trigger: commit - branch: "^main$" - owner: "@cockpit" - project: "main-builds" - preserve_project: True - - - job: copr_build - trigger: release - owner: "@cockpit" - project: "cockpit-preview" - preserve_project: True - actions: - # same as the global one, but specifying actions: does not inherit - post-upstream-clone: - # build patched spec - - tools/node-modules make_package_lock_json - - cp tools/cockpit.spec . - # packit will compute and set the version by itself - - tools/fix-spec ./cockpit.spec 0 - # HACK: tarball for releases (copr_build, koji, etc.), copying spec's Source0; this - # really should be the default, see https://github.com/packit/packit-service/issues/1505 - create-archive: - - sh -exc "curl -L -O https://github.com/cockpit-project/cockpit/releases/download/${PACKIT_PROJECT_VERSION}/${PACKIT_PROJECT_NAME_VERSION}.tar.xz" - - sh -exc "ls ${PACKIT_PROJECT_NAME_VERSION}.tar.xz" - - - job: propose_downstream - trigger: release - dist_git_branches: - - fedora-development - - fedora-38 - - fedora-39 - - - job: koji_build - trigger: commit - dist_git_branches: - - fedora-development - - fedora-38 - - fedora-39 - - - job: bodhi_update - trigger: commit - dist_git_branches: - # rawhide updates are created automatically - - fedora-38 - - fedora-39 diff --git a/pkg/base1/test-spawn-proc.js b/pkg/base1/test-spawn-proc.js index 965c0e538405..f0d5b1ce466b 100644 --- a/pkg/base1/test-spawn-proc.js +++ b/pkg/base1/test-spawn-proc.js @@ -173,7 +173,11 @@ QUnit.test("pty", async assert => { }); QUnit.test("pty window size", async assert => { - const proc = cockpit.spawn(['tput', 'lines', 'cols'], { pty: true, window: { rows: 77, cols: 88 } }); + const proc = cockpit.spawn(['tput', 'lines', 'cols'], { + pty: true, + environ: ["TERM=vt100"], + window: { rows: 77, cols: 88 } + }); assert.equal(await proc, '77\r\n88\r\n', 'Correct rows and columns'); }); diff --git a/src/cockpit/channels/filesystem.py b/src/cockpit/channels/filesystem.py index 1403ecf30741..9df461b27b14 100644 --- a/src/cockpit/channels/filesystem.py +++ b/src/cockpit/channels/filesystem.py @@ -93,14 +93,12 @@ def do_open(self, options): try: scan_dir = os.scandir(path) + except FileNotFoundError as error: + raise ChannelError('not-found', message=str(error)) from error + except PermissionError as error: + raise ChannelError('access-denied', message=str(error)) from error except OSError as error: - if isinstance(error, FileNotFoundError): - problem = 'not-found' - elif isinstance(error, PermissionError): - problem = 'access-denied' - else: - problem = 'internal-error' - raise ChannelError(problem, message=str(error)) from error + raise ChannelError('internal-error', message=str(error)) from error self.ready() for entry in scan_dir: @@ -183,6 +181,9 @@ def do_data(self, data): continue except PermissionError as exc: raise ChannelError('access-denied') from exc + except FileNotFoundError as exc: + # directory of path does not exist + raise ChannelError('not-found') from exc except OSError as exc: raise ChannelError('internal-error', message=str(exc)) from exc else: @@ -214,10 +215,18 @@ def do_done(self): try: os.rename(self._temppath, self._path) - except OSError: - # ensure to not leave the temp file behind + # ensure to not leave the temp file behind + except FileNotFoundError as exc: self.unlink_temppath() - raise + raise ChannelError('not-found', message=str(exc)) from exc + except IsADirectoryError as exc: + self.unlink_temppath() + # not ideal, but the closest code we have + raise ChannelError('access-denied', message=str(exc)) from exc + except OSError as exc: + self.unlink_temppath() + raise ChannelError('internal-error', message=str(exc)) from exc + self._tempfile.close() self._tempfile = None diff --git a/test/pytest/test_bridge.py b/test/pytest/test_bridge.py index 8a34c5753821..3f03bf8327b0 100644 --- a/test/pytest/test_bridge.py +++ b/test/pytest/test_bridge.py @@ -10,7 +10,6 @@ import shlex import subprocess import sys -import tempfile import unittest.mock from collections import deque from pathlib import Path @@ -82,7 +81,7 @@ def transport(no_init_transport: MockTransport) -> MockTransport: @pytest.mark.asyncio -async def test_echo(transport): +async def test_echo(transport: MockTransport) -> None: echo = await transport.check_open('echo') transport.send_data(echo, b'foo') @@ -97,7 +96,7 @@ async def test_echo(transport): @pytest.mark.asyncio -async def test_host(transport): +async def test_host(transport: MockTransport) -> None: # try to open a null channel, explicitly naming our host await transport.check_open('null', host=MOCK_HOSTNAME) @@ -118,7 +117,7 @@ async def test_host(transport): @pytest.mark.asyncio -async def test_dbus_call_internal(bridge, transport): +async def test_dbus_call_internal(bridge: Bridge, transport: MockTransport) -> None: my_object = test_iface() bridge.internal_bus.export('/foo', my_object) assert my_object._dbus_bus == bridge.internal_bus.server @@ -133,7 +132,7 @@ async def test_dbus_call_internal(bridge, transport): @pytest.mark.asyncio -async def test_dbus_watch(bridge, transport): +async def test_dbus_watch(bridge: Bridge, transport: MockTransport) -> None: my_object = test_iface() bridge.internal_bus.export('/foo', my_object) assert my_object._dbus_bus == bridge.internal_bus.server @@ -161,18 +160,18 @@ async def test_dbus_watch(bridge, transport): assert notify['notify']['/foo'] == {'test.iface': {'Prop': 'xyz'}} -async def verify_root_bridge_not_running(bridge, transport): +async def verify_root_bridge_not_running(bridge: Bridge, transport: MockTransport) -> None: assert bridge.superuser_rule.peer is None await transport.assert_bus_props('/superuser', 'cockpit.Superuser', - {'Bridges': bridge.more_superuser_bridges, 'Current': 'none'}) + {'Bridges': bridge.more_superuser_bridges, 'Current': 'none'}) # type: ignore[attr-defined] null = await transport.check_open('null', superuser=True, problem='access-denied') assert null not in bridge.open_channels @pytest.mark.asyncio -async def verify_root_bridge_running(bridge, transport): +async def verify_root_bridge_running(bridge: Bridge, transport: MockTransport) -> None: await transport.assert_bus_props('/superuser', 'cockpit.Superuser', - {'Bridges': bridge.more_superuser_bridges, 'Current': 'pseudo'}) + {'Bridges': bridge.more_superuser_bridges, 'Current': 'pseudo'}) # type: ignore[attr-defined] assert bridge.superuser_rule.peer is not None # try to open dbus on the root bridge @@ -187,7 +186,7 @@ async def verify_root_bridge_running(bridge, transport): @pytest.mark.asyncio -async def test_superuser_dbus(bridge, transport): +async def test_superuser_dbus(bridge: Bridge, transport: MockTransport) -> None: add_pseudo(bridge) await verify_root_bridge_not_running(bridge, transport) @@ -216,7 +215,7 @@ def format_methods(methods: Dict[str, str]): @pytest.mark.asyncio -async def test_superuser_dbus_pw(bridge, transport, monkeypatch): +async def test_superuser_dbus_pw(bridge: Bridge, transport: MockTransport, monkeypatch) -> None: monkeypatch.setenv('PSEUDO_PASSWORD', 'p4ssw0rd') add_pseudo(bridge) await verify_root_bridge_not_running(bridge, transport) @@ -225,7 +224,7 @@ async def test_superuser_dbus_pw(bridge, transport, monkeypatch): await transport.add_bus_match('/superuser', 'cockpit.Superuser') await transport.watch_bus('/superuser', 'cockpit.Superuser', { - 'Bridges': bridge.more_superuser_bridges, + 'Bridges': bridge.more_superuser_bridges, # type: ignore[attr-defined] 'Current': 'none', 'Methods': format_methods({'pseudo': 'pseudo'}), }) @@ -251,7 +250,7 @@ async def test_superuser_dbus_pw(bridge, transport, monkeypatch): @pytest.mark.asyncio -async def test_superuser_dbus_wrong_pw(bridge, transport, monkeypatch): +async def test_superuser_dbus_wrong_pw(bridge: Bridge, transport: MockTransport, monkeypatch) -> None: monkeypatch.setenv('PSEUDO_PASSWORD', 'p4ssw0rd') add_pseudo(bridge) await verify_root_bridge_not_running(bridge, transport) @@ -260,7 +259,7 @@ async def test_superuser_dbus_wrong_pw(bridge, transport, monkeypatch): await transport.add_bus_match('/superuser', 'cockpit.Superuser') await transport.watch_bus('/superuser', 'cockpit.Superuser', { - 'Bridges': bridge.more_superuser_bridges, + 'Bridges': bridge.more_superuser_bridges, # type: ignore[attr-defined] 'Current': 'none', 'Methods': format_methods({'pseudo': 'pseudo'}), }) @@ -287,7 +286,7 @@ async def test_superuser_dbus_wrong_pw(bridge, transport, monkeypatch): @pytest.mark.asyncio -async def test_superuser_init(bridge, no_init_transport): +async def test_superuser_init(bridge: Bridge, no_init_transport: MockTransport) -> None: add_pseudo(bridge) no_init_transport.init(superuser={"id": "pseudo"}) transport = no_init_transport @@ -299,7 +298,7 @@ async def test_superuser_init(bridge, no_init_transport): @pytest.mark.asyncio -async def test_superuser_init_pw(bridge, no_init_transport, monkeypatch): +async def test_superuser_init_pw(bridge: Bridge, no_init_transport: MockTransport, monkeypatch) -> None: monkeypatch.setenv('PSEUDO_PASSWORD', 'p4ssw0rd') add_pseudo(bridge) no_init_transport.init(superuser={"id": "pseudo"}) @@ -315,7 +314,7 @@ async def test_superuser_init_pw(bridge, no_init_transport, monkeypatch): @pytest.mark.asyncio -async def test_no_login_messages(transport): +async def test_no_login_messages(transport: MockTransport) -> None: await transport.check_bus_call('/LoginMessages', 'cockpit.LoginMessages', 'Get', [], ["{}"]) await transport.check_bus_call('/LoginMessages', 'cockpit.LoginMessages', 'Dismiss', [], []) await transport.check_bus_call('/LoginMessages', 'cockpit.LoginMessages', 'Get', [], ["{}"]) @@ -333,7 +332,7 @@ def login_messages_envvar(monkeypatch): @pytest.mark.asyncio -async def test_login_messages(login_messages_envvar, transport): +async def test_login_messages(login_messages_envvar, transport: MockTransport) -> None: del login_messages_envvar await transport.check_bus_call('/LoginMessages', 'cockpit.LoginMessages', 'Get', [], ["msg"]) @@ -348,7 +347,7 @@ async def test_login_messages(login_messages_envvar, transport): @pytest.mark.asyncio -async def test_freeze(bridge, transport): +async def test_freeze(bridge: Bridge, transport: MockTransport) -> None: koelle = await transport.check_open('echo') malle = await transport.check_open('echo') @@ -380,7 +379,7 @@ async def test_freeze(bridge, transport): @pytest.mark.asyncio -async def test_internal_metrics(transport): +async def test_internal_metrics(transport: MockTransport) -> None: metrics = [ {"name": "cpu.core.user", "derive": "rate"}, {"name": "memory.used"}, @@ -401,6 +400,7 @@ async def test_internal_metrics(transport): # actual data _, data = await transport.next_frame() data = json.loads(data) + assert isinstance(data, list) # cpu.core.user instances should be the same as meta sent instances assert instances == len(data[0][0]) # all instances should be False, as this is a rate @@ -410,7 +410,7 @@ async def test_internal_metrics(transport): @pytest.mark.asyncio -async def test_fsread1_errors(transport): +async def test_fsread1_errors(transport: MockTransport) -> None: await transport.check_open('fsread1', path='/etc/shadow', problem='access-denied') await transport.check_open('fsread1', path='/', problem='internal-error', reply_keys={'message': "[Errno 21] Is a directory: '/'"}) @@ -420,7 +420,7 @@ async def test_fsread1_errors(transport): @pytest.mark.asyncio -async def test_fsread1_size_hint(transport): +async def test_fsread1_size_hint(transport: MockTransport) -> None: data = None stat = os.stat('/usr/lib/os-release') with open('/usr/lib/os-release', 'rb') as fp: @@ -431,32 +431,29 @@ async def test_fsread1_size_hint(transport): @pytest.mark.asyncio -async def test_fsread1_size_hint_absent(transport): +async def test_fsread1_size_hint_absent(transport: MockTransport) -> None: # non-binary fsread1 has no size-hint await transport.check_open('fsread1', path='/etc/passwd', absent_keys=['size-hint']) @pytest.mark.asyncio -async def test_fsread1_size_hint_absent_char_device(transport): +async def test_fsread1_size_hint_absent_char_device(transport: MockTransport) -> None: # character device fsread1 has no size-hint await transport.check_open('fsread1', path='/dev/null', binary='raw', absent_keys=['size-hint']) @pytest.mark.asyncio -async def test_fslist1_no_watch(transport): - tempdir = tempfile.TemporaryDirectory() - dir_path = Path(tempdir.name) - +async def test_fslist1_no_watch(transport: MockTransport, tmp_path: Path) -> None: # empty - ch = await transport.check_open('fslist1', path=str(dir_path), watch=False) + ch = await transport.check_open('fslist1', path=str(tmp_path), watch=False) await transport.assert_msg('', command='done', channel=ch) await transport.check_close(channel=ch) # create a file and a directory in some_dir - Path(dir_path, 'somefile').touch() - Path(dir_path, 'somedir').mkdir() + (tmp_path / 'somefile').touch() + (tmp_path / 'somedir').mkdir() - ch = await transport.check_open('fslist1', path=str(dir_path), watch=False) + ch = await transport.check_open('fslist1', path=str(tmp_path), watch=False) # don't assume any ordering msg1 = await transport.next_msg(ch) msg2 = await transport.next_msg(ch) @@ -470,16 +467,70 @@ async def test_fslist1_no_watch(transport): @pytest.mark.asyncio -async def test_fslist1_notexist(transport): +async def test_fslist1_notexist(transport: MockTransport) -> None: await transport.check_open( 'fslist1', path='/nonexisting', watch=False, problem='not-found', reply_keys={'message': "[Errno 2] No such file or directory: '/nonexisting'"}) +@pytest.mark.asyncio +async def test_fsreplace1(transport: MockTransport, tmp_path: Path) -> None: + # create non-existing file + myfile = tmp_path / 'newfile' + ch = await transport.check_open('fsreplace1', path=str(myfile)) + transport.send_data(ch, b'some stuff') + transport.send_done(ch) + await transport.assert_msg('', command='done', channel=ch) + await transport.check_close(channel=ch) + assert myfile.read_bytes() == b'some stuff' + # no leftover files + assert os.listdir(tmp_path) == ['newfile'] + + # now update its contents + ch = await transport.check_open('fsreplace1', path=str(myfile)) + transport.send_data(ch, b'new new new!') + transport.send_done(ch) + await transport.assert_msg('', command='done', channel=ch) + await transport.check_close(channel=ch) + assert myfile.read_bytes() == b'new new new!' + # no leftover files + assert os.listdir(tmp_path) == ['newfile'] + + # write empty file + ch = await transport.check_open('fsreplace1', path=str(myfile)) + transport.send_data(ch, b'') + transport.send_done(ch) + await transport.assert_msg('', command='done', channel=ch) + await transport.check_close(channel=ch) + assert myfile.read_bytes() == b'' + + # delete file + ch = await transport.check_open('fsreplace1', path=str(myfile)) + transport.send_done(ch) + await transport.assert_msg('', command='done', channel=ch) + await transport.check_close(channel=ch) + assert not myfile.exists() + + +@pytest.mark.asyncio +async def test_fsreplace1_error(transport: MockTransport, tmp_path: Path) -> None: + # trying to write a directory + ch = await transport.check_open('fsreplace1', path=str(tmp_path)) + transport.send_data(ch, b'not me') + transport.send_done(ch) + await transport.assert_msg('', command='close', channel=ch, problem='access-denied') + + # nonexisting directory + ch = await transport.check_open('fsreplace1', path='/non/existing/file') + transport.send_data(ch, b'not me') + transport.send_done(ch) + await transport.assert_msg('', command='close', channel=ch, problem='not-found') + + @pytest.mark.asyncio @pytest.mark.parametrize('channeltype', CHANNEL_TYPES) -async def test_channel(bridge, transport, channeltype, tmp_path): +async def test_channel(bridge: Bridge, transport: MockTransport, channeltype, tmp_path: Path) -> None: payload = channeltype.payload args = dict(channeltype.restrictions) @@ -544,7 +595,7 @@ async def serve_page(reader, writer): assert not saw_data break else: - pytest.fail('unexpected event', (payload, args, control)) + pytest.fail(f'unexpected event: {(payload, args, control)}') else: saw_data = True @@ -590,13 +641,13 @@ async def serve_page(reader, writer): {'X': 'a:b', 'Z': 'a-b', 'V': 'a_b'} ), ]) -def test_get_os_release(os_release, expected): +def test_get_os_release(os_release: str, expected: str) -> None: with unittest.mock.patch('builtins.open', unittest.mock.mock_open(read_data=os_release)): assert Bridge.get_os_release() == expected @pytest.mark.asyncio -async def test_flow_control(transport, tmp_path): +async def test_flow_control(transport: MockTransport, tmp_path: Path) -> None: bigun = tmp_path / 'bigun' total_bytes = 8 * 1024 * 1024 recvd_bytes = 0 @@ -605,7 +656,7 @@ async def test_flow_control(transport, tmp_path): # We should receive a number of blocks of initial data, each with a ping. # We save the pings to reply later. - pings = deque() + pings: 'deque[JsonObject]' = deque() async def recv_one(): nonlocal recvd_bytes @@ -638,7 +689,7 @@ async def recv_one(): @pytest.mark.asyncio -async def test_large_upload(event_loop, transport, tmp_path): +async def test_large_upload(event_loop: asyncio.AbstractEventLoop, transport: MockTransport, tmp_path: Path) -> None: fifo = str(tmp_path / 'pipe') os.mkfifo(fifo) diff --git a/tools/valgrind.supp b/tools/valgrind.supp deleted file mode 100644 index 553ac10027dd..000000000000 --- a/tools/valgrind.supp +++ /dev/null @@ -1,12 +0,0 @@ -# Valgrind reports this on clang/i386 due to being unable to find a variable on -# the stack, due to clang discarding the value before calling errx() (since it -# knows it won't need it anymore, due to errx() never returning). -{ - stack_variable_bug_workaround - Memcheck:Leak - match-leak-kinds: definite - ... - fun:cockpit_certificate_locate - fun:cockpit_certificate_find - fun:main -}