Re: [pkg-discuss] code review request: man page updates

[John Beck]

Danek> https://cr.opensolaris.org/action/browse/pkg/dduvall/pkg-nroff/

+1

-- John

http://blogs.oracle.com/jbeck/
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Thejaswini]

Thanks Shawn, Tim.
Please find my reply inlined.

On 02/28/13 02:49, Shawn Walker wrote:

On 02/27/13 13:11, Tim Foster wrote:

On 02/22/13 06:42 PM, Thejaswini wrote:


On 02/21/13 22:00, Shawn Walker wrote:

On 02/20/13 23:56, Thejaswini wrote:


The new rev 03 webrev is @
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev03/

This includes:
For bug "15769004 pkg unset-publisher could use a progress tracker"
- As suggested by Shawn, I have added the progress tracker in
client.py


Again, I cannot be your final reviewer, but I think you want the call
to job_add_progress *after* the call to remove_publisher() since you
haven't actually made any progress before that point.


I agree - that would be a useful change to make.

remove_publisher() is the function which takes long. If 
job_add_progress
is called after this function then the progress tracker would not 
appear

until remove_publisher() returns.
And then again as reported the command would sit displaying nothing.


Yep.


Is there a way where the tracker could display
/Updating package cache *0/*1 /before calling//remove_publisher() and
/Updating package cache *1/*1/ after remove_publisher() returns.


I'm surprised it doesn't do that already -- I'd have thought that by
calling progtrack.job_start(...) that we'd print the first line above,
then once you've actually made progress, it prints the second.

No it does not print anything after calling job_start()


I suspect this is because the JOB stuff is usually called within the 
context of image planning.


But it still seems strange as the JOB interfaces appear to be usable 
outside of that context.


My only concern here is that we're able to provide progress output 
here without adding new interfaces; that makes this suitable for 
backport if desired and keeps us from having to coordinate 
cross-consolidation.
I see in image.py that  job_add_progress() is called before the action 
is actually performed.
Looking at the definition of job_start(), it does not seem to perform 
any printing operations.

I have made changes in job_start() so that it prints 0/1.
Webrev is @ 
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev04/webrev/


- Thejaswini K.


-Shawn


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

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Shawn Walker]

On 02/27/13 13:11, Tim Foster wrote:

On 02/22/13 06:42 PM, Thejaswini wrote:


On 02/21/13 22:00, Shawn Walker wrote:

On 02/20/13 23:56, Thejaswini wrote:


The new rev 03 webrev is @
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev03/

This includes:
For bug "15769004 pkg unset-publisher could use a progress tracker"
- As suggested by Shawn, I have added the progress tracker in
client.py


Again, I cannot be your final reviewer, but I think you want the call
to job_add_progress *after* the call to remove_publisher() since you
haven't actually made any progress before that point.


I agree - that would be a useful change to make.


remove_publisher() is the function which takes long. If job_add_progress
is called after this function then the progress tracker would not appear
until remove_publisher() returns.
And then again as reported the command would sit displaying nothing.


Yep.


Is there a way where the tracker could display
/Updating package cache *0/*1 /before calling//remove_publisher() and
/Updating package cache *1/*1/ after remove_publisher() returns.


I'm surprised it doesn't do that already -- I'd have thought that by
calling progtrack.job_start(...) that we'd print the first line above,
then once you've actually made progress, it prints the second.


I suspect this is because the JOB stuff is usually called within the 
context of image planning.


But it still seems strange as the JOB interfaces appear to be usable 
outside of that context.


My only concern here is that we're able to provide progress output here 
without adding new interfaces; that makes this suitable for backport if 
desired and keeps us from having to coordinate cross-consolidation.


-Shawn

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

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Tim Foster]

On 02/22/13 06:42 PM, Thejaswini wrote:


On 02/21/13 22:00, Shawn Walker wrote:

On 02/20/13 23:56, Thejaswini wrote:


The new rev 03 webrev is @
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev03/

This includes:
For bug "15769004 pkg unset-publisher could use a progress tracker"
 - As suggested by Shawn, I have added the progress tracker in
client.py


Again, I cannot be your final reviewer, but I think you want the call
to job_add_progress *after* the call to remove_publisher() since you
haven't actually made any progress before that point.


I agree - that would be a useful change to make.


remove_publisher() is the function which takes long. If job_add_progress
is called after this function then the progress tracker would not appear
until remove_publisher() returns.
And then again as reported the command would sit displaying nothing.


Yep.


Is there a way where the tracker could display
/Updating package cache *0/*1 /before calling//remove_publisher() and
/Updating package cache *1/*1/after remove_publisher() returns.


I'm surprised it doesn't do that already -- I'd have thought that by 
calling progtrack.job_start(...) that we'd print the first line above, 
then once you've actually made progress, it prints the second.


(in your testing, are you giving the system enough work to do, such that 
there's always a long enough time period for this job to complete?)


cheers,
tim

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

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Thejaswini]

On 02/21/13 22:00, Shawn Walker wrote:

On 02/20/13 23:56, Thejaswini wrote:


The new rev 03 webrev is @
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev03/

This includes:
For bug "15769004 pkg unset-publisher could use a progress tracker"
 - As suggested by Shawn, I have added the progress tracker in 
client.py


Again, I cannot be your final reviewer, but I think you want the call 
to job_add_progress *after* the call to remove_publisher() since you 
haven't actually made any progress before that point.
remove_publisher() is the function which takes long. If job_add_progress 
is called after this function then the progress tracker would not appear 
until remove_publisher() returns.

And then again as reported the command would sit displaying nothing.

Is there a way where the tracker could display
/Updating package cache *0/*1 /before calling//remove_publisher() and
/Updating package cache *1/*1/after remove_publisher() returns.

Thanks
-Thejaswini K.




-Shawn


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

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Shawn Walker]

On 02/20/13 23:56, Thejaswini wrote:


The new rev 03 webrev is @
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev03/

This includes:
For bug "15769004 pkg unset-publisher could use a progress tracker"
 - As suggested by Shawn, I have added the progress tracker in client.py


Again, I cannot be your final reviewer, but I think you want the call to 
job_add_progress *after* the call to remove_publisher() since you 
haven't actually made any progress before that point.


-Shawn

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

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Thejaswini]

The new rev 03 webrev is @ 
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev03/


This includes:
For bug "15769004 pkg unset-publisher could use a progress tracker"
- As suggested by Shawn, I have added the progress tracker in client.py

For bug "15771543 set-publisher -p error message difficult to understand 
when publish "

#pkg set-publisher -p /myrepo/repo1 solaris  adds the origin.
#pkg set-publisher -p /myrepo/repo1 Also adds the 
origin.


Please let me know your comments on this.
Thanks
Thejaswini K

On 02/20/13 01:11, Shawn Walker wrote:

On 02/19/13 00:09, Thejaswini wrote:

Hi,

I have made the suggested changes and uploaded the webrev
@
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev02/webrev/. 



1. "15769004 pkg unset-publisher could use a progress tracker"
 The output now looks like:
 root@S12_13:~# pkg unset-publisher solaris
 PHASE  ITEMS
 Updating package cache   1/1


So one thing I noticed is that there is a distinct disadvantage to 
doing this progress tracking in image.py; notably that simply doing a 
'set-publisher' now outputs the progress message for updating the 
package cache.  That is not something we want.


So instead of placing the progress tracking in 
modules/client/image.py, I think we should consider adding it to 
client.py in publisher_unset. And instead of settings 'goal=1', we can 
also set goal=len(args) which will be nice.


I didn't test this earlier, so I apologise, and because I suggested 
the change, I can't "review" the change for modules/client/iamge.py.


I would like someone else's feedback on that change if possible.

-Shawn

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

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Shawn Walker]

On 02/19/13 00:09, Thejaswini wrote:

Hi,

I have made the suggested changes and uploaded the webrev
@
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev02/webrev/.

1. "15769004 pkg unset-publisher could use a progress tracker"
 The output now looks like:
 root@S12_13:~# pkg unset-publisher solaris
 PHASE  ITEMS
 Updating package cache   1/1


So one thing I noticed is that there is a distinct disadvantage to doing 
this progress tracking in image.py; notably that simply doing a 
'set-publisher' now outputs the progress message for updating the 
package cache.  That is not something we want.


So instead of placing the progress tracking in modules/client/image.py, 
I think we should consider adding it to client.py in publisher_unset. 
And instead of settings 'goal=1', we can also set goal=len(args) which 
will be nice.


I didn't test this earlier, so I apologise, and because I suggested the 
change, I can't "review" the change for modules/client/iamge.py.


I would like someone else's feedback on that change if possible.

-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Shawn Walker]

On 02/11/13 03:23, Thejaswini wrote:

Shawn, Tim, Thanks for looking into this and providing your comments.

In the case of output of the form:
"Removing metadata 1/100"
writing code to compute the goal, i.e. the exact no. of directories
removed, is not worth as it will again add on to the performance.

As per the bug, the no. of metadata to be removed is huge:

edp@mcescher$ du -sh /a/var/pkg/publisher/solaris
  3.2G   /a/var/pkg/publisher/solaris
edp@mcescher$ find /a/var/pkg/publisher/solaris -type f | wc -l
   107422
edp@mcescher$ find /a/var/pkg/publisher/solaris -type d | wc -l
 2385

In such cases, as suggested by Shawn something like below would be good
enough.
# pkg unset-publisher solaris
PHASE  ITEMS
Updating package cache   1/1

IMO we could add an additional jobitem and use it while calling the
progress tracker which would display "Removing package metadata" instead
of   " Updating package cache".


The thing is, I don't know that we need to be specific; "updating 
package cache" is accurate as the package data we're removing is stored 
in the package cache.


-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Thejaswini]

Hi,

I have made the suggested changes and uploaded the webrev
@ 
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev02/webrev/.


1. "15769004 pkg unset-publisher could use a progress tracker"
The output now looks like:
root@S12_13:~# pkg unset-publisher solaris
PHASE  ITEMS
Updating package cache   1/1

2. 15771543 set-publisher -p error message difficult to understand when 
publish

#pkg set-publisher -p /myrepo/repo1 solaris  adds the origin.
#pkg set-publisher -p /myrepo/repo1 Also adds the 
origin.


I have modified 2 testcases to pass after my changes.

Thanks,
Thejaswini K

Oracle
Thejaswini K
Revenue Product Engineering (RPE), Systems
Phone: +91 8066927709 | Mobile: +91 9663324594
ORACLE India | Off Langford Road | Bangalore | 560025
Green Oracle  Oracle is committed to 
developing practices and products that help protect the environment


On 02/12/13 20:30, Shawn Walker wrote:
No, it should add the new origin to the publisher, it should not 
remove any origins.


--
Shawn

On Feb 11, 2013, at 10:51 PM, Thejaswini > wrote:


This is with regard to bug "*Bug 15771543 - SUNBT7143562 
set-publisher -p should simply update existing publisher sources"


*Below is my understanding on how -p should function:
If solaris publisher is already configured on the system:
root@S12_13:~# pkg publisher
PUBLISHER   TYPE STATUS P LOCATION
solaris origin   online F 
file:///root/myrepo/*repo1*/


And then I issue the command "pkg set-publisher -p /root/myrepo/repo2 
solaris"

this should update the location of the publisher configured.

i.e.
root@S12_13:~# pkg publisher
PUBLISHER   TYPE STATUS P LOCATION
solaris origin   online F 
file:///root/myrepo/*repo2*/



Please correct me if I am wrong.

Thanks,
Thejaswini K.

Thejaswini K
Revenue Product Engineering (RPE), Systems
Phone: +91 8066927709 | Mobile: +91 9663324594
ORACLE India | Off Langford Road | Bangalore | 560025
  Oracle 
is committed to developing practices and products that help protect 
the environment


On 02/11/13 16:53, Thejaswini wrote:

Shawn, Tim, Thanks for looking into this and providing your comments.

In the case of output of the form:
"Removing metadata 1/100"
writing code to compute the goal, i.e. the exact no. of directories 
removed, is not worth as it will again add on to the performance.


As per the bug, the no. of metadata to be removed is huge:

edp@mcescher$ du -sh /a/var/pkg/publisher/solaris
 3.2G   /a/var/pkg/publisher/solaris
edp@mcescher$ find /a/var/pkg/publisher/solaris -type f | wc -l
  107422
edp@mcescher$ find /a/var/pkg/publisher/solaris -type d | wc -l
2385

In such cases, as suggested by Shawn something like below would be 
good enough.

# pkg unset-publisher solaris
PHASE  ITEMS
Updating package cache   1/1

IMO we could add an additional jobitem and use it while calling the 
progress tracker which would display "Removing package metadata" 
instead of   " Updating package cache".

The output would look something like:
# pkg unset-publisher solaris
PHASE  ITEMS
Removing package metadata1/1

Is it fine to add this Job item?

Regarding the changes for 15771543, I would remove the check present 
and see if additional changes needs to be done.


Will send the webrev once I have the changes tested.

Thanks,
Thejaswini K

Oracle
Thejaswini K
Revenue Product Engineering (RPE), Systems
Phone: +91 8066927709 | Mobile: +91 9663324594
ORACLE India | Off Langford Road | Bangalore | 560025
Green Oracle  Oracle is committed 
to developing practices and products that help protect the environment


On 02/11/13 02:45, Tim Foster wrote:

On 02/ 9/13 08:12 AM, Shawn Walker wrote:

On 02/07/13 19:15, Tim Foster wrote:

"Removing metadata 1/100"


The problem with that idea is that we don't know how much metadata 
there

is to remove and since we're not actually removing the metadata
piece-by-piece (we use rmtree) we can't provide meaningful 
progress for

each metadata item as it is removed.


We do have some visibility into what we're removing, in some parts 
of the code[1] we're iterating over several directories looking for 
candidate directories to remove, calling shutil.rmtree when we need 
to.


Some shutil.rmtree calls will take longer than others, I'd rather a 
counter telling me how many directory-trees of content to consider 
removing are left, than just a random spinning '|/-\|-\'.


You're right in that when we're removing an entire publisher (line 
3422), we've no visibility into the metadata removal, but not with 
the rest of the code (perhaps in that case, meta

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Thejaswini]

Shawn, Tim, Thanks for looking into this and providing your comments.

In the case of output of the form:
"Removing metadata 1/100"
writing code to compute the goal, i.e. the exact no. of directories 
removed, is not worth as it will again add on to the performance.


As per the bug, the no. of metadata to be removed is huge:

edp@mcescher$ du -sh /a/var/pkg/publisher/solaris
 3.2G   /a/var/pkg/publisher/solaris
edp@mcescher$ find /a/var/pkg/publisher/solaris -type f | wc -l
  107422
edp@mcescher$ find /a/var/pkg/publisher/solaris -type d | wc -l
2385

In such cases, as suggested by Shawn something like below would be good 
enough.

# pkg unset-publisher solaris
PHASE  ITEMS
Updating package cache   1/1

IMO we could add an additional jobitem and use it while calling the 
progress tracker which would display "Removing package metadata" instead 
of   " Updating package cache".

The output would look something like:
# pkg unset-publisher solaris
PHASE  ITEMS
Removing package metadata1/1

Is it fine to add this Job item?

Regarding the changes for 15771543, I would remove the check present and 
see if additional changes needs to be done.


Will send the webrev once I have the changes tested.

Thanks,
Thejaswini K

Oracle
Thejaswini K
Revenue Product Engineering (RPE), Systems
Phone: +91 8066927709 | Mobile: +91 9663324594
ORACLE India | Off Langford Road | Bangalore | 560025
Green Oracle  Oracle is committed to 
developing practices and products that help protect the environment


On 02/11/13 02:45, Tim Foster wrote:

On 02/ 9/13 08:12 AM, Shawn Walker wrote:

On 02/07/13 19:15, Tim Foster wrote:

"Removing metadata 1/100"



The problem with that idea is that we don't know how much metadata there
is to remove and since we're not actually removing the metadata
piece-by-piece (we use rmtree) we can't provide meaningful progress for
each metadata item as it is removed.


We do have some visibility into what we're removing, in some parts of 
the code[1] we're iterating over several directories looking for 
candidate directories to remove, calling shutil.rmtree when we need to.


Some shutil.rmtree calls will take longer than others, I'd rather a 
counter telling me how many directory-trees of content to consider 
removing are left, than just a random spinning '|/-\|-\'.


You're right in that when we're removing an entire publisher (line 
3422), we've no visibility into the metadata removal, but not with the 
rest of the code (perhaps in that case, metadata removal is so fast 
it's not worth considering emitting progress?)


cheers
tim

[1] 
http://src.opensolaris.org/source/xref/pkg/gate/src/modules/client/image.py#3424 
ish

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

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Tim Foster]

On 02/ 9/13 08:12 AM, Shawn Walker wrote:

On 02/07/13 19:15, Tim Foster wrote:

"Removing metadata 1/100"



The problem with that idea is that we don't know how much metadata there
is to remove and since we're not actually removing the metadata
piece-by-piece (we use rmtree) we can't provide meaningful progress for
each metadata item as it is removed.


We do have some visibility into what we're removing, in some parts of 
the code[1] we're iterating over several directories looking for 
candidate directories to remove, calling shutil.rmtree when we need to.


Some shutil.rmtree calls will take longer than others, I'd rather a 
counter telling me how many directory-trees of content to consider 
removing are left, than just a random spinning '|/-\|-\'.


You're right in that when we're removing an entire publisher (line 
3422), we've no visibility into the metadata removal, but not with the 
rest of the code (perhaps in that case, metadata removal is so fast it's 
not worth considering emitting progress?)


cheers
tim

[1] 
http://src.opensolaris.org/source/xref/pkg/gate/src/modules/client/image.py#3424 
ish

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

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Shawn Walker]

On 02/07/13 19:15, Tim Foster wrote:
...

General: how long does metadata removal usually take? Rather than a single:

"Removing metadata ... done"

prompt, would it be useful to have a rolling progress, showing how we're
doing when removing bits of metadata instead? eg.

"Removing metadata 1/100"

In the case of the example in the bug report, 16 minutes is a very long
time to be waiting with no feedback other than, effectively "I'm busy".


The problem with that idea is that we don't know how much metadata there 
is to remove and since we're not actually removing the metadata 
piece-by-piece (we use rmtree) we can't provide meaningful progress for 
each metadata item as it is removed.


The progress tracker should have a spinner that outputs from 
time-to-time for cases like these (I thought it did?).


-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Shawn Walker]

On 02/08/13 11:06, Shawn Walker wrote:
...

As for the progress changes for 15769004, we shouldn't be adding new
methods to the progress tracker unless absolutely necessary. I'd suggest
using the generic JOB progress mechanism instead; see the attached patch.


I'd note that you don't have to implement it this way; you could use one 
of the other progress methods that already exist.


This is the progress output you would see if you use my suggest set of 
changes:


# pkg unset-publisher solaris
PHASE  ITEMS
Updating package cache   1/1

-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Shawn Walker]

On 02/05/13 00:21, Thejaswini wrote:

Hi all,

The below webrev contains the fix for the following bugs:

15771543 set-publisher -p error message difficult to understand when
publish
- The fix is that the error message is made more clear.
15769004 pkg unset-publisher could use a progress tracker
- Here I have added progress tracker logs which would appear while
deleting the publisher metadata.

https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543/


I think these should really be separate changesets instead of the same 
one; they're not really related changes.


To add to that, I think the changes for 15771543 should not be made, or 
rather, are the wrong set of changes to make.


Among the pkg(5) team, we discussed a long time ago whether -p should 
simply accept new package sources, and the consensus was that now that 
we support composition (and package signing), it should be safe to do 
so.  I meant to fix this in S11 FCS but failed to file an RFE for it (or 
I can't find the RFE I filed).


There is little value in enforcing the current check, so I'd suggest the 
right change is to instead delete lines 4489-4496 and 4500-4512 in the 
original src/client.py.  So, I'd repurpose the bug as "set-publisher -p 
should simply update existing publisher sources".


As for the progress changes for 15769004, we shouldn't be adding new 
methods to the progress tracker unless absolutely necessary.  I'd 
suggest using the generic JOB progress mechanism instead; see the 
attached patch.


-Shawn
diff --git a/src/modules/client/image.py b/src/modules/client/image.py
--- a/src/modules/client/image.py
+++ b/src/modules/client/image.py
@@ -3419,8 +3419,18 @@ in the environment or by setting simulat
 if f.publisher == pub.prefix
 ]
 
+if not progtrack:
+progtrack = progress.NullProgressTracker()
+
+progtrack.job_start(progtrack.JOB_PKG_CACHE, goal=1)
+
 if not excluded:
-pub.remove_meta_root()
+try:
+pub.remove_meta_root()
+finally:
+progtrack.job_add_progress(
+progtrack.JOB_PKG_CACHE)
+progtrack.job_done(progtrack.JOB_PKG_CACHE)
 else:
 try:
 # Discard all publisher metadata except
@@ -3465,6 +3475,10 @@ in the environment or by setting simulat
 pub.prefix), ignore_errors=True)
 except EnvironmentError, e:
 raise apx._convert_error(e)
+finally:
+progtrack.job_add_progress(
+progtrack.JOB_PKG_CACHE)
+progtrack.job_done(progtrack.JOB_PKG_CACHE)
 
 if rebuild:
 self.__rebuild_image_catalogs(progtrack=progtrack)
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for pkg set/unset publisher bug fix.

[Tim Foster]

Hi there,

On 02/ 5/13 09:21 PM, Thejaswini wrote:

The below webrev contains the fix for the following bugs:

15771543 set-publisher -p error message difficult to understand when publish
  - The fix is that the error message is made more clear.
15769004 pkg unset-publisher could use a progress tracker
  - Here I have added progress tracker logs which would appear
while deleting the publisher metadata.

https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543/


src/client.py

line 4504, it's ok to delete this blank line I think.


src/modules/client/progress.py

line 1205, why are we calling a method "_load_metadata_output" when 
we're actually removing metadata.  I think it would make more sense to 
call the method on line 1960, 2355 "_remove_metadata_output"?


line 1962, you don't need the continuation '\' character because you're 
using them within parentheses.



General: how long does metadata removal usually take? Rather than a single:

"Removing metadata ...  done"

prompt, would it be useful to have a rolling progress, showing how we're 
doing when removing bits of metadata instead?  eg.


"Removing metadata   1/100"

In the case of the example in the bug report, 16 minutes is a very long 
time to be waiting with no feedback other than, effectively "I'm busy".


cheers,
tim
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request

[Bart Smaalders]

On 01/31/13 14:30, Shawn Walker wrote:

imageplan.py:


line 769: Shouldn't the len() != 2 be first? Faster anyway...


Actually, the latter is an error case... so odds are good both
get checked.


line 3256: what changed here?


tab to spaces. I untabified the file.

- Bart




--
Bart Smaalders  Solaris Core OS
bart.smaald...@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-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request

[Shawn Walker]

On 01/25/13 15:19, Bart Smaalders wrote:

<  I actually got time to write some code again>

We're adding support for revert-tags on directories to
delete unpackaged content. This is useful for cleaning
up log files, etc. when reverting a system to a clean state.
revert-tag=daleks=terminate.* When pkg revert --tagged daleks
is run, any files or directories (recursively) that match
"terminate.*" and are not packaged in the tagged directory
are removed. This doesn't work on mount points or across
file system mounts.

https://cr.opensolaris.org/action/browse/pkg/barts/directory_revert_tags/


=
src/man/pkg.1:
  copyright update

  line 1394: stray whitespace at end of line?

  line 1409: s/unpacked/unpackaged/ ?

  line 1409: s/See /See "File Actions" in the /

=
src/man/pkg.5:
  copyright update

  line 238: s/  This attribute/ This attribute/

  line 244: This line may need updating to include "revert-tag" and to 
see "File Actions" for details.  The current text seems to imply that 
only a few of the file action tags are permitted.



=
src/modules/client/imageplan.py:
  copyright update

  line 769: Shouldn't the len() != 2 be first? Faster anyway...

  lines 771, 835: s/f,m/f, m/

  line 833: if revert_dirs is not being modified during iteration, it 
is more efficient to simply "for f, m in revert_dirs:" as calling 
.keys() returns a private copy of the list of keys.


  lines 864, 892: extra newline

  line 871: s/# if/# If/

  line 885: s/# a/# A/

  line 891: de-indent 4 spaces

  line 907: This will construct a new list; since you don't actually
need a new list, you could also do:

for name in itertools.chain(dirnames, filenames):

   ...but this may be premature optimisation ;-)


  line 916: s/w/W/

  line 918: missing '.'

  line 3256: what changed here?

=
src/tests/cli/t_pkg_revert.py:
  copyright update

  line 129: insert newline before

  line 212: change # into """ """ docstring

  line 234: s/unpacked/unpackaged/ ?

-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request

[Bart Smaalders]

On 01/25/13 15:29, Alan Coopersmith wrote:

So if I'm understanding this right, we might add in the xorg package
something like:
dir path=var/log revert-tag=logs=Xorg\..*\.log.*
and then pkg revert --tagged logs will nuke them, right?


Exactly.

- Bart


--
Bart Smaalders  Solaris Core OS
bart.smaald...@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-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request

[Alan Coopersmith]

On 01/25/13 03:19 PM, Bart Smaalders wrote:
> < I actually got time to write some code again>
> 
> We're adding support for revert-tags on directories to 
> delete unpackaged content. This is useful for cleaning 
> up log files, etc. when reverting a system to a clean state. 
> revert-tag=daleks=terminate.* When pkg revert --tagged daleks 
> is run, any files or directories (recursively) that match 
> "terminate.*" and are not packaged in the tagged directory 
> are removed. This doesn't work on mount points or across 
> file system mounts.

So if I'm understanding this right, we might add in the xorg package
something like:
dir path=var/log revert-tag=logs=Xorg\..*\.log.*
and then pkg revert --tagged logs will nuke them, right?

-- 
-Alan Coopersmith-  alan.coopersm...@oracle.com
 Oracle Solaris Engineering - http://blogs.oracle.com/alanc
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request - 15966328 - pkg update should display SRU index URL not 11.1 release notes

[Shawn Walker]

On 12/11/12 03:51, Albert White wrote:

On 12/11/12 11:49 AM, Albert White wrote:

Hi,

Can I get a review for this small change for the SRU branch please?

As with previous similar changes the URL points to the index page for
the SRU's.

Ah yes, a point to the webrev would help...

https://cr.opensolaris.org/action/browse/pkg/albertw/15966328/webrev/


+1

-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request - 15966328 - pkg update should display SRU index URL not 11.1 release notes

[Albert White]

On 12/11/12 11:49 AM, Albert White wrote:

Hi,

Can I get a review for this small change for the SRU branch please?

As with previous similar changes the URL points to the index page for the 
SRU's. 

Ah yes, a point to the webrev would help...

https://cr.opensolaris.org/action/browse/pkg/albertw/15966328/webrev/

Cheers,
~Al

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

Re: [pkg-discuss] code review request for CR 7139743 (s11-update branch)

[Shawn Walker]

On 10/16/12 01:30, Abhinandan Ekande wrote:

Folks,

Please review backport of CR to s11-update branch :
7139743 license actions without a path can cause a stacktrace

webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7139743-11.2-rev1/webrev/


Unit testing has been using both pkgsend and pkgdepend commands.

The test suite run was successful.


+1

-Shawn

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

Re: [pkg-discuss] code review request for CR 7139743 (s11-update branch)

[Thejaswini]

Changes look good to me for s11-update branch.

- Thejaswini

On 10/16/12 14:00, Abhinandan Ekande wrote:

Folks,

Please review backport of CR to s11-update branch :
7139743 license actions without a path can cause a stacktrace

webrev : 
https://cr.opensolaris.org/action/browse/pkg/ae112802/7139743-11.2-rev1/webrev/


Unit testing has been using both pkgsend and pkgdepend commands.

The test suite run was successful.

Thanks,
Abhi.
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

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

Re: [pkg-discuss] code review request for CR 7197669 (s11-update branch)

[Saurabh Vyas]

On 10/15/12 15:32, Abhinandan Ekande wrote:

Folks,

Please review backport for CR to s11-update :
7197669 mediators and conflicting action fixup can fail with
signature-policy require-signatures

webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7197669-rev1/webrev/

Unit testing has been done.

Ran the IPS test suite and all tests passed.


This backport looks fine.

Saurabh



Thanks,
Abhi.
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss



--


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

ORACLE India | Off Langford Road | Bangalore | 560025

|Bangalore |
Green Oracle  Oracle is committed to
developing practices and products that help protect the environment
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7140762

[Danek Duvall]

Abhinandan Ekande wrote:

> On 08/10/12 00:21, Danek Duvall wrote:
> >t_pkgrepo.py:
> >
> >   - line 1372: We only test both because the OS happens to test for the
> > EINVAL condition before the EEXIST condition.  It could go the other
> > way around, and then we'd only be testing the EEXIST condition.
> >>If multiple versions exists in repository for a package and if "pkgrepo
> >>remove" is executed from package directory, then first error always will
> >>be EINVAL.
> >Is that dictated by anything other than the current implementation of the
> >VFS or filesystem code?  I don't know why you can rely on that.
> 
> As per filesystem code the EINVAL will occur before EEXIST.

Right.  My question is, is there any standard preventing the ZFS guys from
going and reversing the order of the tests for whatever reason they feel
like, thereby breaking our test, or is this error ordering defined
somewhere?  If it's not defined, then you need to be more careful.

> Also what I meant by comment at line 1372 was that the removal
> of truck@1.0 will verify both error code paths in rmdir() definition
> in repository.py.

I know what you meant, but it's not generally true, even if it happens to
be true today for ZFS (or maybe today for all filesystems?).

> In addition I have made changes to check for truck@2 after removing
> truck@1.0, for example :
> +# Verify truck@2 still exists in repo
> +self.pkgrepo("list -s %s truck@2.0,5.11-0" % src_repo)

I don't see how that helps.

Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7139743

[Abhinandan Ekande]

Thanks Saurabh and Shawn for the review.
I have attached the patch for CR 7139743.
Could you please integrate it.

Abhi.

On 09/24/12 15:12, Saurabh Vyas wrote:

Hi Abhi,

These changes LGTM.

Regards,
Saurabh

On 09/24/12 14:27, Abhinandan Ekande wrote:

Folks,

Per the latest webrev, for license action when hash attribute is missing
the
following error would be printed.

pkgsend: invalid action, 'license NOHASH license=copyright': Empty
or invalid path attribute

I had offline conversation with Shawn about this error message and it is
acceptable to him. Please let me know your opinion about this error
message.

Thanks,
Abhi.

On 09/14/12 16:16, Abhinandan Ekande wrote:

Shawn,

On 09/13/12 23:18, Shawn Walker wrote:

src/modules/actions/__init__.py:

lines 348-352: In general, when writing things in Python, the
preferred approach is to "leap first, then look". That is,
don't check for a key or value, simply perform the action and
catch the exception raised and handle it. In addition, the
exception raised here assumes that because there is no "path"
that this is a license action. That's not necessarily true.



Thanks for the feedback.


So instead of:

if "path" in action.attrs and payload == "NOHASH":
filepath = os.path.sep + action.attrs["path"]
elif "path" not in action.attrs and payload == "NOHASH":
raise ActionDataError(_("Hash attribute is missing "
"for license action"))

...I'd try this:

if payload == "NOHASH":
try:
filepath = os.path.sep + action.attrs["path"]
except KeyError:
raise InvalidPathAttributeError(action)



I followed the above approach. Here is updated webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7139743-rev2/webrev/ 






src/modules/publish/dependencies.py:
I don't think this change is needed if you do the above.


Yes, not needed with new approach.

Abhi.



We may need to tweak this, so can you try that and see what you get?

-Shawn

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

___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss 
# HG changeset patch
# User abhinandan.eka...@oracle.com
# Date 1348565222 -19800
# Node ID 3ddcefe9124de9973b4c94468ecc1696229ef212
# Parent  0358e521cc2d9658d2a8d8ccd92112f2605a12ab
7139743 license actions without a path can cause a stacktrace

diff -r 0358e521cc2d -r 3ddcefe9124d src/modules/actions/__init__.py
--- a/src/modules/actions/__init__.py   Fri Sep 14 10:29:11 2012 +0530
+++ b/src/modules/actions/__init__.py   Tue Sep 25 14:57:02 2012 +0530
@@ -346,7 +346,10 @@
 return None, None
 
 if payload == "NOHASH":
-filepath = os.path.sep + action.attrs["path"]
+try:
+filepath = os.path.sep + action.attrs["path"]
+except KeyError:
+raise InvalidPathAttributeError(action)
 else:
 filepath = payload
 
diff -r 0358e521cc2d -r 3ddcefe9124d src/tests/cli/t_pkgsend.py
--- a/src/tests/cli/t_pkgsend.pyFri Sep 14 10:29:11 2012 +0530
+++ b/src/tests/cli/t_pkgsend.pyTue Sep 25 14:57:02 2012 +0530
@@ -1269,6 +1269,17 @@
 self.pkgsend("", "-s %s publish %s" %
 (self.dc.get_repo_url(), mfpath), exit=1)
 
+def test_25_pkgsend_publish_nohash_license(self):
+"""Verify that publishing a manifest with hash attribute
+missing for license action doesn't traceback"""
+
+durl = self.dc.get_depot_url()
+# Should fail because hash attribute is missing.
+self.pkgsend_bulk(durl,
+"""open foo@1.0
+add license license=copyright
+close""", exit=1)
+
 
 class TestPkgsendHardlinks(pkg5unittest.CliTestCase):
 
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7139743

[Saurabh Vyas]

Hi Abhi,

These changes LGTM.

Regards,
Saurabh

On 09/24/12 14:27, Abhinandan Ekande wrote:

Folks,

Per the latest webrev, for license action when hash attribute is missing
the
following error would be printed.

pkgsend: invalid action, 'license NOHASH license=copyright': Empty
or invalid path attribute

I had offline conversation with Shawn about this error message and it is
acceptable to him. Please let me know your opinion about this error
message.

Thanks,
Abhi.

On 09/14/12 16:16, Abhinandan Ekande wrote:

Shawn,

On 09/13/12 23:18, Shawn Walker wrote:

src/modules/actions/__init__.py:

lines 348-352: In general, when writing things in Python, the
preferred approach is to "leap first, then look". That is,
don't check for a key or value, simply perform the action and
catch the exception raised and handle it. In addition, the
exception raised here assumes that because there is no "path"
that this is a license action. That's not necessarily true.



Thanks for the feedback.


So instead of:

if "path" in action.attrs and payload == "NOHASH":
filepath = os.path.sep + action.attrs["path"]
elif "path" not in action.attrs and payload == "NOHASH":
raise ActionDataError(_("Hash attribute is missing "
"for license action"))

...I'd try this:

if payload == "NOHASH":
try:
filepath = os.path.sep + action.attrs["path"]
except KeyError:
raise InvalidPathAttributeError(action)



I followed the above approach. Here is updated webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7139743-rev2/webrev/




src/modules/publish/dependencies.py:
I don't think this change is needed if you do the above.


Yes, not needed with new approach.

Abhi.



We may need to tweak this, so can you try that and see what you get?

-Shawn

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

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



--


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

ORACLE India | Off Langford Road | Bangalore | 560025

|Bangalore |
Green Oracle  Oracle is committed to
developing practices and products that help protect the environment
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7139743

[Abhinandan Ekande]

Folks,

Per the latest webrev, for license action when hash attribute is missing the
following error would be printed.

   pkgsend: invalid action, 'license NOHASH license=copyright': Empty
   or invalid path attribute

I had offline conversation with Shawn about this error message and it is
acceptable to him. Please let me know your opinion about this error message.

Thanks,
Abhi.

On 09/14/12 16:16, Abhinandan Ekande wrote:

Shawn,

On 09/13/12 23:18, Shawn Walker wrote:

src/modules/actions/__init__.py:

  lines 348-352:  In general, when writing things in Python, the
preferred approach is to "leap first, then look".  That is,
don't check for a key or value, simply perform the action and
catch the exception raised and handle it.  In addition, the
exception raised here assumes that because there is no "path"
that this is a license action.  That's not necessarily true.



Thanks for the feedback.


So instead of:

if "path" in action.attrs and payload == "NOHASH":
filepath = os.path.sep + action.attrs["path"]
elif "path" not in action.attrs and payload == "NOHASH":
raise ActionDataError(_("Hash attribute is missing "
"for license action"))

...I'd try this:

if payload == "NOHASH":
try:
filepath = os.path.sep + action.attrs["path"]
except KeyError:
raise InvalidPathAttributeError(action)



I followed the above approach. Here is updated webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7139743-rev2/webrev/ 





src/modules/publish/dependencies.py:
  I don't think this change is needed if you do the above.


Yes, not needed with new approach.

Abhi.



We may need to tweak this, so can you try that and see what you get?

-Shawn

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

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

Re: [pkg-discuss] code review request for CR 7139743

[Abhinandan Ekande]

Shawn,

On 09/13/12 23:18, Shawn Walker wrote:

src/modules/actions/__init__.py:

  lines 348-352:  In general, when writing things in Python, the
preferred approach is to "leap first, then look".  That is,
don't check for a key or value, simply perform the action and
catch the exception raised and handle it.  In addition, the
exception raised here assumes that because there is no "path"
that this is a license action.  That's not necessarily true.



Thanks for the feedback.


So instead of:

if "path" in action.attrs and payload == "NOHASH":
filepath = os.path.sep + action.attrs["path"]
elif "path" not in action.attrs and payload == "NOHASH":
raise ActionDataError(_("Hash attribute is missing "
"for license action"))

...I'd try this:

if payload == "NOHASH":
try:
filepath = os.path.sep + action.attrs["path"]
except KeyError:
raise InvalidPathAttributeError(action)



I followed the above approach. Here is updated webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7139743-rev2/webrev/



src/modules/publish/dependencies.py:
  I don't think this change is needed if you do the above.


Yes, not needed with new approach.

Abhi.



We may need to tweak this, so can you try that and see what you get?

-Shawn

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

Re: [pkg-discuss] code review request for CR 7139743

[Shawn Walker]

On 09/13/12 02:00, Abhinandan Ekande wrote:

Folks,

Please review fix for CR :
7139743 license actions without a path can cause a stacktrace

webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7139743-rev1/webrev/

Unit testing done for both pkgsend and pkgdepend commands.

Ran the IPS test suite. All tests passed.


src/modules/actions/__init__.py:

  lines 348-352:  In general, when writing things in Python, the
preferred approach is to "leap first, then look".  That is,
don't check for a key or value, simply perform the action and
catch the exception raised and handle it.  In addition, the
exception raised here assumes that because there is no "path"
that this is a license action.  That's not necessarily true.

So instead of:

if "path" in action.attrs and payload == "NOHASH":
filepath = os.path.sep + action.attrs["path"]
elif "path" not in action.attrs and payload == "NOHASH":
raise ActionDataError(_("Hash attribute is missing "
"for license action"))

...I'd try this:

if payload == "NOHASH":
try:
filepath = os.path.sep + action.attrs["path"]
except KeyError:
raise InvalidPathAttributeError(action)


src/modules/publish/dependencies.py:
  I don't think this change is needed if you do the above.

We may need to tweak this, so can you try that and see what you get?

-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: prevent publication of ancient packages

[Shawn Walker]

On 09/10/12 10:58, Danek Duvall wrote:

I noticed when doing an S12 build that it published all our old SUNW*
packages and some of the others that we've renamed over time.  That should
only happen when PUBLISHALL is set.  Turns out I just didn't take this into
account when doing the S12 build changeset.  I took this opportunity to
just remove the manifests of the two obsoleted packages we were carrying
around.

 https://cr.opensolaris.org/action/browse/pkg/dduvall/nopublish/


LGTM,

-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: 7193711

[Edward Pilatowicz]

On Sat, Sep 08, 2012 at 09:44:40AM -0700, Danek Duvall wrote:
> Edward Pilatowicz wrote:
>
> > > > 
> > > > https://cr.opensolaris.org/action/browse/pkg/edp/pkg.change-facet/webrev
>
> This seems to have moved to
>
> https://cr.opensolaris.org/action/browse/pkg/edp/pkg.chage-facet/webrev
>
> > let me know if this is ok.
>
> Yeah, this is fine.  Just change "accidently" to "accidentally".  And line
> 262 doesn't need a trailing semicolon.
>

done.
sorry for the typos.
thanks for the review.
ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: 7193711

[Danek Duvall]

Edward Pilatowicz wrote:

> > > 
> > > https://cr.opensolaris.org/action/browse/pkg/edp/pkg.change-facet/webrev

This seems to have moved to

https://cr.opensolaris.org/action/browse/pkg/edp/pkg.chage-facet/webrev

> let me know if this is ok.

Yeah, this is fine.  Just change "accidently" to "accidentally".  And line
262 doesn't need a trailing semicolon.

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: 7193711

[Edward Pilatowicz]

thanks for looking at this.
the webrev is updated.
replies inline below.
ed

On Thu, Sep 06, 2012 at 11:58:36AM -0700, Danek Duvall wrote:
> Edward Pilatowicz wrote:
>
> > https://cr.opensolaris.org/action/browse/pkg/edp/pkg.change-facet/webrev
>
> imageplan.py:
>
>   - line 317: Python doesn't need parens here
>
>   - line 368: can this be re-flowed now?
>
> t_change_facet.py:
>
>   - line 252: this doesn't read well.  Perhaps you mean something like
> "Verify that resetting a facet explicitly set to false restores the
> delivered content."?
>

all addressed.

>   - I'll also point out that this test seems to do more than what either
> the name or the docstring claim.  Are we missing these basic tests
> (like the graf starting at line 291) elsewhere?  The last three tests
> (from line 306 on) seem relevant.  Not sure about the rest.
>

in the new test case there are 3 setup steps (for which i verify the
results after each setup step), then one test that doesn't really match
the docstring, and then 3 tests which match the docstring (and require
the configuration created by the first three setup steps).

i've moved the one test in the middle (which verifies that installing a
random package doesn't change facets and doesn't match the docstring
claim) into it's own test.

i could create additional seperate tests for for the remaining 3 setup
steps, and then re-write the test such that it does setup without any
verification, but given how short the test case is, that seems kinda
like overkill.

let me know if this is ok.

>   - line 291: "an random" -> "a random"
>

done.
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: 7193711

[Danek Duvall]

Edward Pilatowicz wrote:

> https://cr.opensolaris.org/action/browse/pkg/edp/pkg.change-facet/webrev

imageplan.py:

  - line 317: Python doesn't need parens here

  - line 368: can this be re-flowed now?

t_change_facet.py:

  - line 252: this doesn't read well.  Perhaps you mean something like
"Verify that resetting a facet explicitly set to false restores the
delivered content."?

  - I'll also point out that this test seems to do more than what either
the name or the docstring claim.  Are we missing these basic tests
(like the graf starting at line 291) elsewhere?  The last three tests
(from line 306 on) seem relevant.  Not sure about the rest.

  - line 291: "an random" -> "a random"

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request : CR 7168378, 7168375

[Danek Duvall]

Takeshi Asano wrote:

> I understand the .po update are syncup with result
> of the functional bugfixes, rather than the functional
> bugfixes themselves, right?
> 
> Basically could you please exclude the .po file update from
> the changeset?
> 
> The string changes in .py are to be incorporated into English
> pkg.pot at build time and at some builds translations are synced
> up with newer version of English pkg.pot.
> Not usually updating them manually.

The reason I suggested that he update them manually was that the problems
were simply typos that wouldn't cause the translations to change when the
English was fixed, but would cause the translations to fail to appear
unless the .po files were fixed to match.

Since there's often a long lag between the time the messages change and the
time we get updated translations and it's a safe fix, it seemed
user-friendly to do the update on the engineering side, thereby allowing
what might have ended up being a post-l10n deadline message change (or,
now, keeping the translations working for the next couple of years while
S12 is in development).

If there was something incorrect about the change, or a mistake I made in
the risk analysis of the situation, I'd like to understand the mistake.

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request : CR 7168378, 7168375

[Danek Duvall]

Saurabh Vyas wrote:

> https://cr.opensolaris.org/action/browse/pkg/saurabhv/7168375-rev3/webrev/

LGTM.

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request : CR 7168378, 7168375

[Saurabh Vyas]

On 08/30/12 12:15, Takeshi Asano wrote:

Hi Saurabh,

I understand the .po update are syncup with result
of the functional bugfixes, rather than the functional
bugfixes themselves, right?

Basically could you please exclude the .po file update from
the changeset?

The string changes in .py are to be incorporated into English
pkg.pot at build time and at some builds translations are synced
up with newer version of English pkg.pot.
Not usually updating them manually.

There can be exceptional case to do urgent .po fix e.g. .po file
content causes functional problem.


Thanks Takeshi for your comments & providing inputs on the process to
modify .po files.

I have updated the webrev & taken out changes to .po files.
webrev : 
https://cr.opensolaris.org/action/browse/pkg/saurabhv/7168375-rev3/webrev/


Any comments on this fix is greatly appreciated.

Thanks,
~Saurabh



Thanks,
Takeshi

On 2012年08月30日 15:05, Saurabh Vyas wrote:

Hi All,

I am re-instantiating the review for couple of trivial pkgmogrify
issues

7168375 pkgmogrify missing misc import
7168378 pkgmogrify can trace back if arguments aren't provided to
delete operation

Webrev :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/7168378-rev2/webrev/


Please if you can provide your comments on this fix.

Thanks in advance,
Saurabh
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss


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



--


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

ORACLE India | Off Langford Road | Bangalore | 560025

|Bangalore |
Green Oracle  Oracle is committed to
developing practices and products that help protect the environment
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request : CR 7168378, 7168375

[Takeshi Asano]

Hi Saurabh,

I understand the .po update are syncup with result
of the functional bugfixes, rather than the functional
bugfixes themselves, right?

Basically could you please exclude the .po file update from
the changeset?

The string changes in .py are to be incorporated into English
pkg.pot at build time and at some builds translations are synced
up with newer version of English pkg.pot.
Not usually updating them manually.

There can be exceptional case to do urgent .po fix e.g. .po file
content causes functional problem.

Thanks,
Takeshi

On 2012年08月30日 15:05, Saurabh Vyas wrote:

Hi All,

I am re-instantiating the review for couple of trivial pkgmogrify
issues

7168375 pkgmogrify missing misc import
7168378 pkgmogrify can trace back if arguments aren't provided to delete 
operation

Webrev : 
https://cr.opensolaris.org/action/browse/pkg/saurabhv/7168378-rev2/webrev/

Please if you can provide your comments on this fix.

Thanks in advance,
Saurabh
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss


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

Re: [pkg-discuss] code review request: 7193711

[Edward Pilatowicz]

hey all,

could i get a second reviewer on this fix?  (shawn looked at it, but he
mostly came up with the fix to begin with.)

thanks,
ed

On Mon, Aug 27, 2012 at 06:35:30PM -0700, Edward Pilatowicz wrote:
> hey all,
>
> shawn found a facet bug that in introduced with the original linked
> images putback (oops).  i've got a fix for it here:
>
> https://cr.opensolaris.org/action/browse/pkg/edp/pkg.change-facet/webrev
> 7193711 change-facet doesn't restore content for explicitly false facets
>
> ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: 7193711

[Edward Pilatowicz]

On Tue, Aug 28, 2012 at 11:00:29AM -0700, Shawn Walker wrote:
> On 08/27/12 18:35, Edward Pilatowicz wrote:
> >hey all,
> >
> >shawn found a facet bug that in introduced with the original linked
> >images putback (oops).  i've got a fix for it here:
> >
> > https://cr.opensolaris.org/action/browse/pkg/edp/pkg.change-facet/webrev
> > 7193711 change-facet doesn't restore content for explicitly false facets
>
> src/tests/cli/t_change_facet.py:
>   line 68: Drop spaces around '[' and ']'
>

that line was actually changed to:

self.plist = self.pkgsend_bulk(self.rurl, self.pkg_A)
self.plist_B = self.pkgsend_bulk(self.rurl, self.pkg_B)

(this change was made because including pkg_b in self.plist broke other
test cases make the assumption that self.plist only contains pkg_A.)

>   line 291: s/an/a/
>

done.

> Thanks for expanding the test case.
>

yeah.  i spent some time studying the changes to make sure i wasn't
going to break anything else and expanded the test case to verify other
edge cases i was concerned about (it also led me to finding the dead
code in pkg_solver.py and cleaning that up).

> LGTM, but get one other reviewer if you can.
>

sure.

> Is this intended for U1?
>

i don't know.  i was going to putback to the default branch and then ask
folks (danek) if they want me to put this fix into U1 as well.  (i'm
comfortable enough with this fix to be willing to do that.)

thanks again for helping with this.
ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: 7193711

[Shawn Walker]

On 08/27/12 18:35, Edward Pilatowicz wrote:

hey all,

shawn found a facet bug that in introduced with the original linked
images putback (oops).  i've got a fix for it here:

 https://cr.opensolaris.org/action/browse/pkg/edp/pkg.change-facet/webrev
 7193711 change-facet doesn't restore content for explicitly false facets


src/tests/cli/t_change_facet.py:
  line 68: Drop spaces around '[' and ']'

  line 291: s/an/a/

Thanks for expanding the test case.

LGTM, but get one other reviewer if you can.

Is this intended for U1?

-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for: 7190495

[Danek Duvall]

Pete Dennis wrote:

> https://cr.opensolaris.org/action/browse/pkg/petede/urlchange

Looks okay to me.

Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: s12 changes

[Danek Duvall]

Tim Foster wrote:

> This looks good to me.  We've still got a lot of places in the code that
> refer to 5.11.   As I understand it, the majority of strings are only
> used in places where we're creating a pkg.fmri.PkgFmri or a
> pkg.version.Version and the build string isn't also declared in the fmri
> string.
> 
> Would it make sense to use a global variable there instead of '5.11', or
> are we happy to leave it as is, and expect that s12 release engineers
> will use a pkglint check to flag build_release values == [5, 11] for any
> packages that are being published?

I've whipped together (by which I mean it mostly works) another patch that
simply changes the Version constructor to default build_version to "5.12",
and removes most of the hard-coded "5.11" instances in the code to simply
allow the constructor to set the default.  I can't say I ever agreed with
the requirement that it be explicitly passed in.  This reduces the places
it's hardcoded to four.  But I'd rather do that as a separate change,
unless folks think it's something that should happen right now.  And I'm
still not sure whether it's worth carrying this thing around all the time,
though I don't think it hurts much.

> Also, a nit - should
> 
> >  +   nawk '$$1 ~ /^s12b[0-9]*/ {exit} END {print substr($$1,
> 
> actually be:
> 
> >  +   nawk '$$1 ~ /^s12b[0-9]+/ {exit} END {print substr($$1,
> 
> s12b(one or more characters in the range 0-9), not s12(zero or more..)

Cool, thanks.

Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: s12 changes

[Tim Foster]

On 08/18/12 11:27 AM, Danek Duvall wrote:

There are enough changes out for review right now that won't go into S11U1
that it's probably worthwhile branching to allow default to be s12, and for
people to push changes that would otherwise have to wait.  Along with the
following change will be a changeset that introduces the "s11-update"
branch:

 @  2772:f939336f34c9 (none): 7192439 update package versions for S12
 |
 | o  2771:784edc95356c s11-update: create branch for S11 updates
 |/
 o  2770:96cbf714e29b (none): Added tag s11u1b22a for changeset 3962a3f5d4c5




Small, so it's inline:

 7192439 update package versions for S12


This looks good to me.  We've still got a lot of places in the code that 
refer to 5.11.   As I understand it, the majority of strings are only 
used in places where we're creating a pkg.fmri.PkgFmri or a 
pkg.version.Version and the build string isn't also declared in the fmri 
string.


Would it make sense to use a global variable there instead of '5.11', or 
are we happy to leave it as is, and expect that s12 release engineers 
will use a pkglint check to flag build_release values == [5, 11] for any 
packages that are being published?


Also, a nit - should

>  +   nawk '$$1 ~ /^s12b[0-9]*/ {exit} END {print substr($$1,

actually be:

>  +   nawk '$$1 ~ /^s12b[0-9]+/ {exit} END {print substr($$1,

s12b(one or more characters in the range 0-9), not s12(zero or more..)

cheers,
tim



 diff --git a/src/pkg/Makefile b/src/pkg/Makefile
 --- a/src/pkg/Makefile
 +++ b/src/pkg/Makefile
 @@ -21,20 +21,19 @@
  # Copyright (c) 2010, 2012, Oracle and/or its affiliates. All rights 
reserved.
  #

 -PKGVERS_COMPONENT = 0.5.11
 +PKGVERS_COMPONENT = 5.12
  PKGVERS_BUILTON   = 5.11
 -BUILDNUM  = 175
 -UPDATENUM = 1
 +UPDATENUM = 0
  SRUNUM= 0
  PLATNUM   = 0
  BUILDID.cmd   = \
 hg tags | \
 -   nawk '$$1 ~ /^s11u$(UPDATENUM)b[0-9]*/ {print substr($$1, 7) + 0; 
exit}'
 +   nawk '$$1 ~ /^s12b[0-9]*/ {exit} END {print substr($$1, 5) + 0}'
  BUILDID   = $(BUILDID.cmd:sh)
  NIGHTLYID:sh  = hg parents --template '{rev}'
  CHANGESET:sh  = hg id -i
  PKGVERS_BRANCH= \
 -   
0.$(BUILDNUM).$(UPDATENUM).$(SRUNUM).$(PLATNUM).$(BUILDID).$(NIGHTLYID)
 +   
$(PKGVERS_COMPONENT).$(UPDATENUM).$(SRUNUM).$(PLATNUM).$(BUILDID).$(NIGHTLYID)
  PKGVERS   = 
$(PKGVERS_COMPONENT),$(PKGVERS_BUILTON)-$(PKGVERS_BRANCH)
  ARCH  = $(TARGET_ARCH:-%=%)
  REV:sh= date +%Y.%m.%d.%H.%M.%S

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss


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

Re: [pkg-discuss] Code review request for 7190458 & 7192186

[david . comay]

My only comment is that for the vp change, it'd be nice to have a comment
in the manifest that described (roughly) how the particular components were
chosen.  If it's complicated, then skip it.


No problem, I added a comment explaining that it matches
solaris-desktop and that the panel selection basically corresponds to
the included technologies on the Live Media ISO.

Thanks to you and Shawn for the review.
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for 7190458 & 7192186

[Danek Duvall]

david.co...@oracle.com wrote:

> I'd appreciate a review of the following webrev
> 
>   
> https://cr.opensolaris.org/action/browse/pkg/comay/7190458-n-7192186/webrev/

My only comment is that for the vp change, it'd be nice to have a comment
in the manifest that described (roughly) how the particular components were
chosen.  If it's complicated, then skip it.

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for 7187946, 7185502, and 7173792

[Edward Pilatowicz]

On Thu, Aug 16, 2012 at 06:42:49PM -0700, Brock Pytlik wrote:
> On 08/16/12 18:17, Edward Pilatowicz wrote:
> >hey shawn and brock,
> >
> >so while doing some final testing i discovered that my fix for 7187946
> >was incorrect.  to correctly fix the issue i had to make some additional
> >changes to:
> >
> > src/modules/client/bootenv.py
> > src/modules/client/api.py
> >
> >i've updated the webrev.  if you could take another look i'd appreciate
> >it.
> I don't think I understand that change in api.py. The one in
> bootenv.py looks fine.
>

without the api.py change, we always display the message when running
inside a zone.  (before this bug was introduced, we never dispalyed this
message inside a zone even if we were modifying the liveroot and an
alternate BE was active.)

thanks for looking at this,
ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for 7190458 & 7192186

[Shawn Walker]

On 08/16/12 18:47, david.co...@oracle.com wrote:

I'd appreciate a review of the following webrev

https://cr.opensolaris.org/action/browse/pkg/comay/7190458-n-7192186/webrev/


which covers the two following issues

7190458 visual panels in multi-user-desktop should be changed
7192186 pkg(5) manifests need some cleaning


LGTM,
-Shawn

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

Re: [pkg-discuss] Code review request for 7187946, 7185502, and 7173792

[Brock Pytlik]

On 08/16/12 18:17, Edward Pilatowicz wrote:

hey shawn and brock,

so while doing some final testing i discovered that my fix for 7187946
was incorrect.  to correctly fix the issue i had to make some additional
changes to:

src/modules/client/bootenv.py
src/modules/client/api.py

i've updated the webrev.  if you could take another look i'd appreciate
it.
I don't think I understand that change in api.py. The one in bootenv.py 
looks fine.


Brock


thanks,
ed

On Tue, Aug 07, 2012 at 07:48:28PM -0700, Edward Pilatowicz wrote:

webrev:
https://cr.opensolaris.org/action/browse/pkg/edp/pkg.refresh/webrev/

bugs:
7187946 pkg running in a zone always thinks it's modifying a non-active BE
7185502 need to document pkg change-{variant|facet} --no-refresh options
7173792 pkg doesn't auto-refresh previously used and disabled sysrepo publishers

thanks,
ed

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


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

Re: [pkg-discuss] code review request for 7190782 traceback when running pkg update in sv_SE locale

[Takeshi Asano]

Hi Shawn,

Could you please tell me whether the changeset is ok to go
or need revision, and if the former could you please
push the hg export for me?

Thanks,
Takeshi

On 2012年08月16日 10:22, Takeshi Asano wrote:

Hi Shawn,

Is it ok to proceed with the changeset?
If so, could you (or someone else) please push the changeset
using hg export pointed in the bug report?

Thanks,
Takeshi

On 2012年08月15日 09:22, Takeshi Asano wrote:

Hi Shawn,

On 2012年08月15日 00:36, Shawn Walker wrote:

On 08/14/12 00:50, Takeshi Asano wrote:

Hi,

Could you please code review for:

7190782 traceback when running pkg update in sv_SE locale

Webrev:
https://cr.opensolaris.org/action/browse/pkg/asano/7190782

Build done successfully and upgrade testing in the languages
(sv and ru) have been done.


I assume that we expect pkg.pot to be generated with 'fuzzy,' ?


Yes. GNU xgettext adds fuzzy line for the string expecting
it's eliminated during translation process.
Thus pkg.pot generated has the line.

Thanks,
Takeshi


Otherwise, seems fine.

-Shawn



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


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


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

Re: [pkg-discuss] Code review request for 7187946, 7185502, and 7173792

[Edward Pilatowicz]

hey shawn and brock,

so while doing some final testing i discovered that my fix for 7187946
was incorrect.  to correctly fix the issue i had to make some additional
changes to:

src/modules/client/bootenv.py
src/modules/client/api.py

i've updated the webrev.  if you could take another look i'd appreciate
it.

thanks,
ed

On Tue, Aug 07, 2012 at 07:48:28PM -0700, Edward Pilatowicz wrote:
> webrev:
> https://cr.opensolaris.org/action/browse/pkg/edp/pkg.refresh/webrev/
> 
> bugs:
> 7187946 pkg running in a zone always thinks it's modifying a non-active BE
> 7185502 need to document pkg change-{variant|facet} --no-refresh options
> 7173792 pkg doesn't auto-refresh previously used and disabled sysrepo 
> publishers
> 
> thanks,
> ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for 7187946, 7185502, and 7173792

[Brock Pytlik]

On 08/16/12 09:57, Edward Pilatowicz wrote:

On Wed, Aug 15, 2012 at 06:50:42PM -0700, Brock Pytlik wrote:

On 08/07/12 19:48, Edward Pilatowicz wrote:

webrev:
https://cr.opensolaris.org/action/browse/pkg/edp/pkg.refresh/webrev/

bugs:
7187946 pkg running in a zone always thinks it's modifying a non-active BE
7185502 need to document pkg change-{variant|facet} --no-refresh options
7173792 pkg doesn't auto-refresh previously used and disabled sysrepo publishers

thanks,
ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

image.py:
977: Why's it necessary to set the publisher's meta root here?
Alternatively, why do only these publishers need their meta root
set?

the reason we set last_refreshed to None is because we want the
last_refreshed set method remove the on-disk last_refreshed cache file.
but if meta_root is not set then that method doesn't know where the file
is so it won't actually delete it (it will only update the in-memory
state).


978: Personally, I think this line really belongs in
imageconfig.__merge_publishers. Couldn't we add it in as part of the
step of generating modified_pubs around line 1286 in imageconfig.py?


i tried to do this in a previous iteration and it didn't work out too
well.

the problem was that to do this we need to pass meta_root to the
BlendedConfig init routine (for the reasons mentioned above).  but
BlendedConfig objects can get allocated at two places, one of which
happens before we've actually determined the image version number.  but
the image version number is required to determine the meta_root path.  i
tried eliminating the other (earlier) place where we could allocate a
BlendedConfig object, but i couldn't get that working...
Ah, I missed that setting last_refreshed was more than just setting a 
variable (aren't properties wonderful ...). Given that these seem like 
the right changes for now.


Brock



ed


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

Re: [pkg-discuss] Code review request for 7187946, 7185502, and 7173792

[Edward Pilatowicz]

On Wed, Aug 15, 2012 at 06:50:42PM -0700, Brock Pytlik wrote:
> On 08/07/12 19:48, Edward Pilatowicz wrote:
> >webrev:
> >https://cr.opensolaris.org/action/browse/pkg/edp/pkg.refresh/webrev/
> >
> >bugs:
> >7187946 pkg running in a zone always thinks it's modifying a non-active BE
> >7185502 need to document pkg change-{variant|facet} --no-refresh options
> >7173792 pkg doesn't auto-refresh previously used and disabled sysrepo 
> >publishers
> >
> >thanks,
> >ed
> >___
> >pkg-discuss mailing list
> >pkg-discuss@opensolaris.org
> >http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
> image.py:
> 977: Why's it necessary to set the publisher's meta root here?
> Alternatively, why do only these publishers need their meta root
> set?

the reason we set last_refreshed to None is because we want the
last_refreshed set method remove the on-disk last_refreshed cache file.
but if meta_root is not set then that method doesn't know where the file
is so it won't actually delete it (it will only update the in-memory
state).

> 978: Personally, I think this line really belongs in
> imageconfig.__merge_publishers. Couldn't we add it in as part of the
> step of generating modified_pubs around line 1286 in imageconfig.py?
>

i tried to do this in a previous iteration and it didn't work out too
well.

the problem was that to do this we need to pass meta_root to the
BlendedConfig init routine (for the reasons mentioned above).  but
BlendedConfig objects can get allocated at two places, one of which
happens before we've actually determined the image version number.  but
the image version number is required to determine the meta_root path.  i
tried eliminating the other (earlier) place where we could allocate a
BlendedConfig object, but i couldn't get that working...

> t_pkg_sysrepo:
> 456: Why aren't we destroying the image here any more? After this
> loop is finished, we'll now have an extra random image laying around
> on disk (which may confuse developers using the -a option, though
> I'd need to see what kind of archive actually is produced).
>

fixed.

> You might consider adding tests where the user changes the value of
> the image-property "use-system-repo" from true to false, then back
> to true, and while it was set to false, origins were added or
> removed or packages were published to configured publishers.
>

i'll try doing this later today and see what i turn up.

ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] Code review request for 7187946, 7185502, and 7173792

[Brock Pytlik]

On 08/07/12 19:48, Edward Pilatowicz wrote:

webrev:
https://cr.opensolaris.org/action/browse/pkg/edp/pkg.refresh/webrev/

bugs:
7187946 pkg running in a zone always thinks it's modifying a non-active BE
7185502 need to document pkg change-{variant|facet} --no-refresh options
7173792 pkg doesn't auto-refresh previously used and disabled sysrepo publishers

thanks,
ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

image.py:
977: Why's it necessary to set the publisher's meta root here? 
Alternatively, why do only these publishers need their meta root set?
978: Personally, I think this line really belongs in 
imageconfig.__merge_publishers. Couldn't we add it in as part of the 
step of generating modified_pubs around line 1286 in imageconfig.py?


t_pkg_sysrepo:
456: Why aren't we destroying the image here any more? After this loop 
is finished, we'll now have an extra random image laying around on disk 
(which may confuse developers using the -a option, though I'd need to 
see what kind of archive actually is produced).


You might consider adding tests where the user changes the value of the 
image-property "use-system-repo" from true to false, then back to true, 
and while it was set to false, origins were added or removed or packages 
were published to configured publishers.


Thanks,
Brock
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for 7190782 traceback when running pkg update in sv_SE locale

[Takeshi Asano]

Hi Shawn,

Is it ok to proceed with the changeset?
If so, could you (or someone else) please push the changeset
using hg export pointed in the bug report?

Thanks,
Takeshi

On 2012年08月15日 09:22, Takeshi Asano wrote:

Hi Shawn,

On 2012年08月15日 00:36, Shawn Walker wrote:

On 08/14/12 00:50, Takeshi Asano wrote:

Hi,

Could you please code review for:

7190782 traceback when running pkg update in sv_SE locale

Webrev:
https://cr.opensolaris.org/action/browse/pkg/asano/7190782

Build done successfully and upgrade testing in the languages
(sv and ru) have been done.


I assume that we expect pkg.pot to be generated with 'fuzzy,' ?


Yes. GNU xgettext adds fuzzy line for the string expecting
it's eliminated during translation process.
Thus pkg.pot generated has the line.

Thanks,
Takeshi


Otherwise, seems fine.

-Shawn



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


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

Re: [pkg-discuss] code review request for 7190782 traceback when running pkg update in sv_SE locale

[Takeshi Asano]

Thanks, Saurabh,
Takeshi

On 2012年08月14日 17:35, Saurabh Vyas wrote:

On 08/14/12 13:20, Takeshi Asano wrote:

Hi,

Could you please code review for:

7190782 traceback when running pkg update in sv_SE locale

Webrev:
https://cr.opensolaris.org/action/browse/pkg/asano/7190782


These changes LGTM.

Saurabh



Build done successfully and upgrade testing in the languages
(sv and ru) have been done.

Thanks,
Takeshi
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss





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

Re: [pkg-discuss] code review request for 7190782 traceback when running pkg update in sv_SE locale

[Takeshi Asano]

Hi Shawn,

On 2012年08月15日 00:36, Shawn Walker wrote:

On 08/14/12 00:50, Takeshi Asano wrote:

Hi,

Could you please code review for:

7190782 traceback when running pkg update in sv_SE locale

Webrev:
https://cr.opensolaris.org/action/browse/pkg/asano/7190782

Build done successfully and upgrade testing in the languages
(sv and ru) have been done.


I assume that we expect pkg.pot to be generated with 'fuzzy,' ?


Yes. GNU xgettext adds fuzzy line for the string expecting
it's eliminated during translation process.
Thus pkg.pot generated has the line.

Thanks,
Takeshi


Otherwise, seems fine.

-Shawn



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

Re: [pkg-discuss] Code review request for 7187946, 7185502, and 7173792

[Shawn Walker]

On 08/07/12 19:48, Edward Pilatowicz wrote:

webrev:
https://cr.opensolaris.org/action/browse/pkg/edp/pkg.refresh/webrev/

bugs:
7187946 pkg running in a zone always thinks it's modifying a non-active BE
7185502 need to document pkg change-{variant|facet} --no-refresh options
7173792 pkg doesn't auto-refresh previously used and disabled sysrepo publishers


src/modules/client/image.py
  line 996: s/lost/lose/

  line 996: s/suspub/syspub/

Otherwise, LGTM.

-Shawn

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

Re: [pkg-discuss] code review request for 7190782 traceback when running pkg update in sv_SE locale

[Shawn Walker]

On 08/14/12 00:50, Takeshi Asano wrote:

Hi,

Could you please code review for:

7190782 traceback when running pkg update in sv_SE locale

Webrev:
https://cr.opensolaris.org/action/browse/pkg/asano/7190782

Build done successfully and upgrade testing in the languages
(sv and ru) have been done.


I assume that we expect pkg.pot to be generated with 'fuzzy,' ?

Otherwise, seems fine.

-Shawn

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

Re: [pkg-discuss] code review request for 7190782 traceback when running pkg update in sv_SE locale

[Saurabh Vyas]

On 08/14/12 13:20, Takeshi Asano wrote:

Hi,

Could you please code review for:

7190782 traceback when running pkg update in sv_SE locale

Webrev:
https://cr.opensolaris.org/action/browse/pkg/asano/7190782


These changes LGTM.

Saurabh



Build done successfully and upgrade testing in the languages
(sv and ru) have been done.

Thanks,
Takeshi
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss



--


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

ORACLE India | Off Langford Road | Bangalore | 560025

|Bangalore |
Green Oracle  Oracle is committed to
developing practices and products that help protect the environment
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7140762

[Abhinandan Ekande]

On 08/10/12 00:21, Danek Duvall wrote:

t_pkgrepo.py:

   - line 1372: We only test both because the OS happens to test for the
 EINVAL condition before the EEXIST condition.  It could go the other
 way around, and then we'd only be testing the EEXIST condition.

If multiple versions exists in repository for a package and if "pkgrepo
remove" is executed from package directory, then first error always will
be EINVAL.

Is that dictated by anything other than the current implementation of the
VFS or filesystem code?  I don't know why you can rely on that.


As per filesystem code the EINVAL will occur before EEXIST.
Also what I meant by comment at line 1372 was that the removal
of truck@1.0 will verify both error code paths in rmdir() definition
in repository.py.

In addition I have made changes to check for truck@2 after removing
truck@1.0, for example :
+# Verify truck@2 still exists in repo
+self.pkgrepo("list -s %s truck@2.0,5.11-0" % src_repo)

Thanks,
Abhi.
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7140762

[Danek Duvall]

Abhinandan Ekande wrote:

> >t_pkgrepo.py:
> >
> >   - line 1372: We only test both because the OS happens to test for the
> > EINVAL condition before the EEXIST condition.  It could go the other
> > way around, and then we'd only be testing the EEXIST condition.
> 
> If multiple versions exists in repository for a package and if "pkgrepo
> remove" is executed from package directory, then first error always will
> be EINVAL.

Is that dictated by anything other than the current implementation of the
VFS or filesystem code?  I don't know why you can rely on that.

Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7140762

[Abhinandan Ekande]

Thanks Danek for review.

On 07/27/12 02:06 PM, Danek Duvall wrote:

Abhinandan Ekande wrote:


https://cr.opensolaris.org/action/browse/pkg/ae112802/7140762-rev3/webrev/

repository.py:

   - line 1635: I would keep the "elif e.errno not in ( ... )" construction,
 which should have the same effect, but be slightly more compact.


Ok; I will restore earlier "elif e.errno not in" construct.



t_pkgrepo.py:

   - line 1364, 1377: use "publish" instead of "pull"

Will change.



   - line 1372: We only test both because the OS happens to test for the
 EINVAL condition before the EEXIST condition.  It could go the other
 way around, and then we'd only be testing the EEXIST condition.


If multiple versions exists in repository for a package and if "pkgrepo 
remove" is
executed from package directory, then first error always will be EINVAL. 
Then next
rmdir will generate EEXIST error. If user removes package from another 
directory,
then error will be EEXIST only when multiple versions of package are 
present in
repository. So error sequence will be either EINVAL followed by EEXIST, 
or only

EEXIST. IMO, in this scenario EEXIST cannot occur before EINVAL.


   Is
 there value in this sort of combo test?  It might make sense to test to
 make sure that truck@2 is still there, but that's not going on here.


I will check for truck@2 after removing truck@1.

Abhi.
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7140762

[Danek Duvall]

Abhinandan Ekande wrote:

> https://cr.opensolaris.org/action/browse/pkg/ae112802/7140762-rev3/webrev/

repository.py:

  - line 1635: I would keep the "elif e.errno not in ( ... )" construction,
which should have the same effect, but be slightly more compact.

t_pkgrepo.py:

  - line 1364, 1377: use "publish" instead of "pull"

  - line 1372: We only test both because the OS happens to test for the
EINVAL condition before the EEXIST condition.  It could go the other
way around, and then we'd only be testing the EEXIST condition.  Is
there value in this sort of combo test?  It might make sense to test to
make sure that truck@2 is still there, but that's not going on here.

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: 3 small and simple fixes

[Danek Duvall]

Edward Pilatowicz wrote:

> https://cr.opensolaris.org/action/browse/pkg/edp/pkg.msg/webrev

LGTM.

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: 3 small and simple fixes

[Edward Pilatowicz]

On Fri, Jul 20, 2012 at 01:12:42PM -0700, Erik Trauschke wrote:
>
>
> On 07/20/12 11:56 AM, Edward Pilatowicz wrote:
> >hey all,
> >
> >i've got some small and simple bugfixes done:
> >
> >https://cr.opensolaris.org/action/browse/pkg/edp/pkg.msg/webrev
> >7182704 None is not an operation
> >7185026 LinkedImageException doesn't handle empty strings correctly
> >7185447 pkg remote subcommand needs a private help entry
> >
> >these are not stoppers (more just nits) so if folks want me to hold off
> >on them just say so.
>
> api_errors.py:
>
> line 2780: I think here you can leave it like it is. It doesn't look
> like there is a string getting passed so there will not be a problem
> with empty strings. And if I see this correctly there is a syntactic
> difference between 'True' and 'not None', but there is no difference
> between 'True' and 'not False'.
>

update.

> Otherwise, lgtm
>

thanks.

ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request: 3 small and simple fixes

[Erik Trauschke]

On 07/20/12 11:56 AM, Edward Pilatowicz wrote:

hey all,

i've got some small and simple bugfixes done:

https://cr.opensolaris.org/action/browse/pkg/edp/pkg.msg/webrev
7182704 None is not an operation
7185026 LinkedImageException doesn't handle empty strings correctly
7185447 pkg remote subcommand needs a private help entry

these are not stoppers (more just nits) so if folks want me to hold off
on them just say so.


api_errors.py:

line 2780: I think here you can leave it like it is. It doesn't look 
like there is a string getting passed so there will not be a problem 
with empty strings. And if I see this correctly there is a syntactic 
difference between 'True' and 'not None', but there is no difference 
between 'True' and 'not False'.


Otherwise, lgtm

Erik
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for localization update 7182484 and bugfix 7183575

[Takeshi Asano]

Hi Tim,

On 2012年07月19日 08:41, Tim Foster wrote:

On 07/19/12 11:28 AM, Takeshi Asano wrote:

Regarding how to detect the issue in source codes,
/usr/gnu/bin/xgettext itself can report the issue as like:

../client.py:1370: warning: 'msgid' format string with unnamed arguments cannot 
be properly localized:
The translator cannot reorder the arguments.
Please consider using a format string with named arguments,
and a mapping instead of a tuple for the arguments.



That sounds like an excellent check to have at build time, causing the build to 
fail if that error is produced. Shall I file another bug to request that change?


I was thinking to file it if you agree to proceed with
the option but if you can file it that's fine, too.

Thanks,
Takeshi

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

Re: [pkg-discuss] code review request for localization update 7182484 and bugfix 7183575

[Tim Foster]

On 07/19/12 11:28 AM, Takeshi Asano wrote:

Regarding how to detect the issue in source codes,
/usr/gnu/bin/xgettext itself can report the issue as like:

../client.py:1370: warning: 'msgid' format string with unnamed arguments cannot 
be properly localized:
  The translator cannot reorder the arguments.
  Please consider using a format string with named 
arguments,
  and a mapping instead of a tuple for the 
arguments.



That sounds like an excellent check to have at build time, causing the 
build to fail if that error is produced.  Shall I file another bug to 
request that change?


cheers,
tim
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for localization update 7182484 and bugfix 7183575

[Takeshi Asano]

Hi Danek and Tim,

Yes, the multiple unnamed parameters is issue left and
software codes should be modified to eliminate the issue.
(Though updating software for the fix at this moment will
 not likely to get translation syncup within this release.)

Currently we have been doing these:

- Translation group instructs to translators
  that reorder of unnamed parameters in Python format
  cannot be done at translator side and need software fix.
- After received translations from the group, checking that *not* to have:
  * attempt to printf-style explicit reordering 1$, 2$, ...
  * reorder of parameters in different types

but those won't fully eliminate possibility of having reorder
between parameters in same type.
Essential solution by eliminating the issue from software
is appropriate.

Regarding how to detect the issue in source codes,
/usr/gnu/bin/xgettext itself can report the issue as like:

../client.py:1370: warning: 'msgid' format string with unnamed arguments cannot 
be properly localized:
The translator cannot reorder the arguments.
Please consider using a format string with named 
arguments,
and a mapping instead of a tuple for the arguments.

if the xgettext is called directly rather than through
intltool-update --pot. So either seeking way to have
intltool-update forward the warnings or migrating to
direct use of xgettext may be other options.

Thanks,
Takeshi


On 2012年07月19日 06:35, Tim Foster wrote:

On 07/19/12 06:21 AM, Danek Duvall wrote:

Takeshi Asano wrote:

https://cr.opensolaris.org/action/browse/pkg/asano/7182484/


Looks okay to me. Though I've noticed in some of the changes that we still
have strings with multiple %-expandos where they're not named; things like

"invalid '%s' value: %s"

or

"'%s' must be>= %d"

Is there a simple way to find all of these, so that we can file bugs?


Yep, here goes, based on the ja.po file that's in the gate at present, using 
http://pypi.python.org/pypi/polib/ (perhaps there's better libraries out there, 
this was just the first I ran into)

 >>> import polib
 >>> entries = polib.pofile("./ja.po")
 >>> for entry in entries:
... msgid = entry.msgid
... count = msgid.count("%s")
... if count > 1:
... print msgid
...
...
...
%s failed: %s
%s failed (linked image exception(s)):
%s
An unexpected error happened during %s: %s
%s failed (inventory exception):
%s
%s: duplicate variant specified: %s
%s (group dependency of '%s')
pkg: %s/%s catalogs successfully updated:

%s%s
%s dependency on an obsolete package (%s);this package must be uninstalled 
manually
Path outside alternate root: root=%s, path=%s
'%s' supports the following architectures: %s
Error encountered while retrieving data from '%s':
%s
%s failed to be updated. No changes have been made to %s.
The Boot Environment %s failed to be updated. A snapshot was taken before the 
failed attempt and has been restored so no changes have been made
to %s.
%s is expected to be a certificate but could not be parsed. The error 
encountered was:
%s
%s %s [already rejected; see above]
Couldn't find '%s' in any of the specified search directories:
%s
%s had this elf error:%s
%s to %s

Could not change the BE name to:
%s

The following name will be used instead:
%s
'%s' and '%s' have the same meaning
A '%s' action cannot be present in an obsolete package: %s
A '%s' action cannot be present in a renamed package: %s
Out dir %s does not exist and could not be created. Error is: %s
File %s line %s: %s
illegal %s option -- %s
Unable to create basedir '%s': %s
Manifests support different variants %s %s
Invalid file %s: manifest not encoded in UTF-8: %s
Unable to read manifest file %s: %s
transform (%s) has regexp error (%s) in matching clause
transform (%s) has 'edit' operation with malformedregexp (%s)
 >>>


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

Re: [pkg-discuss] code review request for localization update 7182484 and bugfix 7183575

[Tim Foster]

On 07/19/12 06:21 AM, Danek Duvall wrote:

Takeshi Asano wrote:

https://cr.opensolaris.org/action/browse/pkg/asano/7182484/


Looks okay to me.  Though I've noticed in some of the changes that we still
have strings with multiple %-expandos where they're not named; things like

 "invalid '%s' value: %s"

or

 "'%s' must be>= %d"

Is there a simple way to find all of these, so that we can file bugs?


Yep, here goes, based on the ja.po file that's in the gate at present, 
using http://pypi.python.org/pypi/polib/ (perhaps there's better 
libraries out there, this was just the first I ran into)


>>> import polib
>>> entries = polib.pofile("./ja.po")
>>> for entry in entries:
... msgid = entry.msgid
... count = msgid.count("%s")
... if count > 1:
... print msgid
...
...
...
%s failed: %s
%s failed (linked image exception(s)):
%s
An unexpected error happened during %s: %s
%s failed (inventory exception):
%s
%s: duplicate variant specified: %s
%s (group dependency of '%s')
pkg: %s/%s catalogs successfully updated:

%s%s
%s dependency on an obsolete package (%s);this package must be 
uninstalled manually

Path outside alternate root: root=%s, path=%s
'%s' supports the following architectures: %s
Error encountered while retrieving data from '%s':
%s
%s failed to be updated. No changes have been made to %s.
The Boot Environment %s failed to be updated. A snapshot was taken 
before the failed attempt and has been restored so no changes have been 
made

to %s.
%s is expected to be a certificate but could not be parsed.  The error 
encountered was:

%s
%s  %s  [already rejected; see above]
Couldn't find '%s' in any of the specified search directories:
%s
%s had this elf error:%s
%s to %s

Could not change the BE name to:
%s

The following name will be used instead:
%s
'%s' and '%s' have the same meaning
A '%s' action cannot be present in an obsolete package: %s
A '%s' action cannot be present in a renamed package: %s
Out dir %s does not exist and could not be created. Error is: %s
File %s line %s: %s
illegal %s option -- %s
Unable to create basedir '%s': %s
Manifests support different variants %s %s
Invalid file %s: manifest not encoded in UTF-8: %s
Unable to read manifest file %s: %s
transform (%s) has regexp error (%s) in matching clause
transform (%s) has 'edit' operation with malformedregexp (%s)
>>>
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for localization update 7182484 and bugfix 7183575

[Danek Duvall]

Takeshi Asano wrote:

> https://cr.opensolaris.org/action/browse/pkg/asano/7182484/

Looks okay to me.  Though I've noticed in some of the changes that we still
have strings with multiple %-expandos where they're not named; things like

"invalid '%s' value: %s"

or

"'%s' must be >= %d"

Is there a simple way to find all of these, so that we can file bugs?

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for 7140215

[Edward Pilatowicz]

On Thu, Jul 12, 2012 at 01:56:16PM -0700, Brock Pytlik wrote:
> On 07/11/12 19:10, Edward Pilatowicz wrote:
> >hey all,
> >
> >i'm looking for a review for:
> >
> >https://cr.opensolaris.org/action/browse/pkg/edp/pkg.uninstall/webrev
> >7140215 pkg uninstall should probably ignore parent dependencies
> >
> >i updated the bug description today with a summary of my reasoning
> >behind this fix.
> >
> >thanks,
> >ed
> >___
> >pkg-discuss mailing list
> >pkg-discuss@opensolaris.org
> >http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
> imageplan.py:
> 360: couldn't this be written more simply as:
>
> ignore_inst_parent_deps = not li_sync_op
>

done.

> I'm having trouble understanding how the changes in lines 370-390
> and 585-595 are related to this bug. Can you elaborate on why it's
> better to uncommonize this code?
>

i initially thought it was better because it removed parameters from a
public interface, then i realized the __solve_install() was a private
interface, so i backed the changes out.

>
> t_linked_image:
> nit:  1340 and 1348: number mismatch in the comments between "a
> out-of-sync" and "packages"
>

fixed.

> General question:
> Suppose the image is out of sync but upgrading (or downgrading) a
> package brings it more into sync. Is that allowed? I didn't see a
> test for that situation.
>

it's all allowed and i added tests for it.

i've updated imageplan.py and test_unsynced_image_operations():

https://cr.opensolaris.org/action/browse/pkg/edp/pkg.uninstall2/webrev/

thanks for looking at this.

ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for 7140215

[Brock Pytlik]

On 07/11/12 19:10, Edward Pilatowicz wrote:

hey all,

i'm looking for a review for:

https://cr.opensolaris.org/action/browse/pkg/edp/pkg.uninstall/webrev
7140215 pkg uninstall should probably ignore parent dependencies

i updated the bug description today with a summary of my reasoning
behind this fix.

thanks,
ed
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

imageplan.py:
360: couldn't this be written more simply as:

ignore_inst_parent_deps = not li_sync_op

I'm having trouble understanding how the changes in lines 370-390 and 
585-595 are related to this bug. Can you elaborate on why it's better to 
uncommonize this code?



t_linked_image:
nit:  1340 and 1348: number mismatch in the comments between "a 
out-of-sync" and "packages"


General question:
Suppose the image is out of sync but upgrading (or downgrading) a 
package brings it more into sync. Is that allowed? I didn't see a test 
for that situation.


Otherwise it seems good to me.

Brock
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Abhinandan Ekande]

Thanks Shawn for pointing the failure during "make packages".
Modified the file src/pkg/external_deps.txt. Verified that "make packages"
succeeds. Here is updated webrev :

https://cr.opensolaris.org/action/browse/pkg/ae112802/7166082-rev5/webrev/


Patch has been attached.

Abhi.

On 07/09/12 23:50, Shawn Walker wrote:

I think you forgot to run 'make packages':

$ make packages
...
Running pkglint

New pkglint errors detected:
> WARNING pkglint.action005.1   obsolete dependency check skipped: 
unable to find dependency pkg:/system/locale/extra for 
pkg://pkg5-nightly/developer/opensolaris/pkg5

*** Error code 1
...

Which means you also forgot to add 'system/locale/extra' to 
src/pkg/external_deps.txt.
# HG changeset patch
# User saurabh.v...@oracle.com
# Date 1341909804 -19800
# Node ID aff39f25076daae792722a7a7333a82453184221
# Parent  41dcac9de303c46577b5d62c42db26777842c253
7166082 pkg command does not handle Japanese character encoding on Solaris 11

diff -r 41dcac9de303 -r aff39f25076d src/client.py
--- a/src/client.py Mon Jul 09 15:47:03 2012 -0700
+++ b/src/client.py Tue Jul 10 14:13:24 2012 +0530
@@ -6607,7 +6607,8 @@
 
 if __name__ == "__main__":
 misc.setlocale(locale.LC_ALL, "", error)
-gettext.install("pkg", "/usr/share/locale")
+gettext.install("pkg", "/usr/share/locale",
+codeset=locale.getpreferredencoding())
 
 # Make all warnings be errors.
 import warnings
diff -r 41dcac9de303 -r aff39f25076d src/depot.py
--- a/src/depot.py  Mon Jul 09 15:47:03 2012 -0700
+++ b/src/depot.py  Tue Jul 10 14:13:24 2012 +0530
@@ -19,7 +19,7 @@
 #
 # CDDL HEADER END
 #
-# Copyright (c) 2007, 2011 Oracle and/or its affiliates.  All rights reserved.
+# Copyright (c) 2007, 2012, Oracle and/or its affiliates.  All rights reserved.
 #
 
 # pkg.depotd - package repository daemon
@@ -228,7 +228,8 @@
 if __name__ == "__main__":
 
 setlocale(locale.LC_ALL, "")
-gettext.install("pkg", "/usr/share/locale")
+gettext.install("pkg", "/usr/share/locale",
+codeset=locale.getpreferredencoding())
 
 add_content = False
 exit_ready = False
diff -r 41dcac9de303 -r aff39f25076d src/modules/client/history.py
--- a/src/modules/client/history.py Mon Jul 09 15:47:03 2012 -0700
+++ b/src/modules/client/history.py Tue Jul 10 14:13:24 2012 +0530
@@ -26,8 +26,6 @@
 
 import copy
 import errno
-import gettext
-import locale
 import os
 import shutil
 import sys
@@ -104,31 +102,6 @@
 MemoryError: RESULT_FAILED_OUTOFMEMORY,
 }
 
-misc.setlocale(locale.LC_ALL, "")
-gettext.install("pkg", "/usr/share/locale")
-
-# since we store english text in our XML files, we need a way for clients
-# obtain a translated version of these messages.
-result_l10n = {
-"Canceled": _("Canceled"),
-"Failed": _("Failed"),
-"Ignored": _("Ignored"),
-"Nothing to do": _("Nothing to do"),
-"Succeeded": _("Succeeded"),
-"Bad Request": _("Bad Request"),
-"Configuration": _("Configuration"),
-"Constrained": _("Constrained"),
-"Locked": _("Locked"),
-"Search": _("Search"),
-"Storage": _("Storage"),
-"Transport": _("Transport"),
-"Actuator": _("Actuator"),
-"Out of Memory": _("Out of Memory"),
-"Conflicting Actions": _("Conflicting Actions"),
-"Unknown": _("Unknown"),
-"None": _("None")
-}
-
 class _HistoryOperation(object):
 """A _HistoryOperation object is a representation of data about an
 operation that a pkg(5) client has performed.  This class is private
@@ -139,6 +112,8 @@
 manipulated as they are set or retrieved.
 """
 
+result_l10n = {}# Static variable for dictionary
+
 def __copy__(self):
 h = _HistoryOperation()
 for attr in ("name", "start_time", "end_time", "start_state",
@@ -235,10 +210,35 @@
 def result_text(self):
 """Returns a tuple containing the translated text for the
 operation result of the form (outcome, reason)."""
+
+if not _HistoryOperation.result_l10n:
+# since we store english text in our XML files, we
+# need a way for clients obtain a translated version
+# of these messages.
+_HistoryOperation.result_l10n = {
+"Canceled": _("Canceled"),
+"Failed": _("Failed"),
+"Ignored": _("Ignored"),
+"Nothing to do": _("Nothing to do"),
+"Succeeded": _("Succeeded"),
+"Bad Request": _("Bad Request"),
+"Configuration": _("Configuration"),
+"Constrained": _("Constrained"),
+"Locked": _("Locked"),
+"Search": _("Search"),
+   

Re: [pkg-discuss] code review request for CR 7166082

[Shawn Walker]

On 07/08/12 23:36, Abhinandan Ekande wrote:

Hi Shawn,

As per your suggestion moved the result_l10n dictionary to result_text()
method
in history.py file. The dictionary is initialized only once. Thanks for
offline review of
this change.

Here is updated webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7166082-rev4/webrev/

I have also attached the patch. Could you please push it.


I think you forgot to run 'make packages':

$ make packages
...
Running pkglint

New pkglint errors detected:
> WARNING pkglint.action005.1   obsolete dependency check skipped: 
unable to find dependency pkg:/system/locale/extra for 
pkg://pkg5-nightly/developer/opensolaris/pkg5

*** Error code 1
...

Which means you also forgot to add 'system/locale/extra' to 
src/pkg/external_deps.txt.


-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Abhinandan Ekande]

Hi Shawn,

As per your suggestion moved the result_l10n dictionary to result_text() 
method
in history.py file. The dictionary is initialized only once. Thanks for 
offline review of

this change.

Here is updated webrev : 
https://cr.opensolaris.org/action/browse/pkg/ae112802/7166082-rev4/webrev/


I have also attached the patch. Could you please push it.

Thanks,
Abhi.

On 07/02/12 18:07, Abhinandan Ekande wrote:

Hi Danek,

Agree that only one module is getting fixed with this approach.
I will implement either Shawn's or your suggestion and post the
webrev.

Thanks,
Abhi.

On 06/30/12 17:18, Takeshi Asano wrote:

Hi Danek,

I see your point. Thank you.

Yes, once the gettext.install() call is removed history.py will be
in same state as the other modules. Use of gettext.translation()
can be addressed separately for all modules.

Thanks,
Takeshi

On 2012年06月30日 09:52, Danek Duvall wrote:

Takeshi Asano wrote:


The new code doesn't rely on application's gettext.install() call.
The client/__init__.py does the per-module definition of "_"
and client/history.py imports that.


I realize that the latest proposal does the right thing, but the 
rest of
the modules don't, and the proposal is to fix just this module.  I 
don't
like the partial solution.  Either fix it all, or don't fix this 
particular
problem, and get around the fact that history.py i18ns some strings 
at the

module level in a different way.

Danek


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

___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
# HG changeset patch
# User abhinandan.eka...@oracle.com
# Date 1341808384 -19800
# Node ID 6de5cf08614c72c0142fe574123921dfb93d4234
# Parent  9cbeedd42e78b4bbb219a266683ee4137c3d8d02
7166082 pkg command does not handle Japanese character encoding on Solaris 11

diff -r 9cbeedd42e78 -r 6de5cf08614c src/client.py
--- a/src/client.py Wed Jul 04 21:11:40 2012 +1200
+++ b/src/client.py Mon Jul 09 10:03:04 2012 +0530
@@ -6602,7 +6602,8 @@
 
 if __name__ == "__main__":
 misc.setlocale(locale.LC_ALL, "", error)
-gettext.install("pkg", "/usr/share/locale")
+gettext.install("pkg", "/usr/share/locale",
+codeset=locale.getpreferredencoding())
 
 # Make all warnings be errors.
 import warnings
diff -r 9cbeedd42e78 -r 6de5cf08614c src/depot.py
--- a/src/depot.py  Wed Jul 04 21:11:40 2012 +1200
+++ b/src/depot.py  Mon Jul 09 10:03:04 2012 +0530
@@ -19,7 +19,7 @@
 #
 # CDDL HEADER END
 #
-# Copyright (c) 2007, 2011 Oracle and/or its affiliates.  All rights reserved.
+# Copyright (c) 2007, 2012, Oracle and/or its affiliates.  All rights reserved.
 #
 
 # pkg.depotd - package repository daemon
@@ -228,7 +228,8 @@
 if __name__ == "__main__":
 
 setlocale(locale.LC_ALL, "")
-gettext.install("pkg", "/usr/share/locale")
+gettext.install("pkg", "/usr/share/locale",
+codeset=locale.getpreferredencoding())
 
 add_content = False
 exit_ready = False
diff -r 9cbeedd42e78 -r 6de5cf08614c src/modules/client/history.py
--- a/src/modules/client/history.py Wed Jul 04 21:11:40 2012 +1200
+++ b/src/modules/client/history.py Mon Jul 09 10:03:04 2012 +0530
@@ -26,8 +26,6 @@
 
 import copy
 import errno
-import gettext
-import locale
 import os
 import shutil
 import sys
@@ -104,31 +102,6 @@
 MemoryError: RESULT_FAILED_OUTOFMEMORY,
 }
 
-misc.setlocale(locale.LC_ALL, "")
-gettext.install("pkg", "/usr/share/locale")
-
-# since we store english text in our XML files, we need a way for clients
-# obtain a translated version of these messages.
-result_l10n = {
-"Canceled": _("Canceled"),
-"Failed": _("Failed"),
-"Ignored": _("Ignored"),
-"Nothing to do": _("Nothing to do"),
-"Succeeded": _("Succeeded"),
-"Bad Request": _("Bad Request"),
-"Configuration": _("Configuration"),
-"Constrained": _("Constrained"),
-"Locked": _("Locked"),
-"Search": _("Search"),
-"Storage": _("Storage"),
-"Transport": _("Transport"),
-"Actuator": _("Actuator"),
-"Out of Memory": _("Out of Memory"),
-"Conflicting Actions": _("Conflicting Actions"),
-"Unknown": _("Unknown"),
-"None": _("None")
-}
-
 class _HistoryOperation(object):
 """A _HistoryOperation object is a representation of data about an
 operation that a pkg(5) client has performed.  This class is private
@@ -139,6 +112,8 @@
 manipulated as they are set or retrieved.
 """
 
+result_l10n = {}# Static variable for dictionary
+
 def __copy__(self):
 h = _HistoryOperation()
 for attr in ("name", "start_time", "end_time", "start_state",
@@ -235,10 +210,35 @@
 def result_text(self):
 """Returns

Re: [pkg-discuss] code review request for CR 7166082

[Abhinandan Ekande]

Hi Danek,

Agree that only one module is getting fixed with this approach.
I will implement either Shawn's or your suggestion and post the
webrev.

Thanks,
Abhi.

On 06/30/12 17:18, Takeshi Asano wrote:

Hi Danek,

I see your point. Thank you.

Yes, once the gettext.install() call is removed history.py will be
in same state as the other modules. Use of gettext.translation()
can be addressed separately for all modules.

Thanks,
Takeshi

On 2012年06月30日 09:52, Danek Duvall wrote:

Takeshi Asano wrote:


The new code doesn't rely on application's gettext.install() call.
The client/__init__.py does the per-module definition of "_"
and client/history.py imports that.


I realize that the latest proposal does the right thing, but the rest of
the modules don't, and the proposal is to fix just this module.  I don't
like the partial solution.  Either fix it all, or don't fix this 
particular
problem, and get around the fact that history.py i18ns some strings 
at the

module level in a different way.

Danek


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

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

Re: [pkg-discuss] code review request for CR 7140762

[Abhinandan Ekande]

On 06/28/12 23:01, Bart Smaalders wrote:


What happens if another file exists in that directory?



Thanks Bart for review. Yes, if multiple versions of package exist in 
the repository
then previous fix would traceback. I have updated the webrev with your 
suggestion :

https://cr.opensolaris.org/action/browse/pkg/ae112802/7140762-rev3/webrev/


Would like to point that after the "pkgrepo remove" operation user would 
land in a directory

which has been deleted.

Thanks,
Abhi.



Perhaps:

   def rmdir(d):

 """rmdir; but ignores non-empty directories."""
 try:
 os.rmdir(d)
 except OSError, e:
 if e.errno == errno.EINVAL and \
 os.getcwd != self.root:
 # In case of EINVAL error we could be
 # in directory we're removing;
 # chdir to repository root and
 # recurse
 os.chdir(self.root)
 rmdir(d)
 elif e.errno in (
 errno.ENOTEMPTY,
 errno.EEXIST):
 pass
 else:
 raise

- Bart 

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

Re: [pkg-discuss] code review request for CR 7166082

[Takeshi Asano]

Hi Danek,

I see your point. Thank you.

Yes, once the gettext.install() call is removed history.py will be
in same state as the other modules. Use of gettext.translation()
can be addressed separately for all modules.

Thanks,
Takeshi

On 2012年06月30日 09:52, Danek Duvall wrote:

Takeshi Asano wrote:


The new code doesn't rely on application's gettext.install() call.
The client/__init__.py does the per-module definition of "_"
and client/history.py imports that.


I realize that the latest proposal does the right thing, but the rest of
the modules don't, and the proposal is to fix just this module.  I don't
like the partial solution.  Either fix it all, or don't fix this particular
problem, and get around the fact that history.py i18ns some strings at the
module level in a different way.

Danek


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

Re: [pkg-discuss] code review request for CR 7166082

[Danek Duvall]

Takeshi Asano wrote:

> The new code doesn't rely on application's gettext.install() call.
> The client/__init__.py does the per-module definition of "_"
> and client/history.py imports that.

I realize that the latest proposal does the right thing, but the rest of
the modules don't, and the proposal is to fix just this module.  I don't
like the partial solution.  Either fix it all, or don't fix this particular
problem, and get around the fact that history.py i18ns some strings at the
module level in a different way.

Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Takeshi Asano]

Hi Danek,

The new code doesn't rely on application's gettext.install() call.
The client/__init__.py does the per-module definition of "_"
and client/history.py imports that.

It uses common way for modules:
http://docs.python.org/release/2.6.8/library/gettext.html#localizing-your-module
which doesn't care application's message domain (or whether application
does gettext.install() or not).

Thanks,
Takeshi

On 2012年06月30日 02:48, Danek Duvall wrote:

Abhinandan Ekande wrote:


Hi Shawn, Danek, Takeshi :

I have tried the suggestion by both Shawn and Takeshi. Both works fine. I
have
made the changes as suggested by Takeshi. Here is latest updated webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7166082-rev3/webrev/

Below two files have updated in this webrev. Rest of the files are not
changed.

src/modules/client/history.py
src/modules/client/__init__.py

Ran the "pkg history" command with C locale, unicode locale and eucJP
locale. Verified the output. Ran the complete test suite and all test
cases passed.


I think while this is a direction we might want to consider in the future,
I don't want to do it now, because I think it needs to be done more
globally.

I just closed a bug which complained that if you tried to use the progress
tracker class you got a NameError because it used _(), and
gettext.install() hadn't been run.  Moreover, it seems clear to me that if
the modules rely on the program to call gettext.install(), they won't get
the right translations if the program wasn't one of ours, and we really do
need to have a per-module definition of _().

But I think all that will take a little more work and thought, testing, and
performance checking.

I'd rather see Shawn's or my proposal implemented instead.

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss


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

Re: [pkg-discuss] code review request for CR 7166082

[Danek Duvall]

Abhinandan Ekande wrote:

> Hi Shawn, Danek, Takeshi :
> 
> I have tried the suggestion by both Shawn and Takeshi. Both works fine. I
> have
> made the changes as suggested by Takeshi. Here is latest updated webrev :
> https://cr.opensolaris.org/action/browse/pkg/ae112802/7166082-rev3/webrev/
> 
> Below two files have updated in this webrev. Rest of the files are not
> changed.
> 
>src/modules/client/history.py
>src/modules/client/__init__.py
> 
> Ran the "pkg history" command with C locale, unicode locale and eucJP
> locale. Verified the output. Ran the complete test suite and all test
> cases passed.

I think while this is a direction we might want to consider in the future,
I don't want to do it now, because I think it needs to be done more
globally.

I just closed a bug which complained that if you tried to use the progress
tracker class you got a NameError because it used _(), and
gettext.install() hadn't been run.  Moreover, it seems clear to me that if
the modules rely on the program to call gettext.install(), they won't get
the right translations if the program wasn't one of ours, and we really do
need to have a per-module definition of _().

But I think all that will take a little more work and thought, testing, and
performance checking.

I'd rather see Shawn's or my proposal implemented instead.

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Abhinandan Ekande]

Hi Shawn, Danek, Takeshi :

I have tried the suggestion by both Shawn and Takeshi. Both works fine. 
I have

made the changes as suggested by Takeshi. Here is latest updated webrev :

https://cr.opensolaris.org/action/browse/pkg/ae112802/7166082-rev3/webrev/


Below two files have updated in this webrev. Rest of the files are not 
changed.


   src/modules/client/history.py
   src/modules/client/__init__.py

Ran the "pkg history" command with C locale, unicode locale and eucJP 
locale. Verified

the output. Ran the complete test suite and all test cases passed.

Thanks,
Abhi.

On 06/29/12 07:26, Takeshi Asano wrote:

Hi Shawn, Danek, and Abhi.,

I overlooked the use of gettext.install() in module code
when reviewed rev-1 webrev, sorry.

Shawn is right; gettext.install() should not be used in module
code because it installs to global "_". Application may use
different message domain and/or different method.

It seems to me that just using common approach in modules
i.e. use ofgettext.translation() works. Am I missing point?

_ = gettext.translation("pkg", "/usr/share/locale", fallback=True,
codeset=locale.getpreferredencoding()).gettext

(this is often done in __init__.py and imported)

BTW, I think the misc.setlocale() call in the client/history.py
can be removed.

Thanks,
Takeshi


On 2012年06月29日 04:50, Danek Duvall wrote:

Shawn Walker wrote:


On 06/28/12 11:19, Danek Duvall wrote:

Shawn Walker wrote:


src/modules/client/history.py:
  lines 107-109: I don't think we should have this here.  Only the
main program module should be setting locale information.


When pkg command is run pkg.client.api is imported as api. The 
api in

turn imports pkg.client.history as history. During import the _()
function should be available. If not available then gets the error :

NameError: name '_' is not defined

So I think the lines 107 to 109 which install _() function cannot be
removed from history.py file.


Then we need to consider installing the gettext handler early 
before our

imports are done.


That might be trickier than the alternative, which is to redo the way
result_l10n is set in history.py.  We should probably call _() in 
the one
place it's actually used, and use N_(), possibly in the class 
definition,
or maybe in _HistoryOperation.__new__(), so that it gets put into 
pkg.pot.


Maybe just stuff the dict into result_text() and cache it on the first
execution.  Simple and should still get results into pkg.pot?


That'd work too.

Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss


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

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

Re: [pkg-discuss] code review request for CR 7166082

[Takeshi Asano]

Hi Shawn, Danek, and Abhi.,

I overlooked the use of gettext.install() in module code
when reviewed rev-1 webrev, sorry.

Shawn is right; gettext.install() should not be used in module
code because it installs to global "_". Application may use
different message domain and/or different method.

It seems to me that just using common approach in modules
i.e. use ofgettext.translation() works. Am I missing point?

_ = gettext.translation("pkg", "/usr/share/locale", fallback=True,
codeset=locale.getpreferredencoding()).gettext

(this is often done in __init__.py and imported)

BTW, I think the misc.setlocale() call in the client/history.py
can be removed.

Thanks,
Takeshi


On 2012年06月29日 04:50, Danek Duvall wrote:

Shawn Walker wrote:


On 06/28/12 11:19, Danek Duvall wrote:

Shawn Walker wrote:


src/modules/client/history.py:
  lines 107-109: I don't think we should have this here.  Only the
main program module should be setting locale information.


When pkg command is run pkg.client.api is imported as api. The api in
turn imports pkg.client.history as history. During import the _()
function should be available. If not available then gets the error :

NameError: name '_' is not defined

So I think the lines 107 to 109 which install _() function cannot be
removed from history.py file.


Then we need to consider installing the gettext handler early before our
imports are done.


That might be trickier than the alternative, which is to redo the way
result_l10n is set in history.py.  We should probably call _() in the one
place it's actually used, and use N_(), possibly in the class definition,
or maybe in _HistoryOperation.__new__(), so that it gets put into pkg.pot.


Maybe just stuff the dict into result_text() and cache it on the first
execution.  Simple and should still get results into pkg.pot?


That'd work too.

Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss


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

Re: [pkg-discuss] code review request for CR 7166082

[Danek Duvall]

Shawn Walker wrote:

> On 06/28/12 11:19, Danek Duvall wrote:
> >Shawn Walker wrote:
> >
> src/modules/client/history.py:
>   lines 107-109: I don't think we should have this here.  Only the
> main program module should be setting locale information.
> >>>
> >>>When pkg command is run pkg.client.api is imported as api. The api in
> >>>turn imports pkg.client.history as history. During import the _()
> >>>function should be available. If not available then gets the error :
> >>>
> >>>NameError: name '_' is not defined
> >>>
> >>>So I think the lines 107 to 109 which install _() function cannot be
> >>>removed from history.py file.
> >>
> >>Then we need to consider installing the gettext handler early before our
> >>imports are done.
> >
> >That might be trickier than the alternative, which is to redo the way
> >result_l10n is set in history.py.  We should probably call _() in the one
> >place it's actually used, and use N_(), possibly in the class definition,
> >or maybe in _HistoryOperation.__new__(), so that it gets put into pkg.pot.
> 
> Maybe just stuff the dict into result_text() and cache it on the first
> execution.  Simple and should still get results into pkg.pot?

That'd work too.

Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Shawn Walker]

On 06/28/12 11:19, Danek Duvall wrote:

Shawn Walker wrote:


src/modules/client/history.py:
  lines 107-109: I don't think we should have this here.  Only the
main program module should be setting locale information.


When pkg command is run pkg.client.api is imported as api. The api in
turn imports pkg.client.history as history. During import the _()
function should be available. If not available then gets the error :

NameError: name '_' is not defined

So I think the lines 107 to 109 which install _() function cannot be
removed from history.py file.


Then we need to consider installing the gettext handler early before our
imports are done.


That might be trickier than the alternative, which is to redo the way
result_l10n is set in history.py.  We should probably call _() in the one
place it's actually used, and use N_(), possibly in the class definition,
or maybe in _HistoryOperation.__new__(), so that it gets put into pkg.pot.


Maybe just stuff the dict into result_text() and cache it on the first 
execution.  Simple and should still get results into pkg.pot?


-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Danek Duvall]

Shawn Walker wrote:

> >>src/modules/client/history.py:
> >>  lines 107-109: I don't think we should have this here.  Only the
> >>main program module should be setting locale information.
> >
> >When pkg command is run pkg.client.api is imported as api. The api in
> >turn imports pkg.client.history as history. During import the _()
> >function should be available. If not available then gets the error :
> >
> >NameError: name '_' is not defined
> >
> >So I think the lines 107 to 109 which install _() function cannot be
> >removed from history.py file.
> 
> Then we need to consider installing the gettext handler early before our
> imports are done.

That might be trickier than the alternative, which is to redo the way
result_l10n is set in history.py.  We should probably call _() in the one
place it's actually used, and use N_(), possibly in the class definition,
or maybe in _HistoryOperation.__new__(), so that it gets put into pkg.pot.

Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Shawn Walker]

On 06/28/12 04:41, Abhinandan Ekande wrote:

Thanks Shawn for review.

On 06/27/12 03:41, Shawn Walker wrote:


local.getpreferredencoding() has some pitfalls.  As a result, I think
you're better of using the result of locale.getlocale()[1].

See this pages for details:

http://stackoverflow.com/questions/4082645/using-python-2-xs-locale-module-to-format-numbers-and-currency


The pydoc for getpreferredencoding() on docs.python.org also warns
that the result is a "guess" at user preferences.  So it seems like
getlocale() is the better answer, especially as we've already called
setlocale() at this point.  But it would be nice to know for certain.


I chatted regarding pitfalls for local.getpreferredencoding() with
Takeshi and they
are when locale is changed in the program. If locale is changed in the
program then
local.getpreferredencoding() still returns the old encoding. I checked
the source
code, in pkg commands we set locale to the user’s default setting only
and never
change the locale. So it is safe to use local.getpreferredencoding() in
pkg code.
Please let me know your opinion.



src/modules/client/history.py:
  lines 107-109: I don't think we should have this here.  Only the
main program module should be setting locale information.


When pkg command is run pkg.client.api is imported as api. The api in
turn imports
pkg.client.history as history. During import the _() function should be
available. If not
available then gets the error :

NameError: name '_' is not defined

So I think the lines 107 to 109 which install _() function cannot be
removed from
history.py file.


Then we need to consider installing the gettext handler early before our 
imports are done.


The python docs clearly state that imported modules should never install 
gettext themselves in that way.


-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7140762

[Bart Smaalders]

On 06/27/12 23:52, Abhinandan Ekande wrote:



A better fix would cwd out of the directory being deleted, no?


I agree with Bart; we should chdir to the parent directory of the
repository if the working directory is inside of it.

I wouldn't want us to just ignore the EINVAL.


Thanks Bart and Shawn for review. I have incorporated the suggestion.
The updated webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7140762-rev2/webrev/

Please provide comments.


What happens if another file exists in that directory?


Perhaps:

   def rmdir(d):

 """rmdir; but ignores non-empty directories."""
 try:
 os.rmdir(d)
 except OSError, e:
 if e.errno == errno.EINVAL and \
 os.getcwd != self.root:
 # In case of EINVAL error we could be
 # in directory we're removing;
 # chdir to repository root and
 # recurse
 os.chdir(self.root)
 rmdir(d)
 elif e.errno in (
 errno.ENOTEMPTY,
 errno.EEXIST):
 pass
 else:
 raise

- Bart

--
Bart Smaalders  Solaris Kernel Performance
bart.smaald...@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-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Abhinandan Ekande]

Thanks Shawn for review.

On 06/27/12 03:41, Shawn Walker wrote:


local.getpreferredencoding() has some pitfalls.  As a result, I think 
you're better of using the result of locale.getlocale()[1].


See this pages for details:

http://stackoverflow.com/questions/4082645/using-python-2-xs-locale-module-to-format-numbers-and-currency 



The pydoc for getpreferredencoding() on docs.python.org also warns 
that the result is a "guess" at user preferences.  So it seems like 
getlocale() is the better answer, especially as we've already called 
setlocale() at this point.  But it would be nice to know for certain.


I chatted regarding pitfalls for local.getpreferredencoding() with 
Takeshi and they
are when locale is changed in the program. If locale is changed in the 
program then
local.getpreferredencoding() still returns the old encoding. I checked 
the source
code, in pkg commands we set locale to the user’s default setting only 
and never
change the locale. So it is safe to use local.getpreferredencoding() in 
pkg code.

Please let me know your opinion.



src/modules/client/history.py:
  lines 107-109: I don't think we should have this here.  Only the 
main program module should be setting locale information.


When pkg command is run pkg.client.api is imported as api. The api in 
turn imports
pkg.client.history as history. During import the _() function should be 
available. If not

available then gets the error :

   NameError: name '_' is not defined

So I think the lines 107 to 109 which install _() function cannot be 
removed from

history.py file.



src/sign.py:
  update copyright

src/util/publish/pkgfmt.py:
  update copyright

src/util/publish/pkglint.py:
  update copyright

src/util/publish/pkgmogrify.py:
  update copyright 


Thanks for pointing. I have modified the copyright in my workspace.

Abhi.
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7140762

[Abhinandan Ekande]

A better fix would cwd out of the directory being deleted, no?


I agree with Bart; we should chdir to the parent directory of the 
repository if the working directory is inside of it.


I wouldn't want us to just ignore the EINVAL.


Thanks Bart and Shawn for review. I have incorporated the suggestion.
The updated webrev : 
https://cr.opensolaris.org/action/browse/pkg/ae112802/7140762-rev2/webrev/


Please provide comments.

Abhi.
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7140762

[Shawn Walker]

On 06/26/12 11:23, Bart Smaalders wrote:

On 06/26/12 02:32, Abhinandan Ekande wrote:

Folks,

Please review simple fix for CR :
7140762 pkgrepo remove dies when run from the package directory that is
being removed

webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7140762-rev1/webrev/


Unit testing done for pkgrepo command.

Ran the test suite. All test passed.



A better fix would cwd out of the directory being deleted, no?


I agree with Bart; we should chdir to the parent directory of the 
repository if the working directory is inside of it.


I wouldn't want us to just ignore the EINVAL.

-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Shawn Walker]

On 06/25/12 05:32, Abhinandan Ekande wrote:

Folks,

Here is updated webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7166082-rev2/webrev/

Incorporated all the comments except the requirement on system/local/extra
package.

The major change included in this webrev is test case has been modified.
Replaced
test case for "pkg publisher" command with test case for "pkg --help"
command.
With the integration of changeset 2701 output of "pkg publisher" command
changed.
Basically the header for "pkg publisher" command has changed. So we have
selected
"pkg --help" command which is relatively stable. Please let me know
feedback on
the latest webrev.


local.getpreferredencoding() has some pitfalls.  As a result, I think 
you're better of using the result of locale.getlocale()[1].


See this pages for details:

http://stackoverflow.com/questions/4082645/using-python-2-xs-locale-module-to-format-numbers-and-currency

The pydoc for getpreferredencoding() on docs.python.org also warns that 
the result is a "guess" at user preferences.  So it seems like 
getlocale() is the better answer, especially as we've already called 
setlocale() at this point.  But it would be nice to know for certain.


src/modules/client/history.py:
  lines 107-109: I don't think we should have this here.  Only the main 
program module should be setting locale information.


src/sign.py:
  update copyright

src/util/publish/pkgfmt.py:
  update copyright

src/util/publish/pkglint.py:
  update copyright

src/util/publish/pkgmogrify.py:
  update copyright


-Shawn
-Shawn
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7140762

[Bart Smaalders]

On 06/26/12 02:32, Abhinandan Ekande wrote:

Folks,

Please review simple fix for CR :
7140762 pkgrepo remove dies when run from the package directory that is
being removed

webrev :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7140762-rev1/webrev/

Unit testing done for pkgrepo command.

Ran the test suite. All test passed.



A better fix would cwd out of the directory being deleted, no?

- Bart

--
Bart Smaalders  Solaris Kernel Performance
bart.smaald...@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-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request for CR 7166082

[Abhinandan Ekande]

Folks,

Here is updated webrev :

https://cr.opensolaris.org/action/browse/pkg/ae112802/7166082-rev2/webrev/


Incorporated all the comments except the requirement on system/local/extra
package.

The major change included in this webrev is test case has been modified. 
Replaced
test case for "pkg publisher" command with test case for "pkg --help" 
command.
With the integration of changeset 2701 output of "pkg publisher" command 
changed.
Basically the header for "pkg publisher" command has changed. So we have 
selected
"pkg --help" command which is relatively stable. Please let me know 
feedback on

the latest webrev.

Thanks,
Abhi.

On 06/21/12 16:21, Abhinandan Ekande wrote:

Thanks Danek, Takeshi and Pete for review.
I will incorporate the comments and will send the updated webrev.

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

Re: [pkg-discuss] code review request - release-note support.

[Danek Duvall]

Bart Smaalders wrote:

> https://cr.opensolaris.org/action/browse/pkg/barts/release-notes/webrev/

client.py:

  - line 1124, 5998: do the two spaces before the "%s" indent the entire
release note, or just the first line?

  - line 1127, 1133: Put single quotes around the command.

  - line 1145: lowercase the B.  Actually, you probably want to "except
Exception: pass", which will let through KeyboardInterrupt and
SystemExit (and GeneratorExit, but that shouldn't come up here).

  - line 1147: you can skip this

  - line 1204: might be more elegant to skip the if test -- if
get_release_notes() is an empty iteration, then nothing will happen to
release_notes.  And actually, it'd be nice to see the release notes be
a property on the PlanDescription object, rather than having these two
functions.  But licenses are done the way you've done these, and they
could both be changed in the future.

  - line 5996, 6000: i18n

actuator.py:

  - lines 60, 68, 76: space after comma

history.py:

  - line 444: might as well use "with f as file(): for a in ..." to do all
the automatic cleanup.

  - line 446: shouldn't need this clause

imageplan.py:

  - line 2031, et al: blank line between docstring and code.

  - line 2036: I think you need to flesh this comment out a bit more.

  - line 2037ff: Maybe make this a single if statement with three anded
clauses:

if pfmri.pkg_name == "feature/pkg/self" and \
str(pfmri.version) == "0,5.11" and \
containing_fmri.pkg_name not in installed_dict:
return True

pfmri.pkg_name = containing_fmri.pkg_name
...

  - line 2069: I think it would help to be very precise.  You're taking raw
string, and converting them, if necessary, to unicode objects (and to
ascii str objects otherwise), assuming a utf-8 encoding.  Am I correct?

  - line 2070: "convertible"

  - line 2072: so why "encode" instead of "decode" here?

  - line 2073: probably want to use UnicodeError here.

  - line 2083: would this be easier to read?

must_display = act.attrs.get("must-display", "false") == "true"?

  - line 2098: looks to me like you're putting all of the release notes for
a given operation (like "pkg update") into a single file, rather than a
single file for each release note.  Is that intentional?  Regardless,
might be nice to have the release notes sorted by operation timestamp
-- can you extract that from a history object somehow?

  - line 2100: why the "+"?

  - line 2104: maybe tmpfile.write(note)?

  - line 2105: need to close fd, too?

plandesc.py:

  - line 523: return bool(self.release_notes[1])

t_actuators.py:

  - line 474: yeah, but what about your sister and that møøse?

  - line 491, et al: So this will simply raise ValueError, making the test
case error, rather than making the test case fail (this also applies to
line 501).  You should probably do .find() and assert (with assert_())
that the result isn't -1.  There may even be a helper function for this
in pkg5unittest (and if there isn't, maybe there should be).

  - line 499: you're not looking for the availability line (though I see
you're doing that in test 2).

  - line 528: were you intending to test that each of these strings showed
up on different lines?

  - line 559: blank line after this

  - Did you want to test that release notes that were neither ascii nor
utf-8 did something vaguely correct?

Thanks,
Danek
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Re: [pkg-discuss] code review request - release-note support.

[Brock Pytlik]

On 06/22/12 15:42, Bart Smaalders wrote:

https://cr.opensolaris.org/action/browse/pkg/barts/release-notes/webrev/

7137406 pkg image-update could display release notes (or pointer to)

This changeset doesn't include man page changes.
[snip]

Without -v, if any release notes apply to the upgrade the user has
performed, he would be reminded to read them and told how to do so.
If any file with a release note that applies has must-display=true as
well, the release notes will be always be displayed whether or not -v is
used. Release notes for previous packaging operations can be read (&
file name retrieved) using pkg history -N. Release notes prepared for
new boot environments are stored in the new BE; they are also written
to a temporary file and a path given so they may be reviewed easily
prior to reboot.
Just to clarify, if one release note has must-display=true set, then all 
release notes are displayed?


Other than that, everything seems to be nits to me.

client.py:
1127, 1131, 1133, 1139: nit line wrap

1205,1206: why not release_notes = plan.get_release_notes()? Or 
list(plan.get_release_notes())?


api.py:
4761: nit: indent 4 spaces

history.py:
444: does the file opened here ever get closed? do we care?

imageplan.py:
2030: it might be nice to dump a copy of some of what was in the email 
above in as a comment/docstring so that it's easier in the future to 
know what the code's supposed to do


2087: Just curious, why does the decoding need to be done line by line 
instead of note by note?


2101-2104: should this be in a try block, with 2105 in a finally block?

misc.py:
1980: maybe I'm just being dense, but I can't figure out why this change 
was needed


pkg5unittest.py:
808: nit: line wrap

Brock



- Bart



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