curtin-dev team mailing list archive

[Michael Hudson-Doyle <mp+450290@xxxxxxxxxxxxxxxxxx>]

Michael Hudson-Doyle has proposed merging ~mwhudson/curtin:simplify-apt_update-more into curtin:master.

Commit message:
distro: remove even more code from apt_update

The existing code tries to avoid redundant calls to 'apt update' but it
has flaws and redundant calls to 'apt update' should be fairly quick
anyway.



Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/450290
-- 
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:simplify-apt_update-more into curtin:master.

diff --git a/curtin/commands/apt_config.py b/curtin/commands/apt_config.py
index 9c8e543..f045163 100644
--- a/curtin/commands/apt_config.py
+++ b/curtin/commands/apt_config.py
@@ -798,8 +798,7 @@ def add_apt_sources(srcdict, target=None, template_params=None,
             LOG.exception("failed write to file %s: %s", sourcefn, detail)
             raise
 
-    distro.apt_update(target=target, force=True,
-                      comment="apt-source changed config")
+    distro.apt_update(target=target)
 
     return
 
diff --git a/curtin/distro.py b/curtin/distro.py
index 622a520..f608112 100644
--- a/curtin/distro.py
+++ b/curtin/distro.py
@@ -1,5 +1,4 @@
 # This file is part of curtin. See LICENSE file for copyright and license info.
-import glob
 from collections import namedtuple
 import os
 import re
@@ -9,7 +8,6 @@ from typing import Optional, Sequence
 from .paths import target_path
 from .util import (
     ChrootableTarget,
-    find_newer,
     load_file,
     load_shell_content,
     ProcessExecutionError,
@@ -196,35 +194,11 @@ def _lsb_release(target=None):
     return data
 
 
-def apt_update(target=None, env=None, force=False, comment=None,
-               retries=None):
-
-    marker = "tmp/curtin.aptupdate"
-
+def apt_update(target=None, env=None):
+    LOG.debug("Updating apt sources in %s", target)
     if env is None:
         env = os.environ.copy()
 
-    if retries is None:
-        # by default run apt-update up to 3 times to allow
-        # for transient failures
-        retries = (1, 2, 3)
-
-    if comment is None:
-        comment = "no comment provided"
-
-    if comment.endswith("\n"):
-        comment = comment[:-1]
-
-    marker = target_path(target, marker)
-    # if marker exists, check if there are files that would make it obsolete
-    listfiles = [target_path(target, "/etc/apt/sources.list")]
-    listfiles += glob.glob(
-        target_path(target, "etc/apt/sources.list.d/*.list"))
-
-    if os.path.exists(marker) and not force:
-        if len(find_newer(marker, listfiles)) == 0:
-            return
-
     restore_perms = []
 
     try:
@@ -242,14 +216,11 @@ def apt_update(target=None, env=None, force=False, comment=None,
 
         # do not using 'run_apt_command' so we can use 'retries' to subp
         with ChrootableTarget(target, allow_daemons=True) as inchroot:
-            inchroot.subp(update_cmd, env=env, retries=retries)
+            inchroot.subp(update_cmd, env=env, retries=(1, 2, 3))
     finally:
         for fname, perms in restore_perms:
             os.chmod(fname, perms)
 
-    with open(marker, "w") as fp:
-        fp.write(comment + "\n")
-
 
 def run_apt_command(mode, args=None, opts=None, env=None, target=None,
                     execute=True, allow_daemons=False, clean=True,
@@ -278,7 +249,7 @@ def run_apt_command(mode, args=None, opts=None, env=None, target=None,
         return env, cmd
 
     if not assume_downloaded:
-        apt_update(target, env=env, comment=' '.join(cmd))
+        apt_update(target, env=env)
     if mode in ['dist-upgrade', 'install', 'upgrade']:
         cmd_rv = apt_install(mode, args, opts=opts, env=env, target=target,
                              allow_daemons=allow_daemons,
diff --git a/curtin/util.py b/curtin/util.py
index 005a521..3e54795 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -882,12 +882,6 @@ def get_paths(curtin_exe=None, lib=None, helpers=None):
     return {'curtin_exe': curtin_exe, 'lib': mydir, 'helpers': helpers}
 
 
-def find_newer(src, files):
-    mtime = os.stat(src).st_mtime
-    return [f for f in files if
-            os.path.exists(f) and os.stat(f).st_mtime > mtime]
-
-
 def set_unexecutable(fname, strict=False):
     """set fname so it is not executable.
 
diff --git a/tests/unittests/test_distro.py b/tests/unittests/test_distro.py
index 400b4ae..5fac420 100644
--- a/tests/unittests/test_distro.py
+++ b/tests/unittests/test_distro.py
@@ -595,8 +595,7 @@ class TestSystemUpgrade(CiTestCase):
             mock.call(auto_remove, env=env, target=paths.target_path(target)),
         ]
         which_calls = [mock.call('eatmydata', target=target)]
-        apt_update_calls = [
-            mock.call(target, env=env, comment=' '.join(apt_cmd))]
+        apt_update_calls = [mock.call(target, env=env)]
 
         distro.system_upgrade(target=target, osfamily=osfamily)
         m_which.assert_has_calls(which_calls)