> -----Original Message----- > From: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com> > Sent: Tuesday, September 13, 2022 10:38 PM > To: Juraj Linkeš <juraj.lin...@pantheon.tech>; Bruce Richardson > <bruce.richard...@intel.com> > Cc: tho...@monjalon.net; david.march...@redhat.com; > ronan.rand...@intel.com; ohily...@iol.unh.edu; lijuan...@intel.com; > dev@dpdk.org; nd <n...@arm.com> > Subject: RE: [PATCH v4 2/9] dts: add developer tools > > (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? >
What about: There are two Docker images defined in this Dockerfile. One is to be used in CI for automated testing. The other provides a DTS development environment, simplifying Python dependency management. > > > > +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/ > Ack. > > > > + > > > > +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? > Good point, a small introduction would help. It currently documents the steps for setting up DTS development environment as well as explaining a bit about the backround of how it's set up (using Poetry). > 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. > Yea we need this. I'll update the docs with definitions. > > > > @@ -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? > Ack. The next version will have more in terms of devtools, so stay tuned :-). > > > > +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? > We should decide where we'll put it, either to dpdk/devtools or dpdk/dts/devtools. So far I have it in dpdk/dts/devtools, but it may make more sense to put it into dpdk/devtools. > > > > 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. > Yes, as in the unmodified code should be properly formatted by these exact tools (thus the next run of these tools would produce the same output, which is no change). > > > > +} > > > > + > > > > +if [ "$1" == "-h" ] || [ "$1" == "--help" ]; then > > > > + help > > > > + exit 0 > > > > +fi > > > > + > > > > +main "$1" > > > > + > > > > -- > > > > 2.30.2 > > > >
