(I have lost the original patch emails due to quarantine policy, apologies for using this thread for my comments)
<snip> > > > > On Fri, Jul 29, 2022 at 10:55:43AM +0000, Juraj Linkeš wrote: > > > The Dockerfile contains basic image for CI and developers. There's > > > also an integration of the Dockerfile with Visual Studio. > > > > > > The formatter script uses Black and Isort to format the Python code. > > > > > > Signed-off-by: Owen Hilyard <ohily...@iol.unh.edu> > > > Signed-off-by: Juraj Linkeš <juraj.lin...@pantheon.tech> > > > > Comments inline below. > > > > Thanks, > > /Bruce > > > > > --- > > > dts/.devcontainer/devcontainer.json | 30 ++++++++++++ > > > dts/Dockerfile | 38 +++++++++++++++ > > > dts/README.md | 74 ++++++++++++++++++++++++++++- > > > dts/format.sh | 45 ++++++++++++++++++ > > > 4 files changed, 186 insertions(+), 1 deletion(-) create mode > > > 100644 dts/.devcontainer/devcontainer.json > > > create mode 100644 dts/Dockerfile > > > create mode 100755 dts/format.sh > > > > > > diff --git a/dts/.devcontainer/devcontainer.json > > > b/dts/.devcontainer/devcontainer.json > > > new file mode 100644 > > > index 0000000000..41ca28fc17 > > > --- /dev/null > > > +++ b/dts/.devcontainer/devcontainer.json > > > @@ -0,0 +1,30 @@ > > > +// For format details, see https://aka.ms/devcontainer.json. For > > > +config > > options, see the README at: > > > +// > > > +https://github.com/microsoft/vscode-dev-containers/tree/v0.241.1/co > > > +nt > > > +ainers/docker-existing-dockerfile > > > +{ > > > + "name": "Existing Dockerfile", > > > + > > > + // Sets the run context to one level up instead of the > > > +.devcontainer > > folder. > > > + "context": "..", > > > + > > > + // Update the 'dockerFile' property if you aren't using the > > > +standard > > 'Dockerfile' filename. > > > + "dockerFile": "../Dockerfile", > > > + > > > + // Use 'forwardPorts' to make a list of ports inside the container > > available locally. > > > + // "forwardPorts": [], > > > + > > > + // Uncomment the next line to run commands after the container is > > created - for example installing curl. > > > + "postCreateCommand": "poetry install", > > > + > > > + "extensions": [ > > > + "ms-python.vscode-pylance", > > > + ] > > > + > > > + // Uncomment when using a ptrace-based debugger like C++, Go, and > > Rust > > > + // "runArgs": [ "--cap-add=SYS_PTRACE", "--security-opt", > > > +"seccomp=unconfined" ], > > > + > > > + // Uncomment to use the Docker CLI from inside the container. See > > https://aka.ms/vscode-remote/samples/docker-from-docker. > > > + // "mounts": [ > > > +"source=/var/run/docker.sock,target=/var/run/docker.sock,type=bind" > > > +], > > > + > > > + // Uncomment to connect as a non-root user if you've added one. > > > +See > > https://aka.ms/vscode-remote/containers/non-root. > > > + // "remoteUser": "vscode" > > > +} > > > diff --git a/dts/Dockerfile b/dts/Dockerfile new file mode 100644 > > > index 0000000000..6700aa45b8 > > > --- /dev/null > > > +++ b/dts/Dockerfile > > > @@ -0,0 +1,38 @@ > > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2022 > > > +University of New Hampshire # > > > + Is it possible to add some text here talking about the use of this file? > > > +FROM ubuntu:22.04 AS base > > > + > > > +RUN apt-get -y update && apt-get -y upgrade && \ > > > + apt-get -y install --no-install-recommends \ > > > + python3 \ > > > + python3-pip \ > > > + python3-pexpect \ > > > + python3-poetry \ > > > + python3-cachecontrol \ > > > + openssh-client > > > + > > > + > > > +FROM base AS runner > > > + > > > +# This container is intended to be used as the base for automated > > > systems. > > > +# It bakes DTS into the container during the build. > > > + > > > +RUN mkdir /dts > > > +COPY ./pyproject.toml /dts/pyproject.toml COPY ./poetry.lock > > > +/dts/poetry.lock WORKDIR /dts RUN poetry install --no-dev COPY . > > > +/dts > > > > Two questions here: > > * if we copy over the current folder, does it re-copy the same two files > > above, or do we get a new subfolder with the same name as the current > > one (and the two files in that instead)? > > * Can the commands be re-ordered so that we have all the copies together > > rather than being split either side of the workdir and run commands? > > > > Yea, we don't need to copy the two files individually - we only need to copy > the > whole dts folder. I'll move the commands. > > > > + > > > +CMD ["poetry", "run", "python", "main.py"] > > > + > > > +FROM base AS dev > > > + > > > +# This container is intended to be used as a development environment. May be s/development environment/DTS development environment/ > > > + > > > +RUN apt-get -y install --no-install-recommends \ > > > + vim emacs git > > > + > > If it's to be used as a development environment, do we not need > > build-essential installed? > > > > It's meant to be a DTS development environment and we don't need to build > anything for that, so no need for build-essential. The above addition will clarify this. > > > > +WORKDIR /dts > > > > Is this needed twice in the file, since it appears above too? > > > > It appears in the definitions of two separate images, but we can actually > move it > to the base image to have it in the file only once. > > > > diff --git a/dts/README.md b/dts/README.md index > > > d8f88f97fe..55a272d767 100644 > > > --- a/dts/README.md > > > +++ b/dts/README.md It is not clear to me what we want to document in this file. We probably need to add few lines in the beginning of this file to indicate what exactly we want to document. For ex: is this a 'Developer' README? Does it make sense to add a brief introduction about various nodes in DTS (tester, SUT, TG) as we start using the terms without explaining them. > > > @@ -12,4 +12,76 @@ The Python Version required by DTS is specified > > > in [DTS python config file](./pyproject.toml) in the > > > **[tool.poetry.dependencies]** section. Poetry doesn't install > > > Python, so you may need to satisfy this requirement if your Python > > > is not up to date. A tool such as > > > [Pyenv](https://github.com/pyenv/pyenv) > > > -is a good way to get Python, though not the only one. > > > +is a good way to get Python, though not the only one. However, DTS > > > +includes a development environment in the form of a Docker image. > > > + > > > +# Expected Environment > > > + > > > +The expected execution and development environments for DTS are the > > > +same, the container defined by [Dockerfile](./Dockerfile). Using a > > > +container for the development environment helps with a few things. > > > + > > > +1. It helps enforce the boundary between the tester and the traffic > > > + generator/sut, something which has experienced issues in the past. > > > > s/experienced/caused/ > > > > Ack. > > > > +2. It makes creating containers to run DTS inside automated tooling > > > + much easier, since they can be based off of a known-working > environment > > > + that will be updated as DTS is. > > > +3. It abstracts DTS from the server it is running on. This means that the > > > + bare-metal os can be whatever corporate policy or your personal > > preferences > > > + dictate, and DTS does not have to try to support all 15 distros that > > > + are supported by DPDK CI. > > > > Remove the "15". > > > > Ack, this will make it accurate even when thing change slightly in the lab. > > > > +4. It makes automated testing for DTS easier, since new dependencies can > be > > > + sent in with the patches. > > > +5. It fixes the issue of undocumented dependencies, where some test > > > suites > > > + require python libraries that are not installed. > > > +6. Allows everyone to use the same python version easily, even if they > > > are > > > + using an LTS distro or Windows. > > > > Presumably the LTS distro is an *older* LTS distribution with possibly > > out-of-date packages? That should perhaps be made clearer. > > > > I'll change it to "even if they are using a distribution or Windows with > out-of- > date packages", that should be clear enough. > > > > +7. Allows you to run the tester on Windows while developing via Docker > for > > > + Windows. > > > + > > > +## Tips for setting up a development environment > > > + > > > +### Getting a docker shell > > > + > > > +These commands will give you a bash shell inside the container with > > > +all the python dependencies installed. This will place you inside a > > > +python virtual environment. DTS is mounted via a volume, which is > > > +essentially a symlink from the host to the container. This enables > > > +you to edit and run inside the container and then delete the > > > +container when > > you are done, keeping your work. > > > + > > > +```shell > > > +docker build --target dev -t dpdk-dts . > > > +docker run -v $(pwd):/dts -it dpdk-dts bash $ poetry install $ > > > +poetry shell ``` > > > + > > > +### Vim/Emacs > > > + > > > +Any editor in the ubuntu repos should be easy to use. You can add > > > +your normal config files as a volume, enabling you to use your > > > +preferred > > settings. > > > + > > > +```shell > > > +apt install vim > > > +apt install emacs > > > +``` > > > > Were these not already installed in the image created using the > > dockerfile above? > > > > They were. I'll remove the install commands and instead add a modified docker > command mounting vim config file as volume. > > > > + > > > +### Visual Studio Code > > > + > > > +VSCode has first-class support for developing with containers. You > > > +may need to run the non-docker setup commands in the integrated > > > +terminal. DTS contains a .devcontainer config, so if you open the > > > +folder in vscode it should prompt you to use the dev container > > > +assuming you have the plugin installed. Please refer to [VS > > > +Development Containers > > > +Docs](https://code.visualstudio.com/docs/remote/containers) > > > +to set it all up. > > > + > > > +### Other > > > + > > > +Searching for '$IDE dev containers' will probably lead you in the > > > +right direction. > > > + > > > +# Python Formatting > > > + > > > +The tools used to format Python code in DTS are Black and Isort. > > > +There's a shell script, function.sh, which runs the formatters. ^^^^^^^^^^ format.sh? > > > +Poetry will install these tools, so once you have that set up, you > > > +should run it > > before submitting patches. > > > diff --git a/dts/format.sh b/dts/format.sh new file mode 100755 Should this be in dpdk/devtools directory? If yes, need a different name for the script, dts-fix-format.sh? > > > index > > > 0000000000..7d72335470 > > > --- /dev/null > > > +++ b/dts/format.sh > > > @@ -0,0 +1,45 @@ > > > +#!/usr/bin/env bash > > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2022 > > > +University of New Hampshire # Copyright(c) 2022 PANTHEON.tech s.r.o. > > > +# > > > + > > > +function main() { > > > + # The directory to work on is either passed in as argument 1, > > > + # or is the current working directory > > > + DIRECTORY=${1:-$(pwd)} > > > + LINE_LENGTH=88 > > > + > > > + BLACK_VERSION=$(awk '/\[tool.poetry.dev-dependencies\]/,/$^/' > > pyproject.toml |\ > > > + grep black | grep -o '[0-9][^"]*') > > > + > > > + PYTHON_VERSION=$(awk '/\[tool.poetry.dependencies\]/,/$^/' > > pyproject.toml |\ > > > + grep python | grep -o '[0-9][^"]*' | tr -d '.') > > > + > > > + isort \ > > > + --overwrite-in-place \ > > > + --profile black \ > > > + -j "$(nproc)" \ > > > + --line-length $LINE_LENGTH \ > > > + --python-version auto \ > > > + "$DIRECTORY" > > > + > > > + black \ > > > + --line-length $LINE_LENGTH \ > > > + --required-version "${BLACK_VERSION}" \ > > > + --target-version "py${PYTHON_VERSION}" \ > > > + --safe \ > > > + "$DIRECTORY" > > > +} > > > + > > > +function help() { > > > + echo "usage: format.sh <directory>" I guess we are assuming that the unmodified code will not change by running this command. > > > +} > > > + > > > +if [ "$1" == "-h" ] || [ "$1" == "--help" ]; then > > > + help > > > + exit 0 > > > +fi > > > + > > > +main "$1" > > > + > > > -- > > > 2.30.2 > > >
