From e1cf59a709b29d0149993eee439c2ac88b5f3afc Mon Sep 17 00:00:00 2001 From: Sergio Schvezov Date: Tue, 30 Aug 2016 16:43:18 -0300 Subject: [PATCH] Better file conflict message (#766) When parts conflict the message is plain and blunt, we should do some hand holding. LP: #1617390 Signed-off-by: Sergio Schvezov --- integration_tests/test_stage.py | 22 +++++++--- snapcraft/__init__.py | 2 +- snapcraft/internal/errors.py | 62 +++++++++++++++++++++++++++ snapcraft/internal/pluginhandler.py | 22 ++++------ snapcraft/storeapi/errors.py | 18 +------- snapcraft/tests/test_pluginhandler.py | 17 ++++---- 6 files changed, 99 insertions(+), 44 deletions(-) create mode 100644 snapcraft/internal/errors.py diff --git a/integration_tests/test_stage.py b/integration_tests/test_stage.py index 986ab4ca08..f39af72f18 100644 --- a/integration_tests/test_stage.py +++ b/integration_tests/test_stage.py @@ -1,6 +1,6 @@ # -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- # -# Copyright (C) 2015 Canonical Ltd +# Copyright (C) 2015-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 @@ -16,7 +16,7 @@ import subprocess -from testtools.matchers import EndsWith +from testtools.matchers import Contains import integration_tests @@ -30,7 +30,19 @@ def test_conflicts(self): self.run_snapcraft, 'stage', project_dir) self.assertEqual(1, exception.returncode) - expected = ( + expected_conflicts = ( "Parts 'p1' and 'p2' have the following file paths in common " - "which have different contents:\nbin/test\n") - self.assertThat(exception.output, EndsWith(expected)) + "which have different contents:\n bin/test\n") + self.assertThat(exception.output, Contains(expected_conflicts)) + + expected_help = ( + 'Snapcraft offers some capabilities to solve this by use ' + 'of the following keywords:\n' + ' - `filesets`\n' + ' - `stage`\n' + ' - `snap`\n' + ' - `organize`\n\n' + 'Learn more about these part keywords by running ' + '`snapcraft help plugins`' + ) + self.assertThat(exception.output, Contains(expected_help)) diff --git a/snapcraft/__init__.py b/snapcraft/__init__.py index fb66ecf4b9..384b3bd90d 100644 --- a/snapcraft/__init__.py +++ b/snapcraft/__init__.py @@ -213,7 +213,7 @@ - -usr/lib/libtest.so # Excludng libtest.so - $manpages # Including the 'manpages' fileset - - prime: YAML file and fileset list + - snap: YAML file and fileset list A list of files from a part install directory to copy into `prime/`. This section takes exactly the same form as the 'stage' section but the diff --git a/snapcraft/internal/errors.py b/snapcraft/internal/errors.py new file mode 100644 index 0000000000..1c594c2949 --- /dev/null +++ b/snapcraft/internal/errors.py @@ -0,0 +1,62 @@ +# -*- 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 . + + +class SnapcraftError(Exception): + """Base class for all snapcraft exceptions. + + :cvar fmt: A format string that daughter classes override + + """ + fmt = 'Daughter classes should redefine this' + + def __init__(self, **kwargs): + for key, value in kwargs.items(): + setattr(self, key, value) + + def __str__(self): + return self.fmt.format([], **self.__dict__) + + +class PluginError(Exception): + pass + + +class MissingState(Exception): + pass + + +class SnapcraftPartConflictError(SnapcraftError): + + fmt = ( + 'Parts {other_part_name!r} and {part_name!r} have the following file ' + 'paths in common which have different contents:\n' + '{file_paths}\n\n' + 'Snapcraft offers some capabilities to solve this by use of the ' + 'following keywords:\n' + ' - `filesets`\n' + ' - `stage`\n' + ' - `snap`\n' + ' - `organize`\n\n' + 'Learn more about these part keywords by running ' + '`snapcraft help plugins`' + ) + + def __init__(self, *, part_name, other_part_name, conflict_files): + spaced_conflict_files = (' {}'.format(i) for i in conflict_files) + super().__init__(part_name=part_name, + other_part_name=other_part_name, + file_paths='\n'.join(sorted(spaced_conflict_files))) diff --git a/snapcraft/internal/pluginhandler.py b/snapcraft/internal/pluginhandler.py index 0dcf717a0c..c3a364d5d3 100644 --- a/snapcraft/internal/pluginhandler.py +++ b/snapcraft/internal/pluginhandler.py @@ -30,6 +30,11 @@ import snapcraft from snapcraft import file_utils +from snapcraft.internal.errors import ( + PluginError, + MissingState, + SnapcraftPartConflictError, +) from snapcraft.internal import ( common, libraries, @@ -40,14 +45,6 @@ logger = logging.getLogger(__name__) -class PluginError(Exception): - pass - - -class MissingState(Exception): - pass - - class PluginHandler: @property @@ -845,11 +842,10 @@ def check_for_collisions(parts): conflict_files.append(f) if conflict_files: - raise EnvironmentError( - 'Parts {!r} and {!r} have the following file paths in ' - 'common which have different contents:\n{}'.format( - other_part_name, part.name, - '\n'.join(sorted(conflict_files)))) + raise SnapcraftPartConflictError( + other_part_name=other_part_name, + part_name=part.name, + conflict_files=conflict_files) # And add our files to the list parts_files[part.name] = {'files': part_files, diff --git a/snapcraft/storeapi/errors.py b/snapcraft/storeapi/errors.py index 29a02f5fd7..4516ebff32 100644 --- a/snapcraft/storeapi/errors.py +++ b/snapcraft/storeapi/errors.py @@ -14,25 +14,9 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . - from simplejson.scanner import JSONDecodeError - -# TODO move to snapcraft.errors --elopio - 2016-06-20 -class SnapcraftError(Exception): - """Base class for all storeapi exceptions. - - :cvar fmt: A format string that daughter classes override - - """ - fmt = 'Daughter classes should redefine this' - - def __init__(self, **kwargs): - for key, value in kwargs.items(): - setattr(self, key, value) - - def __str__(self): - return self.fmt.format([], **self.__dict__) +from snapcraft.internal.errors import SnapcraftError class InvalidCredentialsError(SnapcraftError): diff --git a/snapcraft/tests/test_pluginhandler.py b/snapcraft/tests/test_pluginhandler.py index 5e63b004be..545fcb03e0 100644 --- a/snapcraft/tests/test_pluginhandler.py +++ b/snapcraft/tests/test_pluginhandler.py @@ -31,6 +31,7 @@ import fixtures import snapcraft +from snapcraft.internal.errors import SnapcraftPartConflictError from snapcraft.internal import ( common, lifecycle, @@ -1949,24 +1950,24 @@ def test_no_collisions(self): pluginhandler.check_for_collisions([self.part1, self.part2]) def test_collisions_between_two_parts(self): - with self.assertRaises(EnvironmentError) as raised: + with self.assertRaises(SnapcraftPartConflictError) as raised: pluginhandler.check_for_collisions( [self.part1, self.part2, self.part3]) - self.assertEqual( - raised.exception.__str__(), + self.assertIn( "Parts 'part2' and 'part3' have the following file paths in " - "common which have different contents:\n1\na/2") + "common which have different contents:\n 1\n a/2", + raised.exception.__str__()) def test_collisions_between_two_parts_pc_files(self): - with self.assertRaises(EnvironmentError) as raised: + with self.assertRaises(SnapcraftPartConflictError) as raised: pluginhandler.check_for_collisions( [self.part1, self.part4]) - self.assertEqual( - raised.exception.__str__(), + self.assertIn( "Parts 'part1' and 'part4' have the following file paths in " - "common which have different contents:\nfile.pc") + "common which have different contents:\n file.pc", + raised.exception.__str__()) class StagePackagesTestCase(tests.TestCase):