Sun Studio 11 & 12 Support on Solaris

[Vlad, 2009-11-10 21:40:24]

Hi Hady,

I have some comments on the path. In some places I ask you to add comments. This
is important - if some future maintainer decides to improve the code, without
seeinc those comments she may very well remove Solaris idiosyncrasies.

Thanks,
Vlad

http://codereview.appspot.com/153042/diff/1/3
File test/gtest_output_test.py (right):

http://codereview.appspot.com/153042/diff/1/3#newcode194
test/gtest_output_test.py:194: old_env_vars = dict(os.environ)
These os.environ changes effectively block the script from setting up
environments for the child processes. Does this test still pass with them?

http://codereview.appspot.com/153042/diff/73/85
File include/gtest/gtest-typed-test.h (right):

http://codereview.appspot.com/153042/diff/73/85#newcode163
include/gtest/gtest-typed-test.h:163: typedef ::testing::internal::TypeList<
Types >::type \
Are the spaces around Types essential? If so, please add a comment why.

http://codereview.appspot.com/153042/diff/73/85#newcode248
include/gtest/gtest-typed-test.h:248: ::testing::internal::TypeList< Types
>::type>::Register(\
Please also comment here.

http://codereview.appspot.com/153042/diff/73/84
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/153042/diff/73/84#newcode939
include/gtest/internal/gtest-port.h:939: #elif defined(__BORLANDC__) ||
defined(__SunOS_5_8) || defined(__SunOS_5_9)
Can't this be written more easily as 

#elif defined(__BORLANDC__) || GTEST_OS_SOLARIS

- here and in gtest_unittest.cc as well?

http://codereview.appspot.com/153042/diff/73/86
File src/gtest-typed-test.cc (right):

http://codereview.appspot.com/153042/diff/73/86#newcode56
src/gtest-typed-test.cc:56: registered_tests = SkipSpaces(registered_tests);
Please add a comment on why this is necessary.

http://codereview.appspot.com/153042/diff/73/75
File test/gtest_filter_unittest.py (right):

http://codereview.appspot.com/153042/diff/73/75#newcode71
test/gtest_filter_unittest.py:71: del os.environ['CAN_UNSET_ENV']
As mentioned in gtest_output_test.py, clearing an environment variable using del
on os.environ does not really work until Python v2.6 - the change will not be
seen by subprocesses. One has to use an alternative method.

What is the version of Python on your system?

http://codereview.appspot.com/153042/diff/73/80
File test/gtest_output_test.py (right):

http://codereview.appspot.com/153042/diff/73/80#newcode195
test/gtest_output_test.py:195: old_env_vars = dict(environ)
Making environ global prevents any the external environment variables from being
passed into test programs. This is a change that may potentially break tests
there.

Try making environ a local variable:

environ = os.environ.copy()
environ.update(env_cmd[0])
return gtest_test_utils.Subprocess(env_cmd[1], env=environ).output

This frees you from any responsibility of cleaning [os.]environ after the
Subprocess call.

http://codereview.appspot.com/153042/diff/73/80#newcode220
test/gtest_output_test.py:220: environ['GTEST_CATCH_EXCEPTIONS'] = '1'
If environ is made a local variable, this may be encapsulated as

environ, cmdline = env_cmd
environ = dict(environ)  # Ensures we are modifying a copy.
environ['GTEST_CATCH_EXCEPTIONS'] = '1'
return NormalizeOutput(GetShellCommandOutput((environ, cmdline)))

http://codereview.appspot.com/153042/diff/73/79
File test/gtest_test_utils.py (right):

http://codereview.appspot.com/153042/diff/73/79#newcode193
test/gtest_test_utils.py:193: def __init__(self, command, working_dir=None,
capture_stderr=True, env=None):
Passing env into Subprocess is a great idea, but you also need to implement it
when the subprocess module is not available (gtest officially supports OS X
10.3, which has Python 2.3). You can test it with by setting
_SUBPROCESS_MODULE_AVAILABLE to False here.

http://codereview.appspot.com/153042/diff/73/81
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/153042/diff/73/81#newcode2111
test/gtest_unittest.cc:2111: added_env[name] = new String((Message() << name <<
"=" << value).GetString());
If putenv on Solaris only accepts char*, you need to add a comment mentioning
it.

http://codereview.appspot.com/153042/diff/73/81#newcode3070
test/gtest_unittest.cc:3070: static const FloatTest::TestValues& v =
this->values_;
Please also add a comment why is it essential to use the assignment syntax.

[Hady, 2009-11-11 14:44:26]

Hi Vlad,

I uploaded a new patch set taking your suggestions into account. Could you
please take a look. There's still
http://codereview.appspot.com/153042/diff/73/75#newcode71 which requires your
input.

Thanks!
had

http://codereview.appspot.com/153042/diff/1/3
File test/gtest_output_test.py (right):

http://codereview.appspot.com/153042/diff/1/3#newcode194
test/gtest_output_test.py:194: old_env_vars = dict(os.environ)
On 2009/11/10 21:40:24, Vlad wrote:
> These os.environ changes effectively block the script from setting up
> environments for the child processes. Does this test still pass with them?
Yeah all tests pass on (NT, Linux, Solaris, and AIX). Are you suggesting that we
no longer iterate and delete the keys since a clear would suffice? Couldn't we
even simplify further? copy environ to old_env_vars, update old_env_vars with
env_cmd[0], pass old_env_vars to Subprocess, no more clearing and restoring of
environ...

http://codereview.appspot.com/153042/diff/1/3#newcode194
test/gtest_output_test.py:194: old_env_vars = dict(os.environ)
On 2009/11/10 21:40:24, Vlad wrote:
> These os.environ changes effectively block the script from setting up
> environments for the child processes. Does this test still pass with them?

Done.

http://codereview.appspot.com/153042/diff/73/85
File include/gtest/gtest-typed-test.h (right):

http://codereview.appspot.com/153042/diff/73/85#newcode163
include/gtest/gtest-typed-test.h:163: typedef ::testing::internal::TypeList<
Types >::type \
On 2009/11/10 21:40:24, Vlad wrote:
> Are the spaces around Types essential? If so, please add a comment why.

Yes they are. Otherwise Sun Studio chokes. I also think it's recommended
surrounding template arguments with spaces. Do you want a more technical comment
or would the fact that SS doesn't parse the code suffice?

http://codereview.appspot.com/153042/diff/73/85#newcode163
include/gtest/gtest-typed-test.h:163: typedef ::testing::internal::TypeList<
Types >::type \
On 2009/11/10 21:40:24, Vlad wrote:
> Are the spaces around Types essential? If so, please add a comment why.

Done.

http://codereview.appspot.com/153042/diff/73/85#newcode248
include/gtest/gtest-typed-test.h:248: ::testing::internal::TypeList< Types
>::type>::Register(\
On 2009/11/10 21:40:24, Vlad wrote:
> Please also comment here.

Done.

http://codereview.appspot.com/153042/diff/73/84
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/153042/diff/73/84#newcode939
include/gtest/internal/gtest-port.h:939: #elif defined(__BORLANDC__) ||
defined(__SunOS_5_8) || defined(__SunOS_5_9)
On 2009/11/10 21:40:24, Vlad wrote:
> Can't this be written more easily as 
> 
> #elif defined(__BORLANDC__) || GTEST_OS_SOLARIS
> 
> - here and in gtest_unittest.cc as well?

Actually, you can't since Solaris >= 5.10 adds unsetenv and doesn't require this
hack.

http://codereview.appspot.com/153042/diff/73/84#newcode939
include/gtest/internal/gtest-port.h:939: #elif defined(__BORLANDC__) ||
defined(__SunOS_5_8) || defined(__SunOS_5_9)
On 2009/11/10 21:40:24, Vlad wrote:
> Can't this be written more easily as 
> 
> #elif defined(__BORLANDC__) || GTEST_OS_SOLARIS
> 
> - here and in gtest_unittest.cc as well?

Done.

http://codereview.appspot.com/153042/diff/73/87
File src/gtest-port.cc (right):

http://codereview.appspot.com/153042/diff/73/87#newcode113
src/gtest-port.cc:113: {
Added a comment here.

http://codereview.appspot.com/153042/diff/73/86
File src/gtest-typed-test.cc (right):

http://codereview.appspot.com/153042/diff/73/86#newcode56
src/gtest-typed-test.cc:56: registered_tests = SkipSpaces(registered_tests);
On 2009/11/10 21:40:24, Vlad wrote:
> Please add a comment on why this is necessary.

Done.

http://codereview.appspot.com/153042/diff/73/86#newcode56
src/gtest-typed-test.cc:56: registered_tests = SkipSpaces(registered_tests);
On 2009/11/10 21:40:24, Vlad wrote:
> Please add a comment on why this is necessary.

Done.

http://codereview.appspot.com/153042/diff/73/75
File test/gtest_filter_unittest.py (right):

http://codereview.appspot.com/153042/diff/73/75#newcode71
test/gtest_filter_unittest.py:71: del os.environ['CAN_UNSET_ENV']
On 2009/11/10 21:40:24, Vlad wrote:
> As mentioned in gtest_output_test.py, clearing an environment variable using
del
> on os.environ does not really work until Python v2.6 - the change will not be
> seen by subprocesses. One has to use an alternative method.
> 
> What is the version of Python on your system?
I have Python 2.5 on all the systems I tested on. Are you sure about this? The
doc (http://www.python.org/doc/2.5.1/lib/os-procinfo.html) seems to imply that
deleting an item in os.environ removes the environment variable iff unsetenv()
is supported. Alternatively, I could check to see if the Python unsetenv
function exists.

http://codereview.appspot.com/153042/diff/73/80
File test/gtest_output_test.py (right):

http://codereview.appspot.com/153042/diff/73/80#newcode195
test/gtest_output_test.py:195: old_env_vars = dict(environ)
On 2009/11/10 21:40:24, Vlad wrote:
> Making environ global prevents any the external environment variables from
being
> passed into test programs. This is a change that may potentially break tests
> there.
> 
> Try making environ a local variable:
> 
> environ = os.environ.copy()
> environ.update(env_cmd[0])
> return gtest_test_utils.Subprocess(env_cmd[1], env=environ).output
> 
> This frees you from any responsibility of cleaning [os.]environ after the
> Subprocess call.

Done.

http://codereview.appspot.com/153042/diff/73/80#newcode220
test/gtest_output_test.py:220: environ['GTEST_CATCH_EXCEPTIONS'] = '1'
On 2009/11/10 21:40:24, Vlad wrote:
> If environ is made a local variable, this may be encapsulated as
> 
> environ, cmdline = env_cmd
> environ = dict(environ)  # Ensures we are modifying a copy.
> environ['GTEST_CATCH_EXCEPTIONS'] = '1'
> return NormalizeOutput(GetShellCommandOutput((environ, cmdline)))

Done.

http://codereview.appspot.com/153042/diff/73/79
File test/gtest_test_utils.py (right):

http://codereview.appspot.com/153042/diff/73/79#newcode193
test/gtest_test_utils.py:193: def __init__(self, command, working_dir=None,
capture_stderr=True, env=None):
On 2009/11/10 21:40:24, Vlad wrote:
> Passing env into Subprocess is a great idea, but you also need to implement it
> when the subprocess module is not available (gtest officially supports OS X
> 10.3, which has Python 2.3). You can test it with by setting
> _SUBPROCESS_MODULE_AVAILABLE to False here.

Done.

http://codereview.appspot.com/153042/diff/73/81
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/153042/diff/73/81#newcode2111
test/gtest_unittest.cc:2111: added_env[name] = new String((Message() << name <<
"=" << value).GetString());
On 2009/11/10 21:40:24, Vlad wrote:
> If putenv on Solaris only accepts char*, you need to add a comment mentioning
> it.

Ok. It's actually the Borland case which is non-standard. Added a comment
explaining.

http://codereview.appspot.com/153042/diff/73/81#newcode2111
test/gtest_unittest.cc:2111: added_env[name] = new String((Message() << name <<
"=" << value).GetString());
On 2009/11/10 21:40:24, Vlad wrote:
> If putenv on Solaris only accepts char*, you need to add a comment mentioning
> it.

Done.

http://codereview.appspot.com/153042/diff/73/81#newcode3070
test/gtest_unittest.cc:3070: static const FloatTest::TestValues& v =
this->values_;
On 2009/11/10 21:40:24, Vlad wrote:
> Please also add a comment why is it essential to use the assignment syntax.

Done.

[Vlad, 2009-11-16 02:01:26]

Nice, we are close! I have a just a few more comments. I have also filed
http://code.google.com/p/googletest/issues/detail?id=224 to further simplify
Python test scripts. It will require rather extensive changes to the test
scripts, so please keep an eye on it and let me know if it breaks them on
Solaris, OK?

Thanks,
Vlad

http://codereview.appspot.com/153042/diff/73/75
File test/gtest_filter_unittest.py (right):

http://codereview.appspot.com/153042/diff/73/75#newcode71
test/gtest_filter_unittest.py:71: del os.environ['CAN_UNSET_ENV']
On 2009/11/11 14:44:27, Hady wrote:
> On 2009/11/10 21:40:24, Vlad wrote:
> > As mentioned in gtest_output_test.py, clearing an environment variable using
> del
> > on os.environ does not really work until Python v2.6 - the change will not
be
> > seen by subprocesses. One has to use an alternative method.
> > 
> > What is the version of Python on your system?
> I have Python 2.5 on all the systems I tested on. Are you sure about this? The
> doc (http://www.python.org/doc/2.5.1/lib/os-procinfo.html) seems to imply that
> deleting an item in os.environ removes the environment variable iff unsetenv()
> is supported. Alternatively, I could check to see if the Python unsetenv
> function exists.
> 

Oh, sorry, this is my mistake.

http://codereview.appspot.com/153042/diff/1067/2012
File include/gtest/gtest-typed-test.h (right):

http://codereview.appspot.com/153042/diff/1067/2012#newcode164
include/gtest/gtest-typed-test.h:164: // instance (e.g. Types<int>)
Hrm. This really a cross-platform issue - and a real bug in our code. Thanks for
catching it!

http://codereview.appspot.com/153042/diff/1067/2002
File test/gtest_filter_unittest.py (right):

http://codereview.appspot.com/153042/diff/1067/2002#newcode62
test/gtest_filter_unittest.py:62: CAN_PASS_EMPTY_ENV = (child.exit_code == 0)
Reading the child's output is a more self-descriptive way of determining this
feature:

os.environ['EMPTY_VAR'] = ''
CAN_PASS_EMPTY_ENV_VAR_TO_CHILDREN = gtest_test_utils.Subprocess(
    [sys.executable, "-c",
     "import os; print 'EMPTY_VAR' in os.environ"]).output == 'True'

Same about CAN_UNSET_ENV

http://codereview.appspot.com/153042/diff/1067/2004
File test/gtest_shuffle_test.py (right):

http://codereview.appspot.com/153042/diff/1067/2004#newcode84
test/gtest_shuffle_test.py:84: original_env = environ.copy()
You don't need to modify and thn restore the original environment here. Just
modify the copy and then discard it.

http://codereview.appspot.com/153042/diff/1067/2006
File test/gtest_test_utils.py (right):

http://codereview.appspot.com/153042/diff/1067/2006#newcode235
test/gtest_test_utils.py:235: # Backup the environment variables
We need to guard for the case of env being NoneL

if env:
  old_environ = os.environ.copy()
  ...
else:
  old_environ = None

and also conditionally restore os.environ in the finally block.

http://codereview.appspot.com/153042/diff/1067/2006#newcode238
test/gtest_test_utils.py:238: os.environ.update(env)
To make an exact copy of an env dictionary, you need to clear the destination
first:

def _CopyEnvDict(dest, src):
  # Changes made by os.environ.clear are not inheritable by child process
  # until Python 2.6. To produce inheritable changes we have to delete
  # environment items with the del statement.
  for key in dest.keys():
    del dest[key]
  dest.update(src)

_CopyEnvDict(os.environ, env)

http://codereview.appspot.com/153042/diff/1067/2006#newcode259
test/gtest_test_utils.py:259: os.environ.update(old_environ)
You can reuse _CopyEnvDict here:

if old_environ:
  _CopyEnvDict(os.environ, old_environ)

[Hady, 2009-11-16 11:50:16]

Thanks!

Yeah, sure for the upcoming changes. I'll keep an eye out. Don't hesitate to
email me if you want me to perform a quick validation on Solaris/AIX. I'll
upload the (much simpler) AIX patches once we're done with this one.

Cheers,
Hady

http://codereview.appspot.com/153042/diff/1067/2002
File test/gtest_filter_unittest.py (right):

http://codereview.appspot.com/153042/diff/1067/2002#newcode62
test/gtest_filter_unittest.py:62: CAN_PASS_EMPTY_ENV = (child.exit_code == 0)
On 2009/11/16 02:01:28, Vlad wrote:
> Reading the child's output is a more self-descriptive way of determining this
> feature:
> 
> os.environ['EMPTY_VAR'] = ''
> CAN_PASS_EMPTY_ENV_VAR_TO_CHILDREN = gtest_test_utils.Subprocess(
>     [sys.executable, "-c",
>      "import os; print 'EMPTY_VAR' in os.environ"]).output == 'True'
> 
> Same about CAN_UNSET_ENV

Good idea. I also changed the implementation to 'eval' the child's output. That
way it'll be symmetrical with the print statement and throw when can't really
perform the check. It's in the comments.

Done.

http://codereview.appspot.com/153042/diff/1067/2004
File test/gtest_shuffle_test.py (right):

http://codereview.appspot.com/153042/diff/1067/2004#newcode84
test/gtest_shuffle_test.py:84: original_env = environ.copy()
On 2009/11/16 02:01:28, Vlad wrote:
> You don't need to modify and thn restore the original environment here. Just
> modify the copy and then discard it.

Done.

http://codereview.appspot.com/153042/diff/1067/2006
File test/gtest_test_utils.py (right):

http://codereview.appspot.com/153042/diff/1067/2006#newcode235
test/gtest_test_utils.py:235: # Backup the environment variables
On 2009/11/16 02:01:28, Vlad wrote:
> We need to guard for the case of env being NoneL
> 
> if env:
>   old_environ = os.environ.copy()
>   ...
> else:
>   old_environ = None
> 
> and also conditionally restore os.environ in the finally block.
You're absolutely right. I was 'updating' the current env when I had to
'replace' them to obtain the same semantics. I also realized that the Popen call
above doesn't change the environment if the 'env' parameter is 'None'. I updated
the patch to do just that and be more explicit.

http://codereview.appspot.com/153042/diff/1067/2006#newcode238
test/gtest_test_utils.py:238: os.environ.update(env)
On 2009/11/16 02:01:28, Vlad wrote:
> To make an exact copy of an env dictionary, you need to clear the destination
> first:
> 
> def _CopyEnvDict(dest, src):
>   # Changes made by os.environ.clear are not inheritable by child process
>   # until Python 2.6. To produce inheritable changes we have to delete
>   # environment items with the del statement.
>   for key in dest.keys():
>     del dest[key]
>   dest.update(src)
> 
> _CopyEnvDict(os.environ, env)

Done.

http://codereview.appspot.com/153042/diff/1067/2006#newcode259
test/gtest_test_utils.py:259: os.environ.update(old_environ)
On 2009/11/16 02:01:28, Vlad wrote:
> You can reuse _CopyEnvDict here:
> 
> if old_environ:
>   _CopyEnvDict(os.environ, old_environ)

Done.

[Vlad, 2009-11-16 15:51:06]

Hi Hady,

Just a couple of nits now. 

Do you plan to post AIX changes as a patch set in this issue?

Regards,
Vlad

http://codereview.appspot.com/153042/diff/2027/3004
File test/gtest_filter_unittest.py (right):

http://codereview.appspot.com/153042/diff/2027/3004#newcode54
test/gtest_filter_unittest.py:54: # We first set the env variable to an empty
string and call
This comment needs to be updated for the new the code. How about
'We set the env variable to an empty string and invoke a Python script in a
subprocess to analyze it"?

http://codereview.appspot.com/153042/diff/2027/3004#newcode60
test/gtest_filter_unittest.py:60: # is thrown if the input is neither 'True' nor
'False'.
Why would this happen? The "'EMPTY_VAR' in os.environ" expression evaluates to
either True or False.

[Hady, 2009-11-16 19:22:59]

Great!

I was actually going to submit the AIX patch in another issue once these were
in. That way I can give you a fresh patch and test out that trunk still works on
the other OSes.

Cheers,
had

http://codereview.appspot.com/153042/diff/2027/3004
File test/gtest_filter_unittest.py (right):

http://codereview.appspot.com/153042/diff/2027/3004#newcode54
test/gtest_filter_unittest.py:54: # We first set the env variable to an empty
string and call
On 2009/11/16 15:51:06, Vlad wrote:
> This comment needs to be updated for the new the code. How about
> 'We set the env variable to an empty string and invoke a Python script in a
> subprocess to analyze it"?

Done.

http://codereview.appspot.com/153042/diff/2027/3004#newcode60
test/gtest_filter_unittest.py:60: # is thrown if the input is neither 'True' nor
'False'.
On 2009/11/16 15:51:06, Vlad wrote:
> Why would this happen? The "'EMPTY_VAR' in os.environ" expression evaluates to
> either True or False.

I'm just being defensive. You know, misconfigured Python, Subprocess bug,
etc....just making sure that a misc. error is not mistaken for 'False'.

[Vlad, 2009-11-18 06:42:19]

Hi Hady,

The patch looks good now. I will touch it up myself to make it conform to our
coding style, to make things faster. It will then have to go through an internal
review (not unlike this one) from which it may emerge slightly changed. After
that it will be committed to svn.

Sounds good?

- Vlad

[Vlad, 2010-02-02 00:39:39]

Hady,

I've got another question on the patch.

In other news, the Python test script changes cause some tests to crash on some
of our Windows build machines, so we'll split the patch into two parts and
commit only the C++ one while we are investigating.

http://codereview.appspot.com/153042/diff/2034/3039
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/153042/diff/2034/3039#newcode1387
test/gtest_unittest.cc:1387: #if !defined(__SUNPRO_CC) || __SUNPRO_CC >= 0x590
Do you know which part of the code is rejected? What are the error messages? Is
there a way to fix the code to work with the Sun compiler?