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

[Abhinandan Ekande]

On 05/16/12 23:57, Danek Duvall wrote:

Sorry for the long delay.

Change looks good, though I'd add an assertion that unicode_list isn't
empty, with an explanation of the failure.  Something like

 self.assert_(unicode_list, "You must have one of the following locales "
 "installed for this test to succeed: " + ", ".join(unicode_locales))

And on line 583, in the dictionary, don't put a space before the colon.

   


Thanks Danek for review. I have incorporated the above suggestion. The
updated webrev is :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7140680-rev4/webrev/

I have also attached patch. Could you please integrate it.

Abhi.

--
Oracle
Abhinandan Ekande
Solaris Install,
Revenue Product Engineering (RPE), Systems
Phone: +91 8067283830 | Fax: +91 80 22231794 | Mobile: +91 9632144088
ORACLE India | Off Langford Road | Bangalore | 560025
Green Oracle  Oracle is committed to 
developing practices and products that help protect the environment
# HG changeset patch
# User abhinandan.eka...@oracle.com
# Date 1337243642 -19800
# Node ID 3bdd03f9d72cf4f84a2a9d5c28724d1011bf78b5
# Parent  41a4fd204b0b2ec5ad9239802e6026d0d21e4e69
7140680 "pkg history" does not work in some localized locales

diff -r 41a4fd204b0b -r 3bdd03f9d72c src/client.py
--- a/src/client.py Mon May 14 09:13:08 2012 -0700
+++ b/src/client.py Thu May 17 14:04:02 2012 +0530
@@ -5652,9 +5652,17 @@
 if not output["new_be_uuid"]:
 output["new_be_uuid"] = _("(None)")
 
+enc = locale.getlocale(locale.LC_CTYPE)[1]
+if not enc:
+enc = locale.getpreferredencoding()
+
 if long_format:
 data = __get_long_history_data(he, output)
 for field, value in data:
+if isinstance(field, unicode):
+field = field.encode(enc)
+if isinstance(value, unicode):
+value = value.encode(enc)
 msg("%18s: %s" % (field, value))
 
 # Separate log entries with a blank line.
@@ -5662,7 +5670,10 @@
 else:
 items = []
 for col in columns:
-items.append(output[col])
+item = output[col]
+if isinstance(item, unicode):
+item = item.encode(enc)
+items.append(item)
 msg(history_fmt % tuple(items))
 return EXIT_OK
 
diff -r 41a4fd204b0b -r 3bdd03f9d72c src/tests/cli/t_pkg_history.py
--- a/src/tests/cli/t_pkg_history.pyMon May 14 09:13:08 2012 -0700
+++ b/src/tests/cli/t_pkg_history.pyThu May 17 14:04:02 2012 +0530
@@ -34,6 +34,7 @@
 import random
 import re
 import shutil
+import subprocess
 import time
 import unittest
 import xml.etree.ElementTree
@@ -567,5 +568,22 @@
 self.pkg("history -n 1 -o time")
 self.assert_("369576:0:0" in self.output)
 
+def test_14_history_unicode_locale(self):
+"""Verify we can get history when unicode locale is set"""
+
+# If pkg history run when below locales set, it fails.
+unicode_locales = ["fr_FR.UTF-8", "zh_TW.UTF-8", "zh_CN.UTF-8",
+"ko_KR.UTF-8", "ja_JP.UTF-8"]
+p = subprocess.Popen(["/usr/bin/locale", "-a"],
+stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+lines = p.stdout.readlines()
+locale_list = [i.rstrip() for i in lines]
+unicode_list = list(set(locale_list) & set(unicode_locales))
+self.assert_(unicode_list, "You must have one of the "
+" following locales installed for this test to succeed: "
++ ", ".join(unicode_locales))
+env = { "LC_ALL": unicode_list[0] }
+self.pkg("history", env_arg=env)
+
 if __name__ == "__main__":
 unittest.main()
diff -r 41a4fd204b0b -r 3bdd03f9d72c src/tests/pkg5unittest.py
--- a/src/tests/pkg5unittest.py Mon May 14 09:13:08 2012 -0700
+++ b/src/tests/pkg5unittest.py Thu May 17 14:04:02 2012 +0530
@@ -2211,7 +2211,7 @@
 
 def pkg(self, command, exit=0, comment="", prefix="", su_wrap=None,
 out=False, stderr=False, cmd_path=None, use_img_root=True,
-debug_smf=True):
+debug_smf=True, env_arg=None):
 if debug_smf and "smf_cmds_dir" not in command:
 command = "--debug smf_cmds_dir=%s %s" % \
 (DebugValues["smf_cmds_dir"], command)
@@ -,7 +,8 @@
 cmd_path = self.pkg_cmdpath
 cmdline = "%s 

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

[Danek Duvall]

Abhinandan Ekande wrote:

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

Sorry for the long delay.

Change looks good, though I'd add an assertion that unicode_list isn't
empty, with an explanation of the failure.  Something like

self.assert_(unicode_list, "You must have one of the following locales "
"installed for this test to succeed: " + ", ".join(unicode_locales))

And on line 583, in the dictionary, don't put a space before the colon.

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 7140680

[Abhinandan Ekande]

Looking for a review ...

Thanks,
Abhi.

On 04/19/12 11:33, Abhinandan Ekande wrote:

Thanks Danek for review.

On 04/18/12 02:25, Danek Duvall wrote:

t_pkg_history.py:

   - Where do we get the localizations for use during the test suite?
 /usr/share/locale, or from the gate or proto area somewhere?  
(Would

 this test fail if I didn't have the Chinese locale installed?)


Good point. I have modified the code to get locale for which
"pkg history" command fails from locale command.

   - line 574: no need for the second argument.  Though I think I'd 
rather

 see you add support for cmdline_run()'s "env_arg" argument to pkg()
 instead of setting the environment globally.

   - line 577: no need for "exit=0"; that's the default


Incorporated above comments too.

The updated webrev is at :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7140680-rev3/webrev/ 



Ran the complete test suite, no failures reported.

Abhi.


Also, please use double quotes instead of single quotes unless there's a
good reason to do otherwise.

Thanks,
Danek






--
Oracle
Abhinandan Ekande
Solaris Install,
Revenue Product Engineering (RPE), Systems
Phone: +91 8041847267 | Fax: +91 80 22231794 | Mobile: +91 9632144088
ORACLE India | Off Langford Road | Bangalore | 560025
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 7140680

[Abhinandan Ekande]

Thanks Danek for review.

On 04/18/12 02:25, Danek Duvall wrote:

t_pkg_history.py:

   - Where do we get the localizations for use during the test suite?
 /usr/share/locale, or from the gate or proto area somewhere?  (Would
 this test fail if I didn't have the Chinese locale installed?)
   


Good point. I have modified the code to get locale for which
"pkg history" command fails from locale command.


   - line 574: no need for the second argument.  Though I think I'd rather
 see you add support for cmdline_run()'s "env_arg" argument to pkg()
 instead of setting the environment globally.

   - line 577: no need for "exit=0"; that's the default
   


Incorporated above comments too.

The updated webrev is at :
https://cr.opensolaris.org/action/browse/pkg/ae112802/7140680-rev3/webrev/

Ran the complete test suite, no failures reported.

Abhi.


Also, please use double quotes instead of single quotes unless there's a
good reason to do otherwise.

Thanks,
Danek
   



--
Oracle
Abhinandan Ekande
Solaris Install,
Revenue Product Engineering (RPE), Systems
Phone: +91 8041847267 | Fax: +91 80 22231794 | Mobile: +91 9632144088
ORACLE India | Off Langford Road | Bangalore | 560025
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 7140680

[Danek Duvall]

Abhinandan Ekande wrote:

> On 04/07/12 03:14, Shawn Walker wrote:
> >For the record, once you've added a test case, this seems fine to me;
> >we'll have to address the other problems separately.
> 
> Thanks Shawn for review.
> 
> Here is updated webrev with test case :
> https://cr.opensolaris.org/action/browse/pkg/saurabhv/7140680-rev2/webrev/

t_pkg_history.py:

  - Where do we get the localizations for use during the test suite?
/usr/share/locale, or from the gate or proto area somewhere?  (Would
this test fail if I didn't have the Chinese locale installed?)

  - line 574: no need for the second argument.  Though I think I'd rather
see you add support for cmdline_run()'s "env_arg" argument to pkg()
instead of setting the environment globally.

  - line 577: no need for "exit=0"; that's the default

Also, please use double quotes instead of single quotes unless there's a
good reason to do otherwise.

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 7140680

[Abhinandan Ekande]

On 04/07/12 03:14, Shawn Walker wrote:
For the record, once you've added a test case, this seems fine to me; 
we'll have to address the other problems separately.


Thanks Shawn for review.

Here is updated webrev with test case :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/7140680-rev2/webrev/

Ran the complete IPS test suite. No new failures reported.
Without fix verified that newly introduced test reports a failure.

I have filed RFE for custom logger -- CR 7161772.

Thanks,
Abhi.

--
Oracle
Abhinandan Ekande
Solaris Install,
Revenue Product Engineering (RPE), Systems
Phone: +91 8041847267 | Fax: +91 80 22231794 | Mobile: +91 9632144088
ORACLE India | Off Langford Road | Bangalore | 560025
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 7140680

[Shawn Walker]

On 03/22/12 05:18, Abhinandan Ekande wrote:

Folks,

Please review fix for CR :
7140680 "pkg history" does not work in some localized locales

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

Unit testing done :
With fix in place ran the 'pkg history' command with locales,
fr_FR.UTF-8, zh_TW.UTF-8,
zh_CN.UTF-8, de_DE.UTF-8, ja_JP.UTF-8 and ko_KR.UTF-8. The command was
successful.

Also ran the IPS test suite. No new failures reported.


For the record, once you've added a test case, this seems fine to me; 
we'll have to address the other problems separately.


-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 7140680

[Shawn Walker]

On 03/27/12 08:12, Bart Smaalders wrote:

On 03/22/12 05:18, Abhinandan Ekande wrote:

Folks,

Please review fix for CR :
7140680 "pkg history" does not work in some localized locales

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




Why did this part of pkg have problems w/ encodings, but no other? Are
there more bugs related to this problem?


This is one of the few places where unicode objects are actually 
encountered.


However, this is not the only place with a potential issue.

We need a custom log formatter for the logging module usage to deal with 
this [1], and msg() and emsg() in pkg.misc likely need fixing as well.


Python's encoding behaviour has unfortunately changed multiple times 
since 2.4 and even during the lifetime of 2.6.


What's worse is that in the case that pygtk or pango are imported (like 
the packagemanager does), Python's default encoding is changed to the 
current locale so this is unnecessary [2].  (Although it shouldn't hurt 
from what I can tell in that case.)


-Shawn

[1] 
http://tony.czechit.net/2009/02/unicode-support-for-pythons-logging-library/


[2] https://bugzilla.gnome.org/show_bug.cgi?id=132040
___
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

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

[Bart Smaalders]

On 03/22/12 05:18, Abhinandan Ekande wrote:

Folks,

Please review fix for CR :
7140680 "pkg history" does not work in some localized locales

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



Why did this part of pkg have problems w/ encodings, but no other?  Are
there more bugs related to this problem?


Unit testing done :
With fix in place ran the 'pkg history' command with locales,
fr_FR.UTF-8, zh_TW.UTF-8,
zh_CN.UTF-8, de_DE.UTF-8, ja_JP.UTF-8 and ko_KR.UTF-8. The command was
successful.



I want a test case added to check this, please.


Also ran the IPS test suite. No new failures reported.


That should be NO failures reported.


- 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 7140680

[Abhinandan Ekande]

Folks,

Looking for a review of this CR.

Thanks,
Abhi.

On 03/22/12 17:48, Abhinandan Ekande wrote:

Folks,

Please review fix for CR :
7140680 "pkg history" does not work in some localized locales

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



Unit testing done :
With fix in place ran the 'pkg history' command with locales, 
fr_FR.UTF-8, zh_TW.UTF-8,
zh_CN.UTF-8, de_DE.UTF-8, ja_JP.UTF-8 and ko_KR.UTF-8. The command was 
successful.


Also ran the IPS test suite. No new failures reported.

Thanks,
Abhi.




--
Oracle
Abhinandan Ekande
Solaris Install,
Revenue Product Engineering (RPE), Systems
Phone: +91 8041847267 | Fax: +91 80 22231794 | Mobile: +91 9632144088
ORACLE India | Off Langford Road | Bangalore | 560025
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