commit 7b44bef640065d5614c38d6b1cd91731547c26ca
author Benjamin Gordon <bmgordon@chromium.org> Fri Jun 08 08:13:59 2018 -0600
committer chrome-bot <chrome-bot@chromium.org> Sat Jun 09 02:19:24 2018 -0700
tree 260788b472a83ebac4fe6ca6811b9180dbbda367
parent 8c44b9157151c539d00a341857f04c6714492b49

cros_sdk: Simplify create/download logic

Right now options.create can get toggled on and off in a couple of
places, and one of the places also sets options.download through some
tricky logic.  This is really hard to reason about and might be one
source of the bug where a chroot is recreated even when it already
exists.  Simplify this by breaking out a couple of the steps and
ensuring that flags are only ever turned on (not turned back to False).

This leads to a scenario where if --create is explicitly requested and
the chroot already exists, make_chroot.sh will be called again.  To
avoid that, also re-check if the chroot is set up before calling
CreateChroot().

BUG=chromium:834412
TEST=Ran cros_sdk with various flag combinations

Change-Id: Ie8ebb3ee9daef440b2b52ec11d7310566f6a80e9
Reviewed-on: https://chromium-review.googlesource.com/1093024
Commit-Ready: Benjamin Gordon <bmgordon@chromium.org>
Tested-by: Benjamin Gordon <bmgordon@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

scripts/cros_sdk.py

diff --git a/scripts/cros_sdk.py b/scripts/cros_sdk.py
index 7948a75..4977d51 100644
--- a/scripts/cros_sdk.py
+++ b/scripts/cros_sdk.py
@@ -877,8 +877,7 @@
     options.working_dir = path_util.ToChrootPath(options.working_dir)
 
   # Discern if we need to create the chroot.
-  chroot_ver_file = os.path.join(options.chroot, 'etc', 'cros_chroot_version')
-  chroot_exists = os.path.exists(chroot_ver_file)
+  chroot_exists = cros_sdk_lib.IsChrootReady(options.chroot)
   if (options.use_image and not chroot_exists and not options.delete and
       not missing_image_tools and
       os.path.exists(_ImageFileForChroot(options.chroot))):
@@ -888,22 +887,18 @@
         logging.debug('Checking if existing chroot image can be mounted.')
         lock.write_lock()
         cros_sdk_lib.MountChroot(options.chroot, create=False)
-        chroot_exists = os.path.exists(chroot_ver_file)
+        chroot_exists = cros_sdk_lib.IsChrootReady(options.chroot)
         if chroot_exists:
           logging.notice('Mounted existing image %s on chroot',
                          _ImageFileForChroot(options.chroot))
-  if (options.create or options.enter or options.snapshot_create or
-      options.snapshot_restore):
-    # Only create if it's being wiped, or if it doesn't exist.
-    if not options.delete and chroot_exists:
-      options.create = False
-    else:
-      options.download = True
 
   # Finally, flip create if necessary.
   if options.enter or options.snapshot_create:
     options.create |= not chroot_exists
 
+  # Make sure we will download if we plan to create.
+  options.download |= options.create
+
   # Anything that needs to manipulate the main chroot mount or communicate with
   # LVM needs to be done here before we enter the new namespaces.
 
@@ -1091,8 +1086,14 @@
 
       if options.create:
         lock.write_lock()
-        CreateChroot(options.chroot, sdk_tarball, options.cache_dir,
-                     nousepkg=(options.bootstrap or options.nousepkg))
+        # Recheck if the chroot is set up here before creating to make sure we
+        # account for whatever the various delete/unmount/remount steps above
+        # have done.
+        if cros_sdk_lib.IsChrootReady(options.chroot):
+          logging.debug('Chroot already exists.  Skipping creation.')
+        else:
+          CreateChroot(options.chroot, sdk_tarball, options.cache_dir,
+                       nousepkg=(options.bootstrap or options.nousepkg))
 
       if options.enter:
         lock.read_lock()