-
Notifications
You must be signed in to change notification settings - Fork 7
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
boot-utils: Initial bring up #1
Conversation
There are a few places where we report errors. Instead of open coding it, create a die() function that prints a message in bold red and exits. This change allows us to simplify and consolidate some statements and add more informative messages to future changes. Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
… no arguments to the script This script is not intended to be run by many people thus no usage text but I frequently forget that I need to pass in 'all' to have it do anything. Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
We are going to rebuild all of the images so we should make sure that we have the latest LTS Buildroot release. Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
My benchmarks show that zstd has the second best decompression ratio/speed, whereas the best decompression ratio algorithm is not the same as the best decompression speed algorithm. Compression speed is not awful either but that is not as much of a concern because compression only happens during image build time, where it is a very small fraction of the overall build process. Check that the zstd tool is installed and bail out if it is not. Link: ClangBuiltLinux/continuous-integration#184 (comment) Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
$ ./buildroot/rebuild.sh all Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
We have had issues with Debian's skiboot.lid so we have to build our own. To make sure we can always build it, add a script that handles this for us. $ ./build-skiboot.sh 6.5.4 Requires CROSS=<path_to_cross_compiler> or for powerpc64le-linux-gnu-gcc to be available in PATH. Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Built with GCC 9.3.0 and tested with current CI checkout. Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
To quote Nick: "Orthogonally, I've been finding it frustrating to: 1. jump on a new machine without shell history, and try to recreate our QEMU commands from driver.sh. Even sending commands to newbies is a PITA. 2. boot more than just our scripts that immediately power off a machine." driver.sh is a very basic kernel build script, it is not good for extensive testing because it only builds a defconfig kernel with no modules. However, the boot testing that it provides is invaluable because we have figured out all of the necessary QEMU flags. Enter boot-qemu.sh, a split out version of driver.sh's booting framework. There are two required arguments: the architecture being booted (which expects the same value as driver.sh does) and a path to the kernel's object folder (either the kernel source itself if an in-tree build or the path to O= if an out-of-tree build). By default, the rootfs will launch /init, which will execute our init script that just prints the kernel version and shuts the machine down. The script has an option to dump into an interactive shell (busybox's shell) so that more testing can be done. Run: $ ./boot-qemu.sh -h for more information. Suggested-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
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.
No real hangups, more-so questions. Feel free to submit with follow up issues filed if you think we need them.
-k | --kbuild-folder: | ||
The kernel build folder, as an absolute path or relative path | ||
from wherever the script is being run. This is wherever the | ||
compiled vmlinux image lives, not the architecture's boot folder. |
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.
Why the folder and not path to kernel image?
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.
For a couple of the ARM devices, we need the dtb to boot:
boot-qemu.sh
Outdated
function sanity_check() { | ||
# Kernel build folder and architecture are required paramters | ||
# We are not interactive by default and the default timeout is 3m | ||
: "${ARCH:?}" "${KBUILD_DIR:?}" "${INTERACTIVE:=false}" "${TIMEOUT:=3m}" |
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.
what does the :
do?
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.
It is basically a shell builtin that acts like true
. The point here is so that I can avoid a bunch of:
[[ -z ${ARCH} ]] && die "ARCH is undefined"
and such.
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.
But you only need it for ARCH
and KBUILD_DIR
because of the :?
?
When we start using sigils, I get nervous that the code starts to look like Perl, and there's good jokes about Perl being unreadable for a reason.
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.
Yes, I prefer to be explicit about the variables that I expect to be defined rather than using set -u
.
Otherwise, I can do the more verbose thing I mentioned if it is more readable.
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.
Specially if the symbols are not that common in other languages.
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.
Alright, I pushed 936f954, which should be less cryptic.
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.
That looks way better for both user and dev, thanks!
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Suggested-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
I will leave this up until Monday afternoon PDT/MST to allow for any last comments. Once we get this merged, we can merge the CI one so that this starts getting used there too. |
This is a bit more clear for the user and anyone reading the script. Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
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.
🉑 🇬🇦
CI reported a crash when attempting to boot arm64 kernels: Traceback (most recent call last): File ".../boot-qemu.py", line 727, in <module> cfg = get_qemu_args(cfg) File ".../boot-qemu.py", line 451, in get_qemu_args linux_ver_code = get_linux_ver_code(gzip_kernel_cmd) File ".../boot-qemu.py", line 338, in get_linux_ver_code return create_version_code(linux_version) File ".../boot-qemu.py", line 262, in create_version_code major, minor, patch = [int(version[i]) for i in (0, 1, 2)] File ".../boot-qemu.py", line 262, in <listcomp> major, minor, patch = [int(version[i]) for i in (0, 1, 2)] ValueError: invalid literal for int() with base 10: '%s' Looking at the strings output reveals: $ gzip -d -c Image.gz &| strings &| grep "Linux version " Linux version %s (%s) Linux version 4.19.254 (tuxmake@tuxmake) (Debian clang version 14.0.6-++20220622053050+f28c006a5895-1~exp1~20220622173135.152, Debian LLD 14.0.6) ClangBuiltLinux#1 SMP PREEMPT @1659593940 Use re.search() plus a regular expression for a more precise match. I had originally tried to do this but I could not get it to work for some reason. I tested this against all the kernels that had issues in CI with no further issues. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
The commit messages should break everything down well. Please let me know if you have any questions.
If this all looks good, I'll write a README after.