[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

review of mrvn build tree, part 1



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..

  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.

  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.

  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.

- 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.

- 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.

  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.

  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.

- 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.

- 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.

  "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.

- pkg/lists

  bootfloppy/ and initrd-bootfloppy/ both exist, with identical contents

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.

- arch/linux-m68k contains the comment "Still using the old -udeb.udebs
  for now", but the next line uses the -di version.
  
- 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.
  
- 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.
  
- 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?

- 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?

- 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.

- pcmcia-cs-udeb needs to be moved to net_drivers (more missing merging
  from HEAD).

- 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.

- 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.

- TYPE=hd-media does not work.

- make/arch/linux-i386 ends in a line ending in a "\" continuation
  character, which seems weird.

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.
  
  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.

-- 
see shy jo

Attachment: signature.asc
Description: Digital signature


Reply to: