Code review comment for lp:~mterry/ubiquity/oem-config-merge

Generally speaking I think this is very good - definitely along something very close to the right lines. I have a few miscellaneous comments:

 * UBIQUITY_OEM_USER_CONFIG is a bit verbose as an environment variable name (and "OEM user config" isn't a phrase I've ever used in connection with oem-config, personally). How about just UBIQUITY_OEM?

 * Use 1.99 rather than 1.99.0 in configure.ac, so that we don't have to rerun autoconf for every minor revision.

 * Since 1.13.8 is currently UNRELEASED, you should merge its changelog entries into those for 1.99.0 rather than leaving 1.13.8 as a stub. (Of course this will change if 1.13.8 is released before we merge this branch.)

 * I think you need to rerun debconf-updatepo. I noticed some diffs there that seemed to be going backwards ("Erase and use the entire disk" replaced by "Use the entire disk").

 * There needs to be an oem-config-debconf package, even if it's only a stub; finish-install.d/01oem-config-udeb relies on that name, as do existing seeds. I think it's a good idea to have that interface anyway.

 * I'm not too surprised that ubiquity-frontend-debconf doesn't work; oem-config-debconf only works at all by some exceedingly careful debconf frontend instance handling, and it's entirely possible that its design relies on properties that simply don't hold in ubiquity. I wouldn't advertise the existence of a ubiquity debconf frontend too much as yet, nor would I be inclined to worry excessively about problems with it.

 * I think it may be hard to justify parameterising /target in some of the d-i scripts in question, at least for patch submission to Debian. I'm happy enough to keep those as modifications local to ubiquity, slightly ugly though it is.

 * I'd rather have netcfg in oem-config than oem-config-debconf (or ubiquity-frontend-debconf), even though it's only used in the debconf frontend right now; in general I would prefer frontend packages not to deliver component-type code as well. (ubiquity-frontend-mythbuntu is a weird exception, perhaps best kept that way.)

 * Whichever package contains netcfg needs to be Architecture: any, and built in *-arch targets in debian/rules; netcfg is written in C.

 * Don't hardcode the path to the hostname program.

 * set_debconf could be done with a little less duplication, I think - if in OEM mode, set dccomm = None and dc = self.db, and refrain from closing dccomm at the end. That way the code that actually talks to the debconf instance is shared.

 * Perhaps OEM-only guards would be useful in the network and tasks components? Similarly, I think it would be best to leave them out of ubiquity/steps Choices until they actually work properly in ubiquity's environment.

 * I'm not convinced that we should set OVERRIDE_SYSTEM_USER outside OEM mode; the only reason we set that is to cope with the fact that the temporary oem user looks like a "system user" (bad naming; it means "ordinary user of the system with uid 1000-29999", which has no overlap with 'adduser --system'!).

 * Why does language_label need an entry in string_questions? I don't see that identifier anywhere else.