Make geoip optional and local sources a default #430

Merged
merged 8 commits into from Apr 8, 2016
View
@@ -115,6 +115,7 @@
import os
import shutil
+from snapcraft._options import ProjectOptions # noqa
from snapcraft._help import topic_help # noqa
from snapcraft._store import login, logout, upload # noqa
from snapcraft import common
View
@@ -1,4 +1,3 @@
-#!/usr/bin/python3
# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
#
# Copyright (C) 2016 Canonical Ltd
View
@@ -0,0 +1,29 @@
+# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
+#
+# Copyright (C) 2016 Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+
+class ProjectOptions:
+
+ @property
+ def use_geoip(self):
+ return self.__use_geoip
+
+ @use_geoip.setter
+ def use_geoip(self, setting):
+ self.__use_geoip = setting
@kyrofa

kyrofa Apr 6, 2016

Member

What is the benefit of this encapsulation?

@sergiusens

sergiusens Apr 6, 2016

Collaborator

El miércoles, 6 de abril de 2016 14h'52:17 ART, Kyle Fazzari
notifications@github.com escribió:

+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see http://www.gnu.org/licenses/.
+
+
+class _Project:
+

  • @property
  • def use_geoip(self):
  •    return self.__use_geoip
    
  • @use_geoip.setter
  • def use_geoip(self, setting):
  •    self.__use_geoip = setting
    

What is the benefit of this encapsulation?

By doing it this way we can change the implementation internally without
worrying about the external interface. Also no one would be able to reach
this.

Enviado con Dekko desde mi dispositivo Ubuntu

@kyrofa

kyrofa Apr 8, 2016

Member

Yeah that's good API design-- is it considered best-practice to do this for all member variables (e.g. like it is in C++)?

@sergiusens

sergiusens Apr 8, 2016

Collaborator

I do feel we will do this for the arch ones specifically.

Another thing I was contemplating was making the values set immutable and only allow them to be set on class instantiation. Thoughts?

@kyrofa

kyrofa Apr 8, 2016

Member

Another thing I was contemplating was making the values set immutable and only allow them to be set on class instantiation. Thoughts?

I'm not sure I understand what you mean by "values set" here. You mean the values of this class? e.g. so plugins can't change these settings for the entire project?

@sergiusens

sergiusens Apr 8, 2016

Collaborator

Exactly that. But we can dive into that when we tackle the common rework

@kyrofa

kyrofa Apr 8, 2016

Member

Yeah 👍 on both the immutable and tackling when reworking common.

+
+ def __init__(self):
+ self.__use_geoip = False
View
@@ -1,4 +1,3 @@
-#!/usr/bin/python3
# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
#
# Copyright (C) 2016 Canonical Ltd
View
@@ -57,7 +57,7 @@ def init():
logger.info('Created snapcraft.yaml.')
-def execute(step, part_names=None):
+def execute(step, project_options, part_names=None):
"""Execute until step in the lifecycle for part_names or all parts.
Lifecycle execution will happen for each step iterating over all
@@ -69,13 +69,14 @@ def execute(step, part_names=None):
and after is not in this set, an exception will be raised.
:param str step: A valid step in the lifecycle: pull, build, strip or snap.
+ :param project_options: Runtime options for the project.
:param list part_names: A list of parts to execute the lifecycle on.
:raises RuntimeError: If a prerequesite of the part needs to be staged
and such part is not in the list of parts to iterate
over.
- :returns: A dict with the snap name, version and architectures.
+ :returns: A dict with the snap name, version, type and architectures.
"""
- config = snapcraft.yaml.load_config()
+ config = snapcraft.yaml.load_config(project_options)
repo.install_build_packages(config.build_tools)
_Executor(config).run(step, part_names)
@@ -192,14 +193,14 @@ def _snap_data_from_dir(directory):
'type': snap.get('type', '')}
-def snap(directory=None, output=None):
+def snap(project_options, directory=None, output=None):
if directory:
snap_dir = os.path.abspath(directory)
snap = _snap_data_from_dir(snap_dir)
else:
# make sure the full lifecycle is executed
snap_dir = common.get_snapdir()
- snap = execute('strip')
+ snap = execute('strip', project_options)
snap_name = output or format_snap_name(snap)
View
@@ -19,9 +19,9 @@
snapcraft
Usage:
- snapcraft [options] [--no-parallel-build]
+ snapcraft [options] [--enable-geoip --no-parallel-build]
snapcraft [options] init
- snapcraft [options] pull [<part> ...]
+ snapcraft [options] pull [<part> ...] [--enable-geoip]
snapcraft [options] build [<part> ...] [--no-parallel-build]
snapcraft [options] stage [<part> ...]
snapcraft [options] strip [<part> ...]
@@ -45,6 +45,10 @@
architecture. Very few plugins support
this.
+Options specific to pulling:
+ --enable-geoip enables geoip for the pull step if stage-packages
+ are used.
+
Options specific to building:
--no-parallel-build use only a single build job per part
(the default number of jobs per part is
@@ -132,11 +136,15 @@ def main(argv=None):
common.set_enable_parallel_builds(not args['--no-parallel-build'])
+ project_options = snapcraft.ProjectOptions()
+ if args['--enable-geoip']:
+ project_options.use_geoip = True
+
if args['--target-arch']:
common.set_target_machine(args['--target-arch'])
try:
- run(args)
+ run(args, project_options)
except Exception as e:
if args['--debug']:
raise
@@ -166,11 +174,12 @@ def _get_command_from_arg(args):
return functions[function[0]]
-def run(args):
+def run(args, project_options):
lifecycle_command = _get_lifecycle_command(args)
argless_command = _get_command_from_arg(args)
if lifecycle_command:
- snapcraft.lifecycle.execute(lifecycle_command, args['<part>'])
+ snapcraft.lifecycle.execute(
+ lifecycle_command, project_options, args['<part>'])
elif argless_command:
argless_command()
elif args['clean']:
@@ -181,8 +190,9 @@ def run(args):
snapcraft.topic_help(args['<topic>'] or args['<plugin>'],
args['--devel'], args['topics'])
else: # snap by default:
- snapcraft.lifecycle.snap(args['<directory>'], args['--output'])
+ snapcraft.lifecycle.snap(
+ project_options, args['<directory>'], args['--output'])
if __name__ == '__main__': # pragma: no cover
- main() # pragma: no cover
+ main() # pragma: no cover
@@ -116,16 +116,19 @@ def installdir(self):
def ubuntu(self):
if not self._ubuntu:
self._ubuntu = repo.Ubuntu(
- self.ubuntudir, sources=self.code.PLUGIN_STAGE_SOURCES)
+ self.ubuntudir, sources=self.code.PLUGIN_STAGE_SOURCES,
+ use_geoip=self._project_options.use_geoip)
return self._ubuntu
- def __init__(self, plugin_name, part_name, properties, part_schema):
+ def __init__(self, plugin_name, part_name, properties,
+ project_options, part_schema):
self.valid = False
self.code = None
self.config = {}
self._name = part_name
self._ubuntu = None
+ self._project_options = project_options
self.deps = []
self.stagedir = os.path.join(os.getcwd(), 'stage')
@@ -572,12 +575,14 @@ def _load_local(module_name):
return module
-def load_plugin(part_name, plugin_name, properties=None, part_schema=None):
+def load_plugin(part_name, plugin_name, properties=None,
+ project_options=None, part_schema=None):
if properties is None:
properties = {}
if part_schema is None:
part_schema = {}
- return PluginHandler(plugin_name, part_name, properties, part_schema)
+ return PluginHandler(plugin_name, part_name, properties,
+ project_options, part_schema)
def _migratable_filesets(fileset, srcdir):
View
@@ -115,19 +115,14 @@ def __init__(self, package_name):
class Ubuntu:
- def __init__(self, rootdir, recommends=False, sources=_DEFAULT_SOURCES):
+ def __init__(self, rootdir, recommends=False,
+ sources=None, use_geoip=True):
self.downloaddir = os.path.join(rootdir, 'download')
self.rootdir = rootdir
self.recommends = recommends
- sources = sources or _DEFAULT_SOURCES
- local = False
- if 'SNAPCRAFT_LOCAL_SOURCES' in os.environ:
- print('using local sources')
- sources = _get_local_sources_list()
- local = True
self.apt_cache, self.apt_progress = _setup_apt_cache(
- rootdir, sources, local)
+ rootdir, sources, use_geoip)
def get(self, package_names):
os.makedirs(self.downloaddir, exist_ok=True)
@@ -224,10 +219,16 @@ def _get_geoip_country_code_prefix():
return ''
-def _format_sources_list(sources, arch, release='vivid'):
+def _format_sources_list(sources, use_geoip, arch, release='xenial'):
+ if not sources:
+ sources = _DEFAULT_SOURCES
+
if arch in ('amd64', 'i386'):
- geoip_prefix = _get_geoip_country_code_prefix()
- prefix = geoip_prefix + '.archive' if geoip_prefix else 'archive'
+ if use_geoip:
+ geoip_prefix = _get_geoip_country_code_prefix()
+ prefix = '{}.archive'.format(geoip_prefix)
+ else:
+ prefix = 'archive'
suffix = 'ubuntu'
security = 'security'
else:
@@ -243,13 +244,16 @@ def _format_sources_list(sources, arch, release='vivid'):
})
-def _setup_apt_cache(rootdir, sources, local=False):
+def _setup_apt_cache(rootdir, sources, use_geoip):
os.makedirs(os.path.join(rootdir, 'etc', 'apt'), exist_ok=True)
srcfile = os.path.join(rootdir, 'etc', 'apt', 'sources.list')
- if not local:
- series = platform.linux_distribution()[2]
- sources = _format_sources_list(sources, common.get_arch(), series)
+ if use_geoip or sources:
+ release = platform.linux_distribution()[2]
+ arch = common.get_arch()
+ sources = _format_sources_list(sources, use_geoip, arch, release)
+ else:
+ sources = _get_local_sources_list()
with open(srcfile, 'w') as f:
f.write(sources)
@@ -14,11 +14,10 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
-import logging
import os
import os.path
-import fixtures
+from unittest import mock
from snapcraft.main import main
from snapcraft import (
@@ -27,6 +26,13 @@
)
+class FakeUbuntuRepo:
+
+ def __init__(self, rootdir, recommends=False,
+ sources=None, use_geoip=True):
+ self.use_geoip = use_geoip
+
+
class PullCommandTestCase(tests.TestCase):
yaml_template = """name: pull-test
@@ -38,11 +44,13 @@ class PullCommandTestCase(tests.TestCase):
{parts}"""
yaml_part = """ pull{:d}:
- plugin: nil
- source: ."""
+ plugin: nil"""
- def make_snapcraft_yaml(self, n=1):
- parts = '\n'.join([self.yaml_part.format(i) for i in range(n)])
+ def make_snapcraft_yaml(self, n=1, yaml_part=None):
+ if not yaml_part:
+ yaml_part = self.yaml_part
+
+ parts = '\n'.join([yaml_part.format(i) for i in range(n)])
super().make_snapcraft_yaml(self.yaml_template.format(parts=parts))
parts = []
@@ -57,8 +65,6 @@ def make_snapcraft_yaml(self, n=1):
return parts
def test_pull_invalid_part(self):
- fake_logger = fixtures.FakeLogger(level=logging.ERROR)
- self.useFixture(fake_logger)
self.make_snapcraft_yaml()
with self.assertRaises(SystemExit) as raised:
@@ -69,8 +75,6 @@ def test_pull_invalid_part(self):
str(raised.exception))
def test_pull_defaults(self):
- fake_logger = fixtures.FakeLogger(level=logging.ERROR)
- self.useFixture(fake_logger)
parts = self.make_snapcraft_yaml()
main(['pull'])
@@ -83,8 +87,6 @@ def test_pull_defaults(self):
self.verify_state('pull0', parts[0]['state_dir'], 'pull')
def test_pull_one_part_only_from_3(self):
- fake_logger = fixtures.FakeLogger(level=logging.ERROR)
- self.useFixture(fake_logger)
parts = self.make_snapcraft_yaml(n=3)
main(['pull', 'pull1'])
@@ -101,3 +103,49 @@ def test_pull_one_part_only_from_3(self):
'Pulled wrong part')
self.assertFalse(os.path.exists(parts[i]['state_dir']),
'Expected for only to be a state file for pull1')
+
+ @mock.patch('snapcraft.repo._setup_apt_cache')
+ @mock.patch('snapcraft.repo.Ubuntu.get')
+ @mock.patch('snapcraft.repo.Ubuntu.unpack')
+ def test_pull_stage_packages_without_geoip(self, mock_get, mock_ubpack,
+ mock_setup_apt_cache):
+ yaml_part = """ pull{:d}:
+ plugin: nil
+ stage-packages: ['mir']"""
+
+ self.make_snapcraft_yaml(n=3, yaml_part=yaml_part)
+
+ mock_apt_cache = mock.Mock()
+ mock_apt_progress = mock.Mock()
+ mock_setup_apt_cache.return_value = (mock_apt_cache, mock_apt_progress)
+
+ main(['pull', 'pull1'])
+
+ mock_setup_apt_cache.assert_called_once_with(
+ os.path.join(common.get_partsdir(), 'pull1', 'ubuntu'),
+ [], # no sources
+ False, # use_geoip
+ )
+
+ @mock.patch('snapcraft.repo._setup_apt_cache')
+ @mock.patch('snapcraft.repo.Ubuntu.get')
+ @mock.patch('snapcraft.repo.Ubuntu.unpack')
+ def test_pull_stage_packages_with_geoip(self, mock_get, mock_ubpack,
+ mock_setup_apt_cache):
+ yaml_part = """ pull{:d}:
+ plugin: nil
+ stage-packages: ['mir']"""
+
+ self.make_snapcraft_yaml(n=3, yaml_part=yaml_part)
+
+ mock_apt_cache = mock.Mock()
+ mock_apt_progress = mock.Mock()
+ mock_setup_apt_cache.return_value = (mock_apt_cache, mock_apt_progress)
+
+ main(['pull', 'pull1', '--enable-geoip'])
+
+ mock_setup_apt_cache.assert_called_once_with(
+ os.path.join(common.get_partsdir(), 'pull1', 'ubuntu'),
+ [], # no sources
+ True, # use_geoip
+ )
Oops, something went wrong.