On Fri, 2012-07-06 at 12:05 -0400, Don Zickus wrote:
> In order to add git as a fetcher, I needed to re-arrange somethings. Git has
> different expectations for file downloading than autoserver's http.
>
> The first expectation is git doesn't use .tar.bz2, instead it has .tar or
> .zip. This is covered by the install_pkg_setup command.
>
> The second expectation is git unpacks the tarball with directory information
> inside it whereas autoserver's http expects the directory to be created for
> it.
> This is covered by the install_pkg_post command.
>
> Most of this is copy-n-paste with minor changes to pass the fetcher around.
Ok, this refactor looks good, 2 minor comments below:
> Signed-off-by: Don Zickus <dzic...@redhat.com>
> ---
> client/shared/base_packages.py | 91
> +++++++++++++++++++++++++++-------------
> 1 files changed, 61 insertions(+), 30 deletions(-)
>
> diff --git a/client/shared/base_packages.py b/client/shared/base_packages.py
> index 675e0d6..12e4df1 100644
> --- a/client/shared/base_packages.py
> +++ b/client/shared/base_packages.py
> @@ -114,6 +114,21 @@ class RepositoryFetcher(object):
> url = None
>
>
> + def install_pkg_setup(self, name, fetch_dir, install):
> + """ Install setup for a package based on fetcher type.
> + @param name: The filename to be munged
> + @param fetch_dir: The destination path to be munged
> + @install: Whether this is be called from the install path or not
> + """
> + if install:
> + pkg_name = self.pkgmgr.get_tarball_name(name, pkg_type)
^ The parameter pkg_type is not referenced [1], causing a NameError if
this gets executed. So this method's interface must include pkg_type as
well.
[1] pkg_type can be one of 'test', 'client' or 'profiler', if I'm not
mistaken. I've just realized that this is not documented in the
docstrings, so +1 fix for the TODO list on this work.
> + fetch_path = os.path.join(fetch_dir, pkg_name)
> + else:
> + pkg_name = name
> + fetch_path = fetch_dir
> +
> + return (pkg_name, fetch_path)
> +
> def fetch_pkg_file(self, filename, dest_path):
> """ Fetch a package file from a package repository.
>
> @@ -124,6 +139,36 @@ class RepositoryFetcher(object):
> """
> raise NotImplementedError()
>
> + def install_pkg_post(self, filename, fetch_dir, install_dir,
> preserve_install_dir=False):
> + """ Fetcher specific post install
> + @param filename: The filename of the package to install
> + @param fetch_dir: The fetched path of the package
> + @param install_dir: The path to install the package to
> + @preserve_install_dir: Preserve the install directory
> + """
> + # check to see if the install_dir exists and if it does
> + # then check to see if the .checksum file is the latest
> + install_dir_exists = False
> + try:
> + self.pkgmgr._run_command("ls %s" % install_dir)
It occurred to me that this command could be more precise, such as
'test -d %s' % install_dir
> + install_dir_exists = True
> + except (error.CmdError, error.AutoservRunError):
> + pass
> +
> + fetch_path = os.path.join(fetch_dir, filename)
> + if (install_dir_exists and
> + not self.pkgmgr.untar_required(fetch_path, install_dir)):
> + return
> +
> + # untar the package into install_dir and
> + # update the checksum in that directory
> + if not preserve_install_dir:
> + # Make sure we clean up the install_dir
> + self.pkgmgr._run_command('rm -rf %s' % install_dir)
> + self.pkgmgr._run_command('mkdir -p %s' % install_dir)
> +
> + self.pkgmgr.untar_pkg(fetch_path, install_dir)
> +
>
> class HttpFetcher(RepositoryFetcher):
> wget_cmd_pattern = 'wget --connect-timeout=15 -nv %s -O %s'
> @@ -135,6 +180,7 @@ class HttpFetcher(RepositoryFetcher):
> """
> self.run_command = package_manager._run_command
> self.url = repository_url
> + self.pkgmgr = package_manager
>
>
> def _quick_http_test(self):
> @@ -191,6 +237,7 @@ class LocalFilesystemFetcher(RepositoryFetcher):
> def __init__(self, package_manager, local_dir):
> self.run_command = package_manager._run_command
> self.url = local_dir
> + self.pkgmgr = package_manager
>
>
> def fetch_pkg_file(self, filename, dest_path):
> @@ -361,34 +408,12 @@ class BasePackageManager(object):
>
> self._run_command('mkdir -p %s' % fetch_dir)
>
> - pkg_name = self.get_tarball_name(name, pkg_type)
> - fetch_path = os.path.join(fetch_dir, pkg_name)
> try:
> # Fetch the package into fetch_dir
> - self.fetch_pkg(pkg_name, fetch_path, use_checksum=True)
> -
> - # check to see if the install_dir exists and if it does
> - # then check to see if the .checksum file is the latest
> - install_dir_exists = False
> - try:
> - self._run_command("ls %s" % install_dir)
> - install_dir_exists = True
> - except (error.CmdError, error.AutoservRunError):
> - pass
> -
> - if (install_dir_exists and
> - not self.untar_required(fetch_path, install_dir)):
> - return
> -
> - # untar the package into install_dir and
> - # update the checksum in that directory
> - if not preserve_install_dir:
> - # Make sure we clean up the install_dir
> - self._run_command('rm -rf %s' % install_dir)
> - self._run_command('mkdir -p %s' % install_dir)
> -
> - self.untar_pkg(fetch_path, install_dir)
> + fetcher = self.fetch_pkg(name, fetch_dir, use_checksum=True,
> + repo_url=repo_url, install=True)
>
> + fetcher.install_pkg_post(name, fetch_dir, install_dir,
> preserve_install_dir)
> except error.PackageFetchError, why:
> raise error.PackageInstallError(
> 'Installation of %s(type:%s) failed : %s'
> @@ -399,7 +424,7 @@ class BasePackageManager(object):
> lockfile.close()
>
>
> - def fetch_pkg(self, pkg_name, dest_path, repo_url=None,
> use_checksum=False):
> + def fetch_pkg(self, pkg_name, dest_path, repo_url=None,
> use_checksum=False, install=False):
> '''
> Fetch the package into dest_dir from repo_url. By default repo_url
> is None and the package is looked in all the repositories specified.
> @@ -413,6 +438,9 @@ class BasePackageManager(object):
> checksum file itself. This is used internally by the
> packaging system. It should be ignored by externals
> callers of this method who use it fetch custom
> packages.
> + install : install path has unique name and destination
> requirements
> + that vary based on the fetcher that is used. So call
> them
> + here as opposed to install_pkg.
> '''
>
> try:
> @@ -444,17 +472,20 @@ class BasePackageManager(object):
> # reverse order, assuming that the 'newest' repos are most desirable
> for fetcher in reversed(repositories):
> try:
> + #different fetchers have different install requirements
> + (name, dest) = fetcher.install_pkg_setup(pkg_name,
> dest_path, install)
> +
> # Fetch the package if it is not there, the checksum does
> # not match, or checksums are disabled entirely
> need_to_fetch = (
> not use_checksum or not pkg_exists
> - or not self.compare_checksum(dest_path, fetcher.url))
> + or not self.compare_checksum(dest, fetcher.url))
> if need_to_fetch:
> - fetcher.fetch_pkg_file(pkg_name, dest_path)
> + fetcher.fetch_pkg_file(pkg_name, dest)
> # update checksum so we won't refetch next time.
> if use_checksum:
> - self.update_checksum(dest_path)
> - return
> + self.update_checksum(dest)
> + return fetcher
> except (error.PackageFetchError, error.AutoservRunError):
> # The package could not be found in this repo, continue
> looking
> logging.debug('%s could not be fetched from %s', pkg_name,
_______________________________________________
Autotest mailing list
Autotest@test.kernel.org
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
