[pkg-discuss] [REVIEW] code review request : 7141684

[Saurabh Vyas]

Hi All,

Please review these trivial fixes and let me know your comments.

CR 7141684 'pkg publisher -P' traceback when NO publisher is configured

webrev :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev1/

Test run after applying this fix :
root@onnv175b-nd:~# pkg publisher -P
PUBLISHER TYPE STATUS URI
root@onnv175b-nd:~#

root@onnv175b-nd:~# pkg publisher
PUBLISHER TYPE STATUS URI
root@onnv175b-nd:~#

root@onnv175b-nd:~# pkg publisher -H
root@onnv175b-nd:~#

Thanks,
~Saurabh

--

Saurabh Vyas
Solaris Install Group,
Revenue Product Engineering (RPE), Systems

ORACLE India | Off Langford Road | Bangalore | 560025

|Bangalore |
Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment

_______________________________________________
pkg-discuss mailing list
pkg-d...@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

[Saurabh Vyas]

A gentle reminder, please if someone can review this fix.

Thanks,
~Saurabh

[Danek Duvall]

Saurabh Vyas wrote:

> webrev : https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev1/

I don't see how this can work, and indeed when I apply your changes to the
current gate, I get the same stack trace described in the bug.

The problem is that pubs still contains a single element, None. So once
you get into the loop starting at 4599, p is set to None, and the very
first time you try to access its members, on line 4563, you get the stack
trace complaining about "prefix".

I think that you need to just make sure that pubs is empty when there are
no publishers.

Danek

[Saurabh Vyas]

On 02/07/12 23:20, Danek Duvall wrote:
> Saurabh Vyas wrote:
>
>> webrev : https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev1/
>
> I don't see how this can work, and indeed when I apply your changes to the
> current gate, I get the same stack trace described in the bug.

Thanks Danek for looking into this fix.

Not sure why you got the traceback after applying the fix, as get
following o/p :

root@S11:~# diff /usr/bin/pkg /usr/bin/pkg.orig
4528a4529
> pref_pub = api_inst.get_highest_ranked_publisher()
4530d4530
< pref_pub = api_inst.get_highest_ranked_publisher()
4599,4600d4598
< if r is None:
< continue

- With my fix
root@S11:~# pkg publisher -P
PUBLISHER TYPE STATUS URI

- Without my fix
root@S11:~# /usr/bin/pkg.orig publisher -P
PUBLISHER TYPE STATUS URI
Traceback (most recent call last):
File "/usr/bin/pkg.orig", line 5982, in handle_errors
__ret = func(*args, **kwargs)
File "/usr/bin/pkg.orig", line 5960, in main_func
return func(api_inst, pargs)
File "/usr/bin/pkg.orig", line 4602, in publisher_list
for uri in sorted(r.origins):
AttributeError: 'NoneType' object has no attribute 'origins'

pkg: This is an internal error in pkg(5) version 3c00fe4465d3. Please log a
Service Request about this issue including the information above and this
message.
I am not

this test was done on a S11u1 build 8 system.

>
> The problem is that pubs still contains a single element, None. So once
> you get into the loop starting at 4599, p is set to None, and the very
> first time you try to access its members, on line 4563, you get the stack
> trace complaining about "prefix".

But I do not see this here. 'prefix' is set to 'publisher-name' that was
present earlier (and from which we have packages installed on the
system i.e. 'solaris in this case)

>
> I think that you need to just make sure that pubs is empty when there are
> no publishers.

So should I move the check for 'pubs is empty' right at the beginning
of the for loop (at 4599 ) ?

@@ -4557,6 +4557,12 @@
msg(fmt % tuple(hdrs))

for p in pubs:
+ if p.repository is None:
+ # We can skip rest of processing if
+ # this publsher do not have any
+ # repository associated
+ continue
+
# Store all our publisher related data in
# field_data ready for output

Please let me know your thoughts.

Thanks again,
~Saurabh

>
> Danek

--

Saurabh Vyas
Solaris Install Group,
Revenue Product Engineering (RPE), Systems

ORACLE India | Off Langford Road | Bangalore | 560025

|Bangalore |
Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment

[Saurabh Vyas - Oracle]

Hi Danek (and others),

Please if you can provide your thoughts on this issue.

Thanks,
~Saurabh

--

Revenue Product Engineering (RPE), Systems |Bangalore |

[Saurabh Vyas]

Hi Everyone,

Please if you can provide you comments for this fix (more details in
mail below).

I have also added fix for CR 7144038 (one liner fix) in the webrev.

webrev :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev2/webrev/

Please let me know your thoughts.

Thanks,
~Saurabh

[Danek Duvall]

Saurabh Vyas wrote:

> Not sure why you got the traceback after applying the fix, as get following
> o/p :
>
> root@S11:~# diff /usr/bin/pkg /usr/bin/pkg.orig
> 4528a4529
> > pref_pub = api_inst.get_highest_ranked_publisher()
> 4530d4530
> < pref_pub = api_inst.get_highest_ranked_publisher()
> 4599,4600d4598
> < if r is None:
> < continue

That's not the diff in the webrev, even accounting for this one being
backwards. But even so, it still doesn't work for me.

Now, I just re-read the bug report, and I'm not seeing the same stack trace
that you are. I'm getting

$ pkg -R /tmp/i publisher
PUBLISHER TYPE STATUS URI
$ pkg -R /tmp/i publisher -P

PUBLISHER TYPE STATUS URI
Traceback (most recent call last):

File "/usr/bin/pkg", line 5982, in handle_errors
__ret = func(*args, **kwargs)
File "/usr/bin/pkg", line 5960, in main_func
return func(api_inst, pargs)
File "/usr/bin/pkg", line 4563, in publisher_list
set_value(field_data["publisher"], p.prefix)
AttributeError: 'NoneType' object has no attribute 'prefix'

which clearly indicates that api_inst.get_highest_ranked_publisher() is
returning None for me, while you seem to be getting further with it. Sorry
I didn't pick up on that earlier.

This seems to be a difference between looking at an image without any
packages installed and one with, as if I install a package into my test
image first, I get the r.origins stack trace.

I think maybe get_highest_ranked_publisher() should always return None if
there are no configured publishers, regardless of whether there are any
installed packages, as there's no sense of publisher ranking if there is no
publisher configuration.

Danek

[Saurabh Vyas]

On 02/20/12 23:38, Danek Duvall wrote:
....

>
> That's not the diff in the webrev, even accounting for this one being
> backwards. But even so, it still doesn't work for me.
>
> Now, I just re-read the bug report, and I'm not seeing the same stack trace
> that you are. I'm getting
>
> $ pkg -R /tmp/i publisher
> PUBLISHER TYPE STATUS URI
> $ pkg -R /tmp/i publisher -P
> PUBLISHER TYPE STATUS URI
> Traceback (most recent call last):
> File "/usr/bin/pkg", line 5982, in handle_errors
> __ret = func(*args, **kwargs)
> File "/usr/bin/pkg", line 5960, in main_func
> return func(api_inst, pargs)
> File "/usr/bin/pkg", line 4563, in publisher_list
> set_value(field_data["publisher"], p.prefix)
> AttributeError: 'NoneType' object has no attribute 'prefix'
>
> which clearly indicates that api_inst.get_highest_ranked_publisher() is
> returning None for me, while you seem to be getting further with it. Sorry
> I didn't pick up on that earlier.

Alright, I was missing this test. I have modified the fix to address
this issue
too.

>
> This seems to be a difference between looking at an image without any
> packages installed and one with, as if I install a package into my test
> image first, I get the r.origins stack trace.
>
> I think maybe get_highest_ranked_publisher() should always return None if
> there are no configured publishers, regardless of whether there are any
> installed packages, as there's no sense of publisher ranking if there is no
> publisher configuration.

Tested my fix on following scenarios :

- No publisher configured for the current image
root@onnv175b-nd:~# pkg publisher -P
PUBLISHER TYPE STATUS URI

- Image with No package
root@onnv175b-nd:~# pkg -R /var/tmp/img publisher -P
PUBLISHER TYPE STATUS URI

works as expected

New webrev :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev3/webrev/

NOTE : I remove one test inside : test_publisher_apis (api/t_api.py)
as now get_highest_ranked_publisher() returns 'None' when there are no
configured publishers.

Please let me know your thought on this fix.

Thanks,
~Saurabh

>
> Danek

--

Saurabh Vyas
Solaris Install Group,
Revenue Product Engineering (RPE), Systems

ORACLE India | Off Langford Road | Bangalore | 560025

|Bangalore |
Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment

[Shawn Walker]

On 03/08/12 02:19, Saurabh Vyas wrote:
> On 02/20/12 23:38, Danek Duvall wrote:
...

>> This seems to be a difference between looking at an image without any
>> packages installed and one with, as if I install a package into my test
>> image first, I get the r.origins stack trace.
>>
>> I think maybe get_highest_ranked_publisher() should always return None if
>> there are no configured publishers, regardless of whether there are any
>> installed packages, as there's no sense of publisher ranking if there is no
>> publisher configuration.
>
> Tested my fix on following scenarios :
>
> - No publisher configured for the current image
> root@onnv175b-nd:~# pkg publisher -P
> PUBLISHER TYPE STATUS URI
>
> - Image with No package
> root@onnv175b-nd:~# pkg -R /var/tmp/img publisher -P
> PUBLISHER TYPE STATUS URI
>
> works as expected
>
> New webrev :
> https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev3/webrev/
>
> NOTE : I remove one test inside : test_publisher_apis (api/t_api.py)
> as now get_highest_ranked_publisher() returns 'None' when there are no
> configured publishers.

Please don't make this change. I specifically had
get_highest_ranked_publisher() return the first publisher because of an
issue with packagemanager. That's also why I had the unit test for it.

I believe this will break search functionality in the packagemanager.

-Shawn

[Saurabh Vyas - Oracle]

On 03/ 8/12 10:18 PM, Shawn Walker wrote:
> On 03/08/12 02:19, Saurabh Vyas wrote:
>> On 02/20/12 23:38, Danek Duvall wrote:
> ...
>>> This seems to be a difference between looking at an image without any
>>> packages installed and one with, as if I install a package into my test
>>> image first, I get the r.origins stack trace.
>>>
>>> I think maybe get_highest_ranked_publisher() should always return
>>> None if
>>> there are no configured publishers, regardless of whether there are any
>>> installed packages, as there's no sense of publisher ranking if there
>>> is no
>>> publisher configuration.

....

>>
>>
>> NOTE : I remove one test inside : test_publisher_apis (api/t_api.py)
>> as now get_highest_ranked_publisher() returns 'None' when there are no
>> configured publishers.
>
> Please don't make this change. I specifically had
> get_highest_ranked_publisher() return the first publisher because of an
> issue with packagemanager. That's also why I had the unit test for it.
>
> I believe this will break search functionality in the packagemanager.

Thanks for this input Shawn.
In this case, I will move back to previous approach i.e :

@@ -4557,6 +4557,11 @@
msg(fmt % tuple(hdrs))

for p in pubs:
+ # We can skip further processing if
+ # there are no configured publishers

+ if p.repository is None:

+ continue
+
# Store all our publisher related data in

Tested both scenarios again :

- No publisher configured for the current image
root@onnv175b-nd:~# pkg publisher -P
PUBLISHER TYPE STATUS URI

- Image with No package
root@onnv175b-nd:~# pkg -R /var/tmp/img publisher -P
PUBLISHER TYPE STATUS URI

works as expected

Created webrev again :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev4/webrev/

Please let me know if this seems fine.

Thanks,
~Saurabh

>
> -Shawn

--

Saurabh Vyas
Solaris Install Group,

Revenue Product Engineering (RPE), Systems |Bangalore |

Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment

[Saurabh Vyas - Oracle]

Hi All,

A gentle reminder, please if someone can review this fix.

More details in the mail below.........

Thanks,
~Saurabh

[Saurabh Vyas]

Hi All,

Another reminder, please if someone can provide inputs on this fix.
More details in mail below........

for quick ref, webrev location :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev4/webrev/

Thanks,
~Saurabh

--

ORACLE India | Off Langford Road | Bangalore | 560025

|Bangalore |

[Shawn Walker]

On 03/20/12 04:32, Saurabh Vyas wrote:
> Hi All,
>
> Another reminder, please if someone can provide inputs on this fix.
> More details in mail below........
>
> for quick ref, webrev location :
> https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev4/webrev/

So this isn't the right fix for a few reasons:

* it will show the publisher returned by
get_highest_ranked_publisher() even if that publisher is not
explicitly configured

* it potentially will not show publishers that do not have configured
origins or mirrors

* this change needs a test case

So here are my suggested changes:
line 24: update copyright

line 4531: change this to:
if api_inst.has_publisher(pref_pub):
pubs = [pref_pub]
else:
# Only publisher known is from an installed package and is not
# configured in the image.
pubs = []

lines 4560-4564: delete these

line 4603: change this to:
if p.repository:
origins = p.repository.origins
mirrors = p.repository.mirrors
else:
origins = mirrors = []

lines 4607, 4619, 4630: delete the 'r.'

Add a test case to src/tests/cli/t_pkg_publisher.py.

I think the above will get the desired result.

-Shawn

[Saurabh Vyas]

On 03/21/12 01:43, Shawn Walker wrote:
> On 03/20/12 04:32, Saurabh Vyas wrote:
>> Hi All,
>>
>> Another reminder, please if someone can provide inputs on this fix.
>> More details in mail below........
>>
>> for quick ref, webrev location :
>> https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev4/webrev/
>>
>
> So this isn't the right fix for a few reasons:
>
> * it will show the publisher returned by
> get_highest_ranked_publisher() even if that publisher is not
> explicitly configured
>
> * it potentially will not show publishers that do not have configured
> origins or mirrors

Thanks alot Shawn for these inputs.

>
> * this change needs a test case
>

...
I have made the changes as suggested, updated webrev :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev5/webrev/

>
> Add a test case to src/tests/cli/t_pkg_publisher.py.

I have added test case for this bug, ran the test suite (runs fine).
Just to be sure I just ran the test case I added on the tip of gate
without this fix, and got the traceback.

Please let me know your thoughts / comments ......

Thanks again,
Saurabh

>
> I think the above will get the desired result.
>
> -Shawn

--

Saurabh Vyas
Solaris Install Group,
Revenue Product Engineering (RPE), Systems

ORACLE India | Off Langford Road | Bangalore | 560025

|Bangalore |
Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment

[Shawn Walker]

On 03/21/12 04:24, Saurabh Vyas wrote:
...

> ...
> I have made the changes as suggested, updated webrev :
> https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev5/webrev/

src/tests/cli/t_pkg_publisher.py:
lines 435-441: This is indented one level too far, so it's currently
being treated as a nested function of test_publisher_properties.
De-indent this and re-run just this test to verify.

>>
>> Add a test case to src/tests/cli/t_pkg_publisher.py.
>
> I have added test case for this bug, ran the test suite (runs fine).
> Just to be sure I just ran the test case I added on the tip of gate
> without this fix, and got the traceback.

That's the correct way to test.

> Please let me know your thoughts / comments ......

Since I helped you write this, I can't really be your final reviewer.
Someone else will need to review this.

-Shawn

[Saurabh Vyas]

Thanks Shawn for quick response, more inline......

On 03/21/12 08:13 PM, Shawn Walker wrote:
> On 03/21/12 04:24, Saurabh Vyas wrote:
> ...
>> ...
>> I have made the changes as suggested, updated webrev :
>> https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev5/webrev/
>>
>
> src/tests/cli/t_pkg_publisher.py:
> lines 435-441: This is indented one level too far, so it's currently
> being treated as a nested function of test_publisher_properties.
> De-indent this and re-run just this test to verify.

Thanks for catching this, I have corrected the indentation.
Re-ran the test, runs successfully.

Update webrev too :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7141684-rev6/webrev/

>
>>>
>>> Add a test case to src/tests/cli/t_pkg_publisher.py.
>>
>> I have added test case for this bug, ran the test suite (runs fine).
>> Just to be sure I just ran the test case I added on the tip of gate
>> without this fix, and got the traceback.
>
> That's the correct way to test.

Thanks for confirming this.....

>
>> Please let me know your thoughts / comments ......
>
> Since I helped you write this, I can't really be your final reviewer.
> Someone else will need to review this.

Please if some one else (apart from Shawn) can provide their review
comments for this fix.

Thanks again,
Saurabh

>
> -Shawn

--

Saurabh Vyas
Solaris Install Group,

Revenue Product Engineering (RPE), Systems |Bangalore |

Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment