Re: review of mrvn build tree, part 1
Joey Hess <joeyh@debian.org> writes:
> I've been doing a review of mrvn's branch of the build directory. This
> included reading every line of code or at least diffs from TRUNK,
> builds, comparison of what was put on the images, booting the images,
> etc. So far I have looked only at the floppy images, on i386, and not
> cdroms or other media types.
>
> What is broken:
>
> - images
>
> I build a bootfloppy, and the floppy *image* has a "diskusage.txt"
> file in the top level of the initrd. No, I don't know why..
I had them created in the tree dir so they don't overwrite each
other. Guess I forgot to move them out again. I will use
diskusage-<tree name>.txt again. Will do.
> The bootfloppy has space wasted in etc with a etc/terminfo. Apparently
> this is because the new tree does away with the config/type
> directories, which included NO_TERMINFO=1 for this image type.
Support is still there, just needs to be set. Will do.
> Comparing the second floppy images, the one build by mrvn's tree is
> missing lib/libnss_dns.so.2, libresolv.so.2. Not sure why, I assume
> this will make it hard to do a network install with that floppy
> though. These also have the diskusage.txt in toplevel.
That was added with the EXTRALIBS variable? So I guess its not working
anymore. Will do.
> Pcmcia config reduction is broken in mrvn's tree, the generated
> etc/pcmcia/config has no entries for pcmcia devices. Probably
> pcmcia-config-reduce.pl is run against the wrong directory, or it is
> not being told about kernel modules in the drivers floppys, or
> something.
Thats either just broken or its due to a missing extras-... pkg-list.
I'm rethinking the extras stuff currently and I will look out for that
when I do it.
> - Makefile
>
> The sources.list.* generation will break if /etc/apt/sources.list
> contains an entry for security.debian.org. I fixed this in TRUNK a few
> days ago.
Huh, I thought I had all the changes between 13. and 26. merged
in. Didn't look too closely at the Makefile though.
> - build/apt-get:
>
> Lists all the libraries that are also in the build-depends,
> in some kind of apt hack-around. This will be painful to maintain
> in two places. There is no indication of why this is necessary,
> and I do not belive it should be necessary. The old tree gets apt to
> work without this gross hack.
Do you mean this part?
----------------------------------------------------------------------
echo -n > $APTDIR/state/status
for i in $LIBC_NAME libnewt0.51 libdebconfclient0 libdebian-installer4 \
libdb1-compat libpopt0 slang1a-utf8 libuuid1 libparted1.6-0; do
dpkg -s $i >> $APTDIR/state/status
done
----------------------------------------------------------------------
Somehow that was neccessary to get apt to download debs or so I
thought. I just commented that out and did a fresh build without
error.
> Though there are now two sources.lists (.deb, .udeb),
> sources.list.local is still used as the sole override for both. I
> don't see how that can possibly work.
You put a source for main and main/debian-installer in .local and it
finds everything.
root@dual:/d-i/build# cat sources.list.local
deb ftp://dual/debian sid main
deb ftp://dual/debian-installer sid main/debian-installer
The split between .deb and .udeb is just for you so you don't have to
download the main/binary-arch/Packages file over modem to build
floppies. Your request made the split neccessary. I can split the
.local file too if you like.
> I have a full set of linux-kernel-di udebs in localudebs, but it
> insists in trying to download old versions of these from the mirror.
> TRUNk does not do this, it skips downloading udebs that have a version
> present in localudebs.
Thats a bug in the code responsible for removing localudebs from the
list of needed packages. The code only removes the first appearance of
a package from the needed list in the assumption there are no
duplicates. But there are. I had the same problem on m68k. I will see
if I can get it to remove all appearances, probably just a missing "g"
on a sed.
> - dest/
>
> The names used for images in dest/ are repetative and/or make no real
> sense. Eg, "floppy-boot-bootfloppy.img", "floppy-root-root.img". These
> names need to be cleaned up to something human-readable.
>
> Useless initrds are included in dest (example: initrd-bootfloppy.img).
>
> Initrds in dest are .gz files, but their extension does not end in
> .gz, but in ".img". This will be confusing.
How about the following release names:
floppy/bootfloppy.img
floppy/rootfloppy.img
floppy/drivers_net.img
floppy/drivers_cd.img
network/linux-<flavour> (amiga, atari, ...)
network/initrd-<flavour>.gz
hd-media/linux-<flavour>
hd-media/initrd-<flavout>.gz
cdrom/cdromboot.iso (bad name. Its a cdrom for when netboot
doesn't work)
cdrom/netinstall.iso (12MB with udebs)
cdrom/businesscard.iso (50MB with udebs and base)
kernel/linux-<flavour> (to be linked into net and hd)
initrd/initrd-<flavour>.gz
> - build/tmp/
>
> After a "make TYPE=floppy", I have directories like
> tmp/tree-floppy-boot-boot, but the actual tree has apparently been
> removed from these directories after the initrd was made. Those trees
> are very useful for debugging and should be retained until make clean.
root@dual:/d-i/build# du -h --max-depth 1 tmp
4.8M tmp/tree-initrd-boot
2.3M tmp/tree-cdrom-boot-none-none
2.3M tmp/tree-floppy-boot-boot
4.8M tmp/tree-initrd-udeb
12M tmp/tree-cdrom-udeb-all-none
2.3M tmp/tree-floppy-boot-udeb
4.8M tmp/tree-initrd-base
52M tmp/tree-cdrom-base-all-base
2.3M tmp/tree-floppy-boot-base
4.4M tmp/tree-initrd-netboot
5.2M tmp/tree-initrd-demo
6.9M tmp/tree-initrd-hd_media
1.1M tmp/tree-floppy-data-drivers_cd
1.2M tmp/tree-floppy-data-drivers_net
1.6M tmp/tree-initrd-bootfloppy
1.4M tmp/tree-floppy-boot-bootfloppy
4.2M tmp/tree-initrd-root
1.4M tmp/tree-floppy-root-root
root@dual:/d-i/build# ls tmp/tree-floppy-boot-boot
initrd.gz linux syslinux.cfg
Works here.
> "tmp/tree-floppy-boot-boot" is a stupid name, and should match
> whatever image is produced in dest/, once the names in dest/ are
> cleaned up.
The tree name can be (actually must be) set with TREE_DIR before
calling the tree-%: target. Its changeable but then it must be passed
along with the target names in some way. Currently the
initrd/floppy/cdrom targets generate the name. Let me think about that.
The floppy-...img, initrd-...img, cdrom-...img names are hard to
change though since, if they don't math the target name in the Makefile,
they will be rebuild every time.
> - pkg/lists
>
> bootfloppy/ and initrd-bootfloppy/ both exist, with identical contents
Bootfloppy is leftover from HEAD I guess.
> Nitpicks:
>
> - build/apt-get is a confusing name. It even makes the program hard to
> refer to (see above). If someone's path unwisely had "." at the front
> of it, or does not have an apt-get in PATH, this script would call itself
> instead of the real apt. Should be renamed, perhaps to get_packages.
Ok.
> - arch/linux-m68k contains the comment "Still using the old -udeb.udebs
> for now", but the next line uses the -di version.
2 out of 3 archs are. :)
Worked on making -di work for m68k flavours? I had to link
2.4.20-amiga -> 2.4.20 for the amiga -di.udebs as I told you on irc.
Problem here is that we have flavours and each flavour has a different
version. Real messy.
> - arch/linux has the commented out INITRD_FS=ext2 which I removed from
> build/ last week. Probably symptomatic of an incomplete merge to
> trunk.
>
> - arch/linux-alpha is missing the "Still using the old -udeb.udeb's, for
> now.." comment that I added last week.
Awaiting your -di alpha udebs :)
> - Creates files named sources.list.deb and sources.list.udeb. Both of
> these file extensions have special meansings, and this can lead to
> confusing/annoying behavior.
As I said, you forced the split. Its either download main and
main/debian-instaler or the auto-clean will delete any cached
udebs/debs depending on what it fetches.
> - The makefile has a target in it named mirror-check-foo, with no
> documentation. This target needs a better name, documentation, etc.
> Nothing seems to depend on this target. Remove?
WIP, leftover from round 2 to be used to verify the cdrom trees.
> - The Makefile has a empty "guess-local" target, with the comment
> "Guessing of local config". I have no idea what the point is, this
> target seems to be unreferenced. Remove?
yep.
> - The README refers to "make listtypes", but that target does not exist.
>
> - The README refers to "make sources.list", when that has been renamed
> to "sources.list.udeb". However, I think this step should be removed
> from the makefile entirely, as it happens automatically now.
>
> - The README goes on to refer to sources.list some more, and
> sources.list.local.
README is from HEAD.
> - pcmcia-cs-udeb needs to be moved to net_drivers (more missing merging
> from HEAD).
I copied the HEAD pkg-lists to net_drivers. Must have been changed
after I updated.
> - apt-style "I:", "E:", etc messages are Evil, and I hate to see more
> code outputting them in here, but this is just my opinion.
What about FYI?
> - The Makefile uses a variable named "FOOBAR" that is set by a $(shell)
> that makes some directories. What an obfuscated way to do that; a
> proper way to do it would be to have a target that made directories,
> and make targets that need the dirs depend on it.
>
> - EXTRAFILES is not "Old cruft" (Makefile comment)
>
> - The README refers to "per-arch and per-image fragments" in the config
> dir; only per-arch fragments remain in this tree (where did the
> per-image config go?)
>
> - The README refers to pkg-lists/TYPE/ and to TYPE=, which is the wrong
> terminology in this new tree.
>
> - If a TYPE is given that it does not know about, it silently decides to
> build all possible image types instead, which is a bit confusing.
> config/targets needs an else block that error out if TYPE is set to
> something unnown.
Will fix.
> - TYPE=hd-media does not work.
Will fix.
Do we actually want to keep using TYPE=xxx? Do we want or need
config/TYPE/?
I was thinking of making a list:
#filename arch type target
#----------------------------------------------------------------------
dest/floppy/bootfloppy.img i386,alpha floppy tmp/floppy-boot-bootfloppy.img
dest/cdrom/businesscard.iso all cdrom tmp/cdrom-base-all-base.img
If TYPE= is given the list would be filtered for the type.
If no line with the right arch or all remains the makefile refuses to
build that TYPE.
If no or TYPE=all is given all targets for that arch are build.
Alterntaively to the list a debian/control style format could be used
adding a description and other stuff to it. Maybe even a real
debian/control file that can be used to autobuild.
> - make/arch/linux-i386 ends in a line ending in a "\" continuation
> character, which seems weird.
right, fixed.
> What worked:
>
> - "make TYPE=floppy", even in offline mode with a populated apt cache.
>
> - Booting the 2 floppy set from bochs, (mostly) loading modules from
> driver floppies.
>
> What's better than the old tree:
>
> - udeb_include lists on the driver floppies get the wrong names for
> kernel udebs (missing "-di") in the old tree. This turns out the be
> fixed in mrvn's tree.
>
> - Breaking out standalone programs, eg build/initrd_install made the
> Makefile much easier to follow.
>
> <rant>
> However, in these new standalone programs, once again someone has
> taken sanely indented code[1] that I wrote for d-i, and wrecked the
> indentation with their own idea of what's "right". You're welcome to
> use any indentation you like in your own code, but I consider
> reindenting someone else's code behind their back to be quite rude,
> especially if they're still actively maintaining it. Frankly, this
> reindentation nonsense has been one of the more annoying things for me
> as I have came back to working on d-i, and while it was excusable to
> eg, reindent main-menu and anna when I was not working on them
> anymore, that excuse does not apply here. Revert pls.
Aehm, reindenting to Makefile syntax? You want a "1 tab per indent"
indentation?
> While I'm ranting, I want to mention that some people's idea of proper
> indentation in shell scripts does not compress as well as it could.
>
> 2720 hw-detect.sh.current-(random)-indent.gz
> 2676 hw-detect.sh.tabs-for-indentation-spaces-for-formatting.gz
>
> Those bytes will add up, it's something to keep in mind.
>
> [1] Code that looks properly indented at any tab setting, in any text
> editor/viewer that uses a fixed width font.
> </rant>
>
> TODO: look at hd-media, and cdrom images. Try to boot hd-media. Compare
> them with images from HEAD.
If you find differences in the pkg-lists or if you change them in head
please fix them in mrvn too.
MfG
Goswin
PS: I have changed a few things while writing this already and will do
more. Give me some time to test it though beforeI check it in.
Reply to: