Re: [pkg-discuss] Code review for 7156990
Bart Smaalders
> Webrev:
> https://cr.opensolaris.org/action/browse/pkg/bpytlik/7156990-v1
>
> Bug:
> 7156990 setting a mediator with signed packages containing variants
> doesn't work
>
> This is causing some real pain, so if we could get this reviewed
> quickly, that'd be great.
This appears correct. Is the same bug present in the revert code?
- Bart
--
Bart Smaalders Solaris Kernel Performance
bart.sm...@oracle.com http://blogs.oracle.com/barts
"You will contribute more with Mercurial than with Thunderbird."
"Civilization advances by extending the number of important
operations which we can perform without thinking about them."
_______________________________________________
pkg-discuss mailing list
pkg-d...@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Shawn Walker
> On 03/27/12 15:42, Brock Pytlik wrote:
>> Webrev:
>> https://cr.opensolaris.org/action/browse/pkg/bpytlik/7156990-v1
>>
>> Bug:
>> 7156990 setting a mediator with signed packages containing variants
>> doesn't work
>>
>> This is causing some real pain, so if we could get this reviewed
>> quickly, that'd be great.
>
> This appears correct. Is the same bug present in the revert code?
plan_revert() does appear to have the same issue; verify() and repair()
seem to be correct.
t_pkgsign.py also needs a copyright update.
Otherwise, looks right to me as well.
-Shawn
Brock Pytlik
> On 03/29/12 09:22, Bart Smaalders wrote:
>> On 03/27/12 15:42, Brock Pytlik wrote:
>>> Webrev:
>>> https://cr.opensolaris.org/action/browse/pkg/bpytlik/7156990-v1
>>>
>>> Bug:
>>> 7156990 setting a mediator with signed packages containing variants
>>> doesn't work
>>>
>>> This is causing some real pain, so if we could get this reviewed
>>> quickly, that'd be great.
>>
>> This appears correct. Is the same bug present in the revert code?
>
> plan_revert() does appear to have the same issue; verify() and
> repair() seem to be correct.
>
> t_pkgsign.py also needs a copyright update.
>
> Otherwise, looks right to me as well.
Thanks for the reviews,
Brock
Brock Pytlik
https://cr.opensolaris.org/action/browse/pkg/bpytlik/7156990-v2
It adds a test case for revert and fixes that issue as well.
Brock
Shawn Walker
> Updated webrev:
> https://cr.opensolaris.org/action/browse/pkg/bpytlik/7156990-v2
>
> It adds a test case for revert and fixes that issue as well.
Only two questions:
1) does the new revert test fail without your fix?
2) does revert fail (as expected) if you attempt to revert a file
that's supplied by a variant that isn't currently applicable to the image?
Brock Pytlik
> On 03/29/12 15:19, Brock Pytlik wrote:
>> Updated webrev:
>> https://cr.opensolaris.org/action/browse/pkg/bpytlik/7156990-v2
>>
>> It adds a test case for revert and fixes that issue as well.
>
> Only two questions:
>
> 1) does the new revert test fail without your fix?
>
> 2) does revert fail (as expected) if you attempt to revert a file
> that's supplied by a variant that isn't currently applicable to the
> image?
revert fails. But since you were concerned I've added a test for that as
well.
I'm going to go ahead and put this back.
Brock
Shawn Walker
> On 03/29/12 15:26, Shawn Walker wrote:
>> On 03/29/12 15:19, Brock Pytlik wrote:
>>> Updated webrev:
>>> https://cr.opensolaris.org/action/browse/pkg/bpytlik/7156990-v2
>>>
>>> It adds a test case for revert and fixes that issue as well.
>>
>> Only two questions:
>>
>> 1) does the new revert test fail without your fix?
> Yes.
>>
>> 2) does revert fail (as expected) if you attempt to revert a file
>> that's supplied by a variant that isn't currently applicable to the
>> image?
> Um... if you try to revert a file that's not on the system... then yes,
> revert fails. But since you were concerned I've added a test for that as
> well.
I believe you are able to revert files that have been removed but should
exist, so I thought it was relevant to ask.
It was just a question really...
Thanks,