Hi Thomas, On 12 October 2017 at 17:59, Thomas Lange <la...@informatik.uni-koeln.de> wrote: > Hi Riku,
> now I had a look at your linaro fai config space. Here are my comments: Thanks for your detailed review. I'll take some time to make them more idiomatic following your suggestions. > First I tried to call the fai-diskimage command using your config > space. It failed when calling debootstrap :-( > So I found a little bug in FAI 5.4 and fixed it :-) > https://github.com/faiproject/fai/commit/efbb5a8448b6b04d9f3be74e32a222b2c8e6a09a > > class/INSTALL.var is not needed for fai-diskimage. This is only used > when you use the curses menu which reads the class/*.profile files. > The other files in class/ look good. > > I wonder why you use a fixed size (4GB) in disk_config/RAW. I would > say "4GB-" which does not define an upper bound and is therefore more > general. Some of the images will got < 4GB internal eMMC storage, so we want to make sure all images fit. > The files in package_config are well partitioned into classes. > > Instead of files/etc/hostname/DEBIAN and the preinst, I would just use > this code: echo $HOSTNAME > $target/etc/hostname > > Thanks for showing me the envsubst command in several preinst files. I > didn't knew this one. It's easier than what's currently used in the FAI > preinst examples. Neither did I know about it until recently. 20+ years of using Linux and still finding new useful commands. > The scripts DB820C/21-customize and DB410C/21-customize share some > lines of code. Keep in mind, that duplicating code is bad, and think > about putting the duplicated code into a more general FAI class, which > is then used everytime you use the DB820C and DB410C class. We are still developing these. Like premature optimization, premature abstraction is bad ;) We are adding at least hikey and hikey960 support. With all HW support in, we will see more clearly howto split the classes. > The wget of the firmware zip file will put and extract the files into > the current working directory as root. I would use /tmp for that. > > In several scripts you use "chroot $target" which should be replaced > just by $ROOTCMD, which is the same (except if you call fai > softupdate, which is then empty). Oh, cool, didn't notice $ROOTCMD. > For things like "echo string >> file" you could use ainsl(1). > > In 22-disable-systemd-services.chroot I wonder why you use an array > instead of just a space separated string like this: > services="NetworkManager systemd-networkd" > for service in ${services}; do > $ROOTCMD systemctl mask ${service}-wait-online.service > done > > Silimar is valid for 02-add_linaro_to_groups.chroot where you redefine > IFS instead of separating the list of DEFGROUP just by spaces. > The hook debconf.SAVECACHE is not needed IMO. If there's a basefile > matching a class name in the subdirectory basefiles/, then FAI will > automatically use it and will not call debootstrap. If you define > something linke this > export FAI_BASEFILEURL=https://fai-project.org/download/basefiles/ > the first time you call fai-diskimage, FAI will download a basefile > (matching a class name) into the subdir and the second time you call > fai-diskimage FAI will just use this file automatically. This hook is a hack to *create* the basefile. The documentation suggests using pbuilder to create the basefile, and the basefile url has mk-basefile. The problem is that neither of these ways is identical to what fai-diskimage creates on the first run. > My overall impression is very good! All the things I said are just > minor cosmetic details. > > > Finally I've created a SID_ARM64.tar.xz basefile and ran fai-diskimage > with the new cross-arch support using your config space on a Amd64 > machine which finished in 289 seconds. I used FAI 5.4 with the one > patch I mentioned above. How long does it take on a native Arm64 > machine? 7 minutes without basefile, and 1.5min with. This is on a HPE Moonshot m400 blade. Riku