-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools/docker: Add Docker image support for devlib #672
Conversation
a590c12
to
ac12eee
Compare
8bab068
to
72bc9fd
Compare
tools/docker/Dockerfile
Outdated
RUN rm -rf /var/cache/apt | ||
|
||
RUN git clone -b ${DEVLIB_REF} -v https://github.com/ARM-software/devlib.git /devlib | ||
RUN cd /devlib && python3 setup.py install && pip3 install .[full] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity why do we need to run both python setup
and pip install
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had copied from https://github.com/ARM-software/workload-automation/blob/master/extras/Dockerfile#L109. Eliminated python setup install
which looks to be obsolote now.
tools/docker/Dockerfile
Outdated
flex \ | ||
gawk \ | ||
git \ | ||
g++-multilib \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely not to be an expected case but on my M1 Mac this fails to build with:
E: Unable to locate package g++-multilib
E: Couldn't find any package by regex 'g++-multilib'
I'm wondering where this list of required packages is generated from or whether there is an arch agnostic version we could use instead?
Just to confirm this builds correctly on my x86 machine so if not it might be worth adding a note to note this restriction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main source of the packages is https://github.com/buildroot/buildroot/blob/master/support/docker/Dockerfile#L33.
Apparently g++-multilib
is not required (at least explicitly). Thus, removed it.
${EMULATOR} -avd devlib-chromeos "${EMULATOR_ARGS}" & | ||
|
||
# AVDs needs ~30 seconds to complete boot up. | ||
sleep 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: For such a delay is it worth displaying something to the user to inform them of this wait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Will show a message before sleeping.
a30c61e
to
a8311dc
Compare
78b1a8a
to
12063f0
Compare
devlib/utils/android.py
Outdated
self.logger.debug('server=%s port=%s device=%s as_root=%s', | ||
adb_server, adb_port, device, adb_as_root) | ||
# pylint: disable=logging-fstring-interpolation | ||
self.logger.debug(f'{adb_server=} {adb_port=} {device=} {adb_as_root=}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f'{x=}'
is nice syntax but only available starting with Python 3.8. Devlib currently supports down to 3.7, so you can use the more verbose f'x={x}'
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted this hunk.
tools/docker/Dockerfile
Outdated
rm -rf /var/cache/apt | ||
|
||
RUN git clone -b ${DEVLIB_REF} -v https://github.com/ARM-software/devlib.git /devlib | ||
RUN cd /devlib && pip install .[full] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the pip
version packaged by ubuntu is typically quite old, so the first thing one does generally is calling pip install --upgrade pip
. Also, setuptools is typically required for most setup.py implementations, so you'll need to install pip install setuptools
, and also the wheel
package (otherwise you might end up install from source some stuff that are pre-packaged AFAIR).
pyproject.toml
has removed the necessity to install setuptools
and wheel
separately AFAIK, but not all projects use it, and some advanced use cases are not covered by it (yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python setup.py install
is also obsolete. Unsure how to address your concern. Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python setup.py install is also obsolete.
Calling setup.py directly is indeed obsolete, but pip
calling it is very much still supported and is used by a large amount (the majority ?) of projects. pyproject.toml
has gained a lot of traction recently, but it's quite new, and there are still things that require executing code (for example there is no way to automatically accumulate all the extra requires into an "all"/"full" extra requires, or complex builds with C extensions, things like that). There used to be a setup.cfg
thing as well, but it's an older iteration of the same idea as pyproject.toml
.
Unsure how to address your concern. Can you elaborate?
pip install --upgrade pip setuptools wheels
pip install .[full]
If you don't do the first line, you might end up with:
- an antediluvian pip that does not know how to talk to current PyPI
- no setuptools package installed, leading to an ImportError in setup.py (assuming the setup.py uses setuptools, which it does for devlib and lots of projects)
- no wheels package, leading to issues if pip needs to build a wheel for whatever reason
Otherwise, if you migrate to using pyproject.toml
, all you would need is ensuring pip is recent-enough, since the toml file describes what build backend is used (e.g. setuptools) so that pip can install it without having to do it manually beforehand:
pip install --upgrade pip
pip install .[full]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Updated.
Also clean a mutable default value (``modules=[]`` in ``ChromeOsTarget`` class). Signed-off-by: Metin Kaya <metin.kaya@arm.com>
We can mimic ChromeOS target by combining a QEMU guest (for Linux bindings of ``ChromeOsTarget`` class) with a Android virtual desktop (for Android bits of ``ChromeOsTarget``). Note that Android bindings of ``ChromeOsTarget`` class also requires existence of ``/opt/google/containers/android`` folder on the Linux guest. Signed-off-by: Metin Kaya <metin.kaya@arm.com>
Default connection timeout (30 secs) may be insufficient for some test setups or in some conditions. Thus, support specifying timeout parameter in target configuration file. Signed-off-by: Metin Kaya <metin.kaya@arm.com>
Introduce a Dockerfile in order to create Docker image for devlib and ``run_tests.sh`` script to test Android, Linux, LocalLinux, and QEMU targets on the Docker image. The Dockerfile forks from ``Ubuntu-22.04``, installs required system packages, checks out ``master`` branch of devlib, installs devlib, creates Android virtual devices via ``tools/android/install_base.sh``, and QEMU images for aarch64 and x86_84 architectures. Note that Android command line tools version, buildroot and devlib branches can be customized via environment variables. Signed-off-by: Metin Kaya <metin.kaya@arm.com>
Commits: