Re: [PATCHv2 09/11][auh] upgradehelper.py: Change policy for send emails and fix error passing

["Aníbal Limón" <anibal.limon@linux.intel.com>]

Hi Paul,

On 16/06/15 08:01, Paul Eggleton wrote:
> Hi Aníbal,
>
> On Friday 12 June 2015 20:10:45 Aníbal Limón wrote:
>> Now send emails almost in all cases this give the maintainer
>> patches and diff to continue work also if the build isn't
>> succesful.
>>
>> [YOCTO #7489]
>>
>> Signed-off-by: Aníbal Limón <anibal.limon@linux.intel.com>
>> ---
>>   upgradehelper.py | 19 ++++++++++---------
>>   1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/upgradehelper.py b/upgradehelper.py
>> index b1f075d..d065fba 100755
>> --- a/upgradehelper.py
>> +++ b/upgradehelper.py
>> @@ -346,8 +346,6 @@ class Updater(object):
>>                   self.git.clean_untracked()
>>                   return
>>
>> -        status = type(err).__name__
>> -
>>           # drop last upgrade from git. It's safer this way if the upgrade
>> has # problems and other recipes depend on it. Give the other recipes a #
>> chance...
>> @@ -381,8 +379,14 @@ class Updater(object):
>>                   "Attached are the patch, license diff (if change) and
>> bitbake log.\n\n" \ "Regards,\nThe Upgrade Helper"
>>
>> -            # don't bother maintainer with mail if the recipe is already up
>> to date -            if status == "UpgradeNotNeededError":
>> +            # if error only send email when useful infomration for
>> maintainers exist +            if err and not (isinstance(err, PatchError)
>> or \
>> +               isinstance(err, ConfigureError) or \
>> +               isinstance(err, CompilationError) or \
>> +               isinstance(err, LicenseError)):
>> +                D( "%s: Don't send email to maintainer because the error
>> was " \ +                   "%s and the information isn't useful, please
>> review it." \ +                    % (self.pn, type(err).__name__))
>>                   return
> I think a better approach here would be to have the classes of error that are
> likely to be fixable by the maintainer as inheriting from a particular class
> (e.g. MaintainerError) and then we can just check for that rather than having
> to extend this code every time we add a new type of error.
I agree, i'll add new class MaintainerError and make 
Configure/Compilation/Patch/License to
extend it.

>
>>               if self.maintainer in maintainer_override:
>> @@ -478,6 +482,7 @@ class Updater(object):
>>
>>           attempted_pkgs = 0
>>           for self.pn, self.new_ver, self.maintainer in pkgs_to_upgrade:
>> +            error = None
>>               self.recipe = None
>>               attempted_pkgs += 1
>>               I(" ATTEMPT PACKAGE %d/%d" % (attempted_pkgs, total_pkgs))
>> @@ -489,10 +494,6 @@ class Updater(object):
>>                       step()
>>
>>                   I(" %s: Upgrade SUCCESSFUL! Please test!" % self.pn)
>> -                error = None
>> -            except UpgradeNotNeededError as e:
>> -                I(" %s: %s" % (self.pn, e.message))
>> -                error = e
> I'm confused by this. Won't this change result in UpgradeNotNeededError being
> treated as an actual error? Surely we don't actually want that?
Now UpgradeNotNeededError is handled by new upstream mechanism when load 
recipes to upgrade
printing message about it.

>
> Cheers,
> Paul
>

Regards,
     alimon