Easier CI Package Addition #2748
Conversation
shashank-mahadasyam
force-pushed
the
consolidate-ci-packages
branch
2 times, most recently
from
eb18b6a to
f28b0c0
Compare
|
Thanks for the extra PR. |
shashank-mahadasyam
force-pushed
the
consolidate-ci-packages
branch
from
f28b0c0 to
05c574f
Compare
|
I've mostly ironed out all the CI issues. So, hopefully, the failures shouldn't be because of this PR 🤞 |
scripts/build/Dockerfile.archlinux
Outdated
|
|
||
| COPY . /criu | ||
| WORKDIR /criu | ||
| RUN pacman -Syu --noconfirm $(sed 's/\#.*$//' contrib/pacman-packages.txt) |
There was a problem hiding this comment.
Would it be possible to create subdirectory called "dependencies" and add "pacman.txt", "apk.txt", "apt.txt" inside?
| iptables \ | ||
| util-linux-dev \ | ||
| bash | ||
| RUN apk update |
There was a problem hiding this comment.
Every RUN command creates a new layer in the container image. It might be better to use RUN apk update && apk add $(sed 's/\#.*$//' contrib/apk-packages.txt)
There was a problem hiding this comment.
That's a good point. I'll combine the update and installation steps in all the Dockerfiles wherever possible
|
|
||
| COPY . /criu | ||
| WORKDIR /criu | ||
| RUN apt-install $(sed 's/\#.*$//' contrib/apt-packages.txt) |
There was a problem hiding this comment.
Most of the packages above (e.g., protobuf-c-compiler, protobuf-compiler, libbsd-dev, make, pkg-config, gcc, and iptables) are also required for CRIU. Should we include them as well?
There was a problem hiding this comment.
Sure, I can do that. The reason some of the obvious ones are not there in apt-packages.txt is because some Dockerfiles/scripts aren't installing those packages. I didn't think too much when populating these package list files. I just saw the common packages across all the Dockerfiles/scripts for each package manager and put the common ones in the package list files.
For example, libbsd-dev is used in Dockerfile.openj9-ubuntu, Dockerfile.hotspot-ubuntu, and run-ci-tests.sh, but not in Dockerfile.[un]stable-cross.tmpl, and Dockerfile.linux32.tmpl
There was a problem hiding this comment.
This is because different people have introduced the Dockerfile files at different time. When consolidating the list of packages it would be good, if possible, to make them consistent.
There was a problem hiding this comment.
Then I'll add the packages mentioned on the criu installation page (https://criu.org/Installation) to all the package list files
|
|
||
| COPY . /criu | ||
| WORKDIR /criu | ||
| RUN apt-install $(sed 's/\#.*$//' contrib/apt-packages.txt) |
There was a problem hiding this comment.
Same comment here. It would be good to include all required dependencies in the TXT file. The following page provides more information: https://criu.org/Installation
|
@shashank-mahadasyam What do you think about using shell scripts instead of |
shashank-mahadasyam
force-pushed
the
consolidate-ci-packages
branch
2 times, most recently
from
d3345b4 to
b571721
Compare
|
Hi, @rst0git! I've made three changes, as suggested:
I also sorted all package name lists. You may see quite some diff noise because of that.
I'm okay with that too, but I don't see much of a difference. To go from these files to the installation command is just a one-liner 😅 Also, I've spent more time than I anticipated on this now. My main goal is the uprobes PR (#2717), and this ended up being a side-quest. So, I'll definitely polish this PR up to the point where it can support the uprobes PR and where the CI pipelines pass. So I would really appreciate it if any further changes are done post-merge :) |
Currently, adding a package which is required either for development or testing requires it to be added in multiple places (many Dockerfiles, some scripts), which is difficult to track, and can lead to some places being missed. Add package list files for the package managers apk, apt, dnf, and pacman, and download the packages in the files in the Dockerfiles/scripts. Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
Packages common to the Dockerfiles of alpine and hotspot-alpine are consolidated in apk-packages.txt. Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
shashank-mahadasyam
force-pushed
the
consolidate-ci-packages
branch
from
b571721 to
093ec9d
Compare
| libnftables-dev:${DEBIAN_ARCH} \ | ||
| libgnutls28-dev:${DEBIAN_ARCH} \ | ||
| iproute2:${DEBIAN_ARCH} \ | ||
| $(awk '!/^#/ && NF {print $0 ":'"${DEBIAN_ARCH}"'"}' /tmp/apt-packages.txt) \ |
There was a problem hiding this comment.
apt-packages.txt contains protobuf-c-compiler, doesn't it? Do we really need to install protobuf-c-compiler:${DEBIAN_ARCH} package?
There was a problem hiding this comment.
Oops, I overlooked that. I'll filter out the host-only packages from apt-packages.txt
|
@shashank-mahadasyam Would it be okay with you if we use something like the scripts in #2755? |
|
Closing this because #2755 is more complete |
4 participants
To test the
--allow-uprobesintroduces by #2717 requires the addition of a few packages to be downloaded in the CI pipeline. The current way of doing this requires adding the package names to multiple Dockerfiles and scripts, which are not exhaustively listed, as far as I can see. This way, some files may end up being missed. As discussed in #2717 (comment), this PR makes the Dockerfiles and scripts pull in common package names from package-manager specific files, making it easier to add new packages.