This one time, at band camp, Frans Pop said: > On Sunday 23 December 2007, Stephen Gran wrote: > > Please do _not_ apply this patch without review. This patch serves two > > purposes: the first, and the one most useful to remember is that it > > serves as a way to introduce me to the code base. The second purpose is > > to reduce the size of the code base, and make things slightly more > > unified and canonical. > > Thanks for the patch. Saving about 300 lines of code looks worth the effort. > > Have you followed the discussion on your original thread, especially the > part about updating/adding dependencies on di-utils? > > Some comments on the patch. I've not done a full review, but I have taken a > fairly close look :-) > > * I warned you about partman: it already has its own log() function and your > patch conflicts with that (see partman-base/lib/base.sh). It's really best > to initially leave all partman components out of this change. Damn. I thought I'd managed to skip the partman part of it, but I see now that I didn't. Not quite sure what happened, sorry. > * Not sure if "output.sh" is a very logical name for this. Seems to general. Probably so. I wasn't feeling too inventive when I created the file. I was thinking along the lines of stdio.h (although it wraps syslog functions and not print functions, it seems to be a fairly common way to communicate in the d-i environment. Feel free to rename to something better if you end up using soem of the patch. > * I'd define the functions in order of "severity": info before warning Fair enough. My more long term plan was to create a list of functions available in the d-i environemnt so that people new to the project don't keep reinventing the wheel. > * The "die" function. > That function seems somewhat out of place with the others and its name does > not really indicate that it also displays an error dialog (maybe > "exit_error" as it was called in some places would be better?). I happened on implementations of the function named die before I happened on ones named exit_error, and I was trying to keep the diff minimal. Although I think you're right, the latter name is better. > It is used in quite a few places, so factoring it out does seem logical. > > At least in one place (base-installer) we lose the error that was printed to > syslog with the new function. > > I'm not really sure what to do here. Me either. I went ahead and did the changes, so that it would be in the diff and visible for discussion, but it didn't seem like the right change there. > * debian-installer-utils/in-target > Not sure about the change there. Note the "--" in the command, which was > added to avoid problems with some error strings. > Does that still work? Should that maybe be included in the new log() > function by default instead? Yes, I think that makes sense. > * finish-install/finish-install.d/90console > You mangled the indentation in the first hunk. Arg. > * apt-setup > That udeb is somewhat intended to also be used in installed systems (see the > use of the $ROOT variable). Having its scripts depend on a d-i function > library is therefore probably not a good idea. > > * os-prober > Same, but that udeb already _has_ a regular package equivalent! > > * debian-installer-utils/chroot-setup.sh > Note that this file itself is already sourced, so the chance is pretty high > that we'll be sourcing the new file twice (at least from in-target). > Not sure if that'd need fixing, but certainly worth a comment. Right, these are the sorts of architectural issues I knew I would screw up. I don't think re-sourceing the same file will create any problems (you'll overwrite the previous definition of log() with the new, same definition of log()). As for the others, yes, they should probably be left alone. Thanks, -- ----------------------------------------------------------------- | ,''`. Stephen Gran | | : :' : sgran@debian.org | | `. `' Debian user, admin, and developer | | `- http://www.debian.org | -----------------------------------------------------------------
Attachment:
signature.asc
Description: Digital signature