RE: [PATCH v4 2/9] dts: add developer tools

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Tuesday, September 13, 2022 10:38 PM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>; Bruce Richardson
> <bruce.richardson@intel.com>
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> ronan.randles@intel.com; ohilyard@iol.unh.edu; lijuan.tu@intel.com;
> dev@dpdk.org; nd <nd@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 <ohilyard@iol.unh.edu>
> > > > Signed-off-by: Juraj Linkeš <juraj.linkes@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
> > > >