Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3140028:
Better build_image for factory install (Closed)
|
Created:
10 years, 4 months ago by Nick Sanders Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Base URL:
ssh://gitrw.chromium.org/crosutils.git Visibility:
Public. |
DescriptionAllow full build of factory install shim in build_image.
BUG=4951
TEST=Build and boot install shim on legacy bios, on H2C
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 30
Patch Set 5 : . #
Total comments: 5
Patch Set 6 : add quotes #Messages
Total messages: 11 (0 generated)
This change does: * removes the "mod_image_for_test --factory_install" step * resizes factory shim to be smaller than tmpfs for initramfs * allows use flag for initramfs This change doesn't: * work with verified rootfs * create a working install shim - there's a bunch of places where the installer and updater look at the SRC device, which fail now. Discussed with davidjames, there may be a better way to enforce kernel use flags/build config Should we leave the mod_image_for_test so that buildbots can sort of generate install shims until the unified build_image is in? Also, disregard kernel params, it's not possible to rebase without commiting them.. =( Will be removed before checkin
Lots of comments below. Would be interested in comments from vlaviano as well since he's refactoring this code significantly. http://codereview.chromium.org/3140028/diff/7001/5002 File build_image (right): http://codereview.chromium.org/3140028/diff/7001/5002#newcode100 build_image:100: INSTALL_MASK="${DEFAULT_INSTALL_MASK}" Why do we initialize this twice? I see the same code on line 210. http://codereview.chromium.org/3140028/diff/7001/5002#newcode106 build_image:106: INSTALL_MASK="${INSTALL_MASK} ${FACTORY_INSTALL_MASK}" Same question here. Isn't this already happening on line 217? http://codereview.chromium.org/3140028/diff/7001/5002#newcode107 build_image:107: EXTRA_PACKAGES="chromeos-base/chromeos-factoryinstall kernel" kernel isn't needed here -- it's already in the base image http://codereview.chromium.org/3140028/diff/7001/5002#newcode108 build_image:108: EXTRA_USE="initramfs" Don't need this -- just inline it in the one place it's used. http://codereview.chromium.org/3140028/diff/7001/5002#newcode110 build_image:110: EXTRA_FLAGS="--newuse --binpkg-respect-use y --changed-use" No need for this line. We already pass -N to parallel_emerge, which implies binpkg-respect-use and changed-use. http://codereview.chromium.org/3140028/diff/7001/5002#newcode114 build_image:114: FLAGS_withdev="${FLAGS_FALSE}" Does it make sense to just silently change the withdev flag here? Or should we have incompatible flags messages like we do below? http://codereview.chromium.org/3140028/diff/7001/5002#newcode455 build_image:455: --usepkg ${EXTRA_FLAGS} -uDNv chromeos ${EXTRA_PACKAGES} ${EMERGE_JOBS} EXTRA_FLAGS are not needed here. http://codereview.chromium.org/3140028/diff/7001/5002#newcode588 build_image:588: # Freshen kernel. This is a noop if we have the right kernel. Can you add more comments here? http://codereview.chromium.org/3140028/diff/7001/5002#newcode589 build_image:589: # TODO: --binpkg-respect-use=y is broken in parallel_emerge Can you file a bug for this problem and reference it here? Assign the bug to me if you like. http://codereview.chromium.org/3140028/diff/7001/5002#newcode590 build_image:590: emerge-${FLAGS_board} -pvg --newuse --binpkg-respect-use=y kernel Oops, I don't think you want pretend mode here. And where are the use flags? How about USE=${EXTRA_USE} emerge-${FLAGS_board} -uDNvg kernel http://codereview.chromium.org/3140028/diff/7001/5002#newcode597 build_image:597: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" ${EMERGE_BOARD_CMD} \ Can you add comments for why this USE line is required, even with the workaround above? I guess the issue here is that the emerge might overwrite the kernel we installed earlier with a new kernel with the wrong use flags. But a comment will help because I missed that the first time. http://codereview.chromium.org/3140028/diff/7001/5002#newcode599 build_image:599: --usepkg ${EXTRA_FLAGS} chromeos ${EXTRA_PACKAGES} ${EMERGE_JOBS} Could we get rid of EXTRA_FLAGS and just pass the flags you want unconditionally? -uDNv is fine to use globally
http://codereview.chromium.org/3140028/diff/7001/5002 File build_image (right): http://codereview.chromium.org/3140028/diff/7001/5002#newcode108 build_image:108: EXTRA_USE="initramfs" On 2010/08/26 16:27:04, David James wrote: > Don't need this -- just inline it in the one place it's used. Oops. Disregard that comment -- we need this, as discussed below (we just need comments explaining why the emerge of updated packages needs the EXTRA_USE)
Few small comments. http://codereview.chromium.org/3140028/diff/7001/5002 File build_image (right): http://codereview.chromium.org/3140028/diff/7001/5002#newcode107 build_image:107: EXTRA_PACKAGES="chromeos-base/chromeos-factoryinstall kernel" Can you rename this to something more specific for its use e.g. FACTORY_INSTALL_PACKAGES= http://codereview.chromium.org/3140028/diff/7001/5002#newcode114 build_image:114: FLAGS_withdev="${FLAGS_FALSE}" Please fail if withdev as opposed to changing the flag and check for this incompatibility earlier. On 2010/08/26 16:27:04, David James wrote: > Does it make sense to just silently change the withdev flag here? Or should we > have incompatible flags messages like we do below? http://codereview.chromium.org/3140028/diff/7001/5002#newcode589 build_image:589: # TODO: --binpkg-respect-use=y is broken in parallel_emerge Please assign a name in the TODO e.g. "TODO(nsanders | davidjames):" On 2010/08/26 16:27:04, David James wrote: > Can you file a bug for this problem and reference it here? Assign the bug to me > if you like.
http://codereview.chromium.org/3140028/diff/7001/5002 File build_image (right): http://codereview.chromium.org/3140028/diff/7001/5002#newcode106 build_image:106: INSTALL_MASK="${INSTALL_MASK} ${FACTORY_INSTALL_MASK}" On 2010/08/26 16:27:04, David James wrote: > Same question here. Isn't this already happening on line 217? I suspect that it was to group all of the factory_install settings in one place under one conditional. That being said, it should only be done once, and I'd prefer that we leave it on lines 208-218. http://codereview.chromium.org/3140028/diff/7001/5002#newcode107 build_image:107: EXTRA_PACKAGES="chromeos-base/chromeos-factoryinstall kernel" On 2010/08/26 16:58:33, sosa wrote: > Can you rename this to something more specific for its use e.g. > FACTORY_INSTALL_PACKAGES= I think that EXTRA_PACKAGES is reasonable since it is used in an emerge command that is executed for all image types. Though, to future proof it, we maybe want: EXTRA_PACKAGES= ... if (factory_install) EXTRA_PACKAGES="${EXTRA_PACKAGES} chromeos-base/chromeos-factoryinstall" http://codereview.chromium.org/3140028/diff/7001/5002#newcode112 build_image:112: FLAGS_rootfs_size="280" These are set again on lines 684-688. Those settings will supersede these. I get that you did it here because there's a check for rootfs size > root partition size just below, but I think that it would be better to put a similar check down there. In my pending CL, I don't overwrite the flags when building image A because I may later want the original flag values when building image B. Instead, I define env vars ROOT_FS_SIZE and STATEFUL_FS_SIZE and initialize them from the flags. http://codereview.chromium.org/3140028/diff/7001/5002#newcode114 build_image:114: FLAGS_withdev="${FLAGS_FALSE}" On 2010/08/26 16:58:33, sosa wrote: > Please fail if withdev as opposed to changing the flag and check for this > incompatibility earlier. I agree that we should fail. This should probably go with similar checks on lines 125-141 (all of which my pending CL removes because the various flags will be compatible with one another and produce different images). But, rather than checking earlier, I think that most of the new stuff above ought to happen elsewhere (or already happens elsewhere). > > On 2010/08/26 16:27:04, David James wrote: > > Does it make sense to just silently change the withdev flag here? Or should we > > have incompatible flags messages like we do below? > >
Thanks for looking it over! http://codereview.chromium.org/3140028/diff/7001/5002 File build_image (right): http://codereview.chromium.org/3140028/diff/7001/5002#newcode100 build_image:100: INSTALL_MASK="${DEFAULT_INSTALL_MASK}" On 2010/08/26 16:27:04, David James wrote: > Why do we initialize this twice? I see the same code on line 210. Arg, I deleted the lines below but they snuck back in somehow. will fix. http://codereview.chromium.org/3140028/diff/7001/5002#newcode106 build_image:106: INSTALL_MASK="${INSTALL_MASK} ${FACTORY_INSTALL_MASK}" partition sizes must be set here before they are used a few lines down. I intended to move everything up here so it would all be in one spot. http://codereview.chromium.org/3140028/diff/7001/5002#newcode107 build_image:107: EXTRA_PACKAGES="chromeos-base/chromeos-factoryinstall kernel" On 2010/08/26 16:58:33, sosa wrote: > Can you rename this to something more specific for its use e.g. > FACTORY_INSTALL_PACKAGES= My intent was to reuse EXTRA_PACKAGES for factory test and dev installer, maybe others. I can change it to be specific, but we may end up with an env var for each on the emerge command then. http://codereview.chromium.org/3140028/diff/7001/5002#newcode107 build_image:107: EXTRA_PACKAGES="chromeos-base/chromeos-factoryinstall kernel" > > FACTORY_INSTALL_PACKAGES= will fix initialization. http://codereview.chromium.org/3140028/diff/7001/5002#newcode108 build_image:108: EXTRA_USE="initramfs" will do. http://codereview.chromium.org/3140028/diff/7001/5002#newcode110 build_image:110: EXTRA_FLAGS="--newuse --binpkg-respect-use y --changed-use" Sounds good. http://codereview.chromium.org/3140028/diff/7001/5002#newcode114 build_image:114: FLAGS_withdev="${FLAGS_FALSE}" The issue is that dev_mode defaults to true, and simply spends an extra couple minutes building a useless "dev_mode shim" along with the base image shim. Is there a way to make dev_mode default to false if factory install is true? Adding a check here simply makes the command line longer and more confusing. Allowing it is fine, but then the default value doubles build time and wastes disk space. This will all be nicer when Vince's change goes through and we can build a shim and an actual dev image in the same pass. http://codereview.chromium.org/3140028/diff/7001/5002#newcode455 build_image:455: --usepkg ${EXTRA_FLAGS} -uDNv chromeos ${EXTRA_PACKAGES} ${EMERGE_JOBS} On 2010/08/26 16:27:04, David James wrote: > EXTRA_FLAGS are not needed here. will fix
http://codereview.chromium.org/3140028/diff/7001/5002 File build_image (right): http://codereview.chromium.org/3140028/diff/7001/5002#newcode107 build_image:107: EXTRA_PACKAGES="chromeos-base/chromeos-factoryinstall kernel" SGTM On 2010/08/26 20:18:03, Nick Sanders wrote: > On 2010/08/26 16:58:33, sosa wrote: > > Can you rename this to something more specific for its use e.g. > > FACTORY_INSTALL_PACKAGES= > > My intent was to reuse EXTRA_PACKAGES for factory test and dev installer, maybe > others. I can change it to be specific, but we may end up with an env var for > each on the emerge command then. http://codereview.chromium.org/3140028/diff/7001/5002#newcode114 build_image:114: FLAGS_withdev="${FLAGS_FALSE}" Can you just check earlier? i.e. as a convention check for incompatible flags as the first thing you do after parsing flags? On 2010/08/26 20:18:03, Nick Sanders wrote: > The issue is that dev_mode defaults to true, and simply spends an extra couple > minutes building a useless "dev_mode shim" along with the base image shim. Is > there a way to make dev_mode default to false if factory install is true? > > Adding a check here simply makes the command line longer and more confusing. > Allowing it is fine, but then the default value doubles build time and wastes > disk space. > > This will all be nicer when Vince's change goes through and we can build a shim > and an actual dev image in the same pass.
I'll break this up into more fully baked smaller CLs
Ok, this CL is now simplified down to just adding the initramfs+kernel/factoryinstall packages. About the call to emerge to build the kernel package, any preference? http://codereview.chromium.org/3140028/diff/17001/18001 File build_image (right): http://codereview.chromium.org/3140028/diff/17001/18001#newcode577 build_image:577: if [ ${FLAGS_factory_install} -eq ${FLAGS_TRUE} ] ; then This will prevent anything from being run in the normal case, however it will not build a correct image if a normal image is built after a shim. Should I remove the if [ factory_install ], or leave it?
LGTM w/suggestions http://codereview.chromium.org/3140028/diff/17001/18001 File build_image (right): http://codereview.chromium.org/3140028/diff/17001/18001#newcode136 build_image:136: EXTRA_USE="initramfs" How about running the emerge here? http://codereview.chromium.org/3140028/diff/17001/18001#newcode577 build_image:577: if [ ${FLAGS_factory_install} -eq ${FLAGS_TRUE} ] ; then On 2010/09/01 10:07:09, Nick Sanders wrote: > This will prevent anything from being run in the normal case, however it will > not build a correct image if a normal image is built after a shim. > > Should I remove the if [ factory_install ], or leave it? Remove it. http://codereview.chromium.org/3140028/diff/17001/18001#newcode583 build_image:583: USE="${EXTRA_USE}" emerge-${FLAGS_board} -uNDv --binpkg-respect-use=y kernel A few suggestions: 1. We should emerge kernel regardless, so that if you run first with factory install and second with a regular image, the regular image will contain the right kernel (and it won't need to rebuild it during the install phase, which could be bad). 2. We should add -g here so that we can use binary packages if they are available. They will only be available for the non-factory case right now.
LGTM w/ a suggestion. http://codereview.chromium.org/3140028/diff/17001/18001 File build_image (right): http://codereview.chromium.org/3140028/diff/17001/18001#newcode135 build_image:135: EXTRA_PACKAGES="chromeos-base/chromeos-factoryinstall" EXTRA_PACKAGES="${EXTRA_PACKAGES} chromeos-base/chromeos-factoryinstall" (and similar for EXTRA_USE) might be better so that, if this code is shuffled around in the future, we don't overwrite packages or use flags that might be set by other compatible build_image flags. |
