Adding Xcode project for Google Testing Framework

[Preston, 2008-07-19 00:49:23]

Hi everyone,

I just uploaded some code for review. It adds an Xcode project for building a
gtest.framework on Mac OS X (universal binary). Please have a look.

Thanks,
Preston

[Preston, 2008-07-21 20:06:20]

http://codereview.appspot.com/2595/diff/24/46
File xcode/Config/General.xcconfig (right):

http://codereview.appspot.com/2595/diff/24/46#newcode55
Line 55: GTEST_VERSIONINFO_SHORT = 0.0
Don't require manual editing of the version information in the Xcode project
file.

http://codereview.appspot.com/2595/diff/24/47
File xcode/Resources/Info.plist (right):

http://codereview.appspot.com/2595/diff/24/47#newcode1
Line 1: <?xml version="1.0" encoding="UTF-8"?>
#include the version information here.

[Preston, 2008-07-23 17:58:22]

Uploaded a new version of the Xcode project patch for a review. Please take a
look.

This patch

1. automatically extracts the version information from the configure.ac file.

2. fixes some build settings

3. fixes a typo in README

[Vlad, 2008-07-23 18:12:41]

Looks very good.

http://codereview.appspot.com/2595/diff/52/72
File trunk/README (right):

http://codereview.appspot.com/2595/diff/52/72#newcode155
Line 155: directory.
This needs a comment on how to select the build directory. Mentioning that it
defaults to projdir/build also doesn't hurt.

[Zhanyong, 2008-07-23 19:19:12]

Thanks for the hard work, Preston!

I don't really know how to make sense of the various project files, but I guess
it's OK as long as they work.

Chandler, could you take a look at the Python script for extracting the version
string and give Preston your feedback?

Thanks.

http://codereview.appspot.com/2595/diff/52/72
File trunk/README (right):

http://codereview.appspot.com/2595/diff/52/72#newcode49
Line 49: * Developer Tools Installed
A user can build using the autotools scripts without Xcode.  Can we remove this
or make it clear that Xcode is needed only if the user wants to use it?

http://codereview.appspot.com/2595/diff/52/72#newcode107
Line 107: ### Linux, Mac OS X, and Cygwin ###
Can you add " (without Xcode)" after "Mac OS X"?

http://codereview.appspot.com/2595/diff/52/76
File trunk/xcode/Config/DebugProject.xcconfig (right):

http://codereview.appspot.com/2595/diff/52/76#newcode4
Line 4: //  Licensed under the Apache License, Version 2.0 (the "License"); you
may not
We probably don't need this legal boiler plate for build scripts.  I asked
Daniel Berlin if we need to include this in the autotools scripts and he said
no.

Also, Google Test uses the new BSD license and we should be consistent.

http://codereview.appspot.com/2595/diff/52/74
File trunk/xcode/Config/FrameworkTarget.xcconfig (right):

http://codereview.appspot.com/2595/diff/52/74#newcode4
Line 4: //  Licensed under the Apache License, Version 2.0 (the "License"); you
may not
The same.

http://codereview.appspot.com/2595/diff/52/77
File trunk/xcode/Config/General.xcconfig (right):

http://codereview.appspot.com/2595/diff/52/77#newcode4
Line 4: //  Licensed under the Apache License, Version 2.0 (the "License"); you
may not
Ditto.

http://codereview.appspot.com/2595/diff/52/75
File trunk/xcode/Config/ReleaseProject.xcconfig (right):

http://codereview.appspot.com/2595/diff/52/75#newcode4
Line 4: //  Licensed under the Apache License, Version 2.0 (the "License"); you
may not
Ditto.

http://codereview.appspot.com/2595/diff/52/78
File trunk/xcode/Scripts/versiongenerate.py (right):

http://codereview.appspot.com/2595/diff/52/78#newcode5
Line 5: #  Licensed under the Apache License, Version 2.0 (the "License"); you
may not
This too.

http://codereview.appspot.com/2595/diff/52/78#newcode17
Line 17: #  versiongenerate.py
Could you make this comment block a doc string?

http://codereview.appspot.com/2595/diff/52/78#newcode72
Line 72: # Write the version information to a header file to be included in the
This generated file needs a header guard to prevent it from being included
twice.

http://codereview.appspot.com/2595/diff/52/78#newcode82
Line 82: #define GTEST_VERSIONINFO_SHORT %s.%s
Does this have to be a macro?  Can it instead be a const char* const? e.g.

const char* const kVersionInfoShort = "...";

[Preston, 2008-07-23 20:01:39]

Thanks for the feedback!

I've updated the patch taking into account suggestions by vlalosev and shiqian.

Please let me know if you have additional changes to suggest.

Preston

http://codereview.appspot.com/2595/diff/52/72
File trunk/README (right):

http://codereview.appspot.com/2595/diff/52/72#newcode49
Line 49: * Developer Tools Installed
There is no compiler on Mac OS X unless you install the Developer Tools (which
includes Xcode, and gcc). I wouldn't be sad to take this out, but I just wanted
to clarify why I put this in.

http://codereview.appspot.com/2595/diff/52/72#newcode107
Line 107: ### Linux, Mac OS X, and Cygwin ###
On 2008/07/23 19:19:13, Zhanyong wrote:
> Can you add " (without Xcode)" after "Mac OS X"?

Done.

http://codereview.appspot.com/2595/diff/52/72#newcode155
Line 155: directory.
On 2008/07/23 18:12:41, Vlad wrote:
> This needs a comment on how to select the build directory. Mentioning that it
> defaults to projdir/build also doesn't hurt.

Done.

http://codereview.appspot.com/2595/diff/52/78
File trunk/xcode/Scripts/versiongenerate.py (right):

http://codereview.appspot.com/2595/diff/52/78#newcode72
Line 72: # Write the version information to a header file to be included in the
This is a header file used only by the Info.plist file. The Info.plist file is
an xml-based property file that specifies framework properties like version
number, framework identifier, etc. You can tell Xcode to preprocess the
Info.plist with the C Preprocessor, which is nice for string substitution. 

It isn't used for building and is only used for string substitution in an XML
file. I can put the header guards in, but I believe it to be unnecessary.

http://codereview.appspot.com/2595/diff/52/78#newcode82
Line 82: #define GTEST_VERSIONINFO_SHORT %s.%s
These do need to be macros. As explained above, these are string substitutions
into an xml-based property file.

[chandlerc, 2008-07-23 20:01:53]

Several comments in line, however I have a more general concern. I'm sorry I
didn't mention it sooner, but I misunderstood the manner in which the version
information was being used.

Could you run the ./configure shell script rather than using python to process
the configure.ac file? One of the (many) things that shell script will do is
build a config.h ("$(SRCDIR)/config_aux/config.h" i think) which should have C
preprocessor definitions for the version string already embedded in it. I think
it might be much cleaner to extract the information at this level rather than
the other way around, but if the executing of the configure script is part of
the problem, then by all means proceed. =]

-Chandler

http://codereview.appspot.com/2595/diff/52/79
File trunk/xcode/Resources/Info.plist (right):

http://codereview.appspot.com/2595/diff/52/79#newcode20
Line 20: <string>GTEST_VERSIONINFO_LONG</string>
Is this the reason for macro's maybe?

http://codereview.appspot.com/2595/diff/52/78
File trunk/xcode/Scripts/versiongenerate.py (right):

http://codereview.appspot.com/2595/diff/52/78#newcode27
Line 27: #       of configure.ac
This one is safe.

http://codereview.appspot.com/2595/diff/52/78#newcode29
Line 29: #       parameter representing the version string
This one technically isn't but I think for this project it will be.

http://codereview.appspot.com/2595/diff/52/78#newcode32
Line 32: #       version and the third represents the fix version.
This one suffers from the same problem as above, but again, for our project it's
fine.

The issue is that I could define a variable "VERSION_STRING" and simply expand
it here. Dealing with that is sufficiently hard that it is absolutely not worth
it at this stage.

http://codereview.appspot.com/2595/diff/52/78#newcode34
Line 34: #       AC_INIT, including in comments and character strings.
This one is really unsafe, but again, at this time it holds, and hopefully we
can find a good CMake solution before it breaks.

Could you make a note that the assumptions are not related to Autoconf, and
specific to this project?

Finally, I would really like the meat of these assumptions to be mirrored in a
comment in the configure.ac file so that we remember to not break them. ;]

http://codereview.appspot.com/2595/diff/52/78#newcode48
Line 48: config_file = open('../configure.ac', 'r')
The path to the configure.ac file should either be provided to the script rather
than fixed within it, or documented as another (major) assumption.

http://codereview.appspot.com/2595/diff/52/78#newcode66
Line 66: version_expression = re.compile(r"(\d+)\.(\d+)\.(\d+)")
This can be done in a faster, and more "correct" way, in that it will assume
fewer things.

re.compile(r"^AC_INIT\(\_[^)]*?(\d+)\.(\d+)\.(\d+)")")

where "\_[^...]" is some variation of "[^...]" s.t. it will even match newlines
when non-newlines are specified in the "...". "\_[]" is the vim syntax, I'm not
sure what the Python syntax is.

This would eliminate the entire preceding processing, and I don't see immediate
dangers introduced by it.

http://codereview.appspot.com/2595/diff/52/78#newcode82
Line 82: #define GTEST_VERSIONINFO_SHORT %s.%s
On 2008/07/23 19:19:13, Zhanyong wrote:
> Does this have to be a macro?  Can it instead be a const char* const? e.g.
> 
> const char* const kVersionInfoShort = "...";

The very very bad reason is that I can't do this in the preprocessor:

cout << "Version: "GTEST_VERSIONINFO_LONG" of Google Test"

Personally, I'm with you, but I think the reason the original configure script
designs used macros was to leverage them in preprocessor magic.

Side note, if you go this route, it may need to be a static variable, so it can
be included and used in multiple files w/o name clashes.

http://codereview.appspot.com/2595/diff/52/78#newcode82
Line 82: #define GTEST_VERSIONINFO_SHORT %s.%s
This probably needs ""s if you leave it as a macro.

http://codereview.appspot.com/2595/diff/52/73
File trunk/xcode/gtest.xcodeproj/project.pbxproj (right):

http://codereview.appspot.com/2595/diff/52/73#newcode547
Line 547: "$(SRCROOT)/../configure.ac",
Here is the relative path to the configure.ac script that I'd like to see as a
parameter to the versiongenerate.py script.

http://codereview.appspot.com/2595/diff/52/73#newcode555
Line 555: shellScript = "/usr/bin/python Scripts/versiongenerate.py ../
$DERIVED_FILE_DIR\n";
Is this hard-coded path safe? I don't know enough about Xcode / Mac OS X Python
development to have the faintest idea.

[Preston, 2008-07-23 20:58:33]

Chandler,

Thanks for the useful feedback. I hesitate to use the ./configure script since
it seems to be a too heavy handed. I would prefer to parse the source of the
version string (whether that is the configure.ac, or a header file, or the svn
tag, or a build script, or whatever) than make Xcode depend on Autoconf. Also,
using ./configure would leave the source directory in a state that must be
manually cleaned if someone wanted to build out of it, since cleaning the Xcode
project won't call "make clean" in the source directory. For both of these
reasons, I would like to continue to use the included python script. However, I
will use ./configure if you insist.

The other changes have been made.

Preston

http://codereview.appspot.com/2595/diff/52/79
File trunk/xcode/Resources/Info.plist (right):

http://codereview.appspot.com/2595/diff/52/79#newcode20
Line 20: <string>GTEST_VERSIONINFO_LONG</string>
On 2008/07/23 20:01:53, chandlerc wrote:
> Is this the reason for macro's maybe?

Yep. This is it.

http://codereview.appspot.com/2595/diff/52/78
File trunk/xcode/Scripts/versiongenerate.py (right):

http://codereview.appspot.com/2595/diff/52/78#newcode5
Line 5: #  Licensed under the Apache License, Version 2.0 (the "License"); you
may not
On 2008/07/23 19:19:13, Zhanyong wrote:
> This too.

Done.

http://codereview.appspot.com/2595/diff/52/78#newcode17
Line 17: #  versiongenerate.py
On 2008/07/23 19:19:13, Zhanyong wrote:
> Could you make this comment block a doc string?

Done.

http://codereview.appspot.com/2595/diff/52/78#newcode34
Line 34: #       AC_INIT, including in comments and character strings.
On 2008/07/23 20:01:53, chandlerc wrote:
> This one is really unsafe, but again, at this time it holds, and hopefully we
> can find a good CMake solution before it breaks.
> 
> Could you make a note that the assumptions are not related to Autoconf, and
> specific to this project?
> 
> Finally, I would really like the meat of these assumptions to be mirrored in a
> comment in the configure.ac file so that we remember to not break them. ;]

Ok, I updated the comment and the configure.ac comment.

http://codereview.appspot.com/2595/diff/52/78#newcode48
Line 48: config_file = open('../configure.ac', 'r')
On 2008/07/23 20:01:53, chandlerc wrote:
> The path to the configure.ac file should either be provided to the script
rather
> than fixed within it, or documented as another (major) assumption.

You are right. As you can see, I'm already getting the "input_dir" from the
command line. I just forgot to update the path. Good catch.

http://codereview.appspot.com/2595/diff/52/78#newcode66
Line 66: version_expression = re.compile(r"(\d+)\.(\d+)\.(\d+)")
On 2008/07/23 20:01:53, chandlerc wrote:
> This can be done in a faster, and more "correct" way, in that it will assume
> fewer things.
> 
> re.compile(r"^AC_INIT\(\_[^)]*?(\d+)\.(\d+)\.(\d+)")")
> 
> where "\_[^...]" is some variation of "[^...]" s.t. it will even match
newlines
> when non-newlines are specified in the "...". "\_[]" is the vim syntax, I'm
not
> sure what the Python syntax is.
> 
> This would eliminate the entire preceding processing, and I don't see
immediate
> dangers introduced by it.

Ok. I'll do this. I was trying to separate the AC_INIT parameter processing from
the version interpretation, such that the former or latter could change without
affect the latter. Still, simpler is better.

http://codereview.appspot.com/2595/diff/52/78#newcode82
Line 82: #define GTEST_VERSIONINFO_SHORT %s.%s
On 2008/07/23 20:01:53, chandlerc wrote:
> This probably needs ""s if you leave it as a macro.

This is not used in C code, just for string substitutions in the xml-based
Info.plist. No worries about correct C syntax.

http://codereview.appspot.com/2595/diff/52/73
File trunk/xcode/gtest.xcodeproj/project.pbxproj (right):

http://codereview.appspot.com/2595/diff/52/73#newcode547
Line 547: "$(SRCROOT)/../configure.ac",
Sure, I can prepend $SRCROOT to the input parameter, though it is "understood"
that the script is executed from the $SRCROOT

http://codereview.appspot.com/2595/diff/52/73#newcode555
Line 555: shellScript = "/usr/bin/python Scripts/versiongenerate.py ../
$DERIVED_FILE_DIR\n";
In Xcode, scripts are executed in the directory containing the project files
(i.e. the SRCROOT). Making things run relative to this directory allow them to
be built on anyone's machine (no matter where they choose to put the gtest svn
repository. "Scripts" is a directory inside of xcode/ and should be safe.

[Vlad, 2008-07-23 21:52:08]

Looks Good To Me now!

[chandlerc, 2008-07-24 14:32:12]

LGTM, provided my question below is ok. The fix is trivial if it needs fixing.
Thanks for working on this.

Regarding ./configure, I think the most compelling point is the Autoconf
dependency. That seems sufficiently compelling to make this approach preferable.

http://codereview.appspot.com/2595/diff/129/120
File trunk/xcode/Scripts/versiongenerate.py (right):

http://codereview.appspot.com/2595/diff/129/120#newcode60
Line 60: // defines some version strings for the Info.plist
One final nit, could you mention here that this is not a C/C++ header file, and
doesn't need to conform to those standards? Just to prevent someone later from
coming through and decrying "Why on earth aren't there include guards!! And we
need quotes around those strings!!" I understand now, but it's sufficiently
non-obvious for non-xcode-savvy individuals to be worth noting.

http://codereview.appspot.com/2595/diff/129/115
File trunk/xcode/gtest.xcodeproj/project.pbxproj (right):

http://codereview.appspot.com/2595/diff/129/115#newcode555
Line 555: shellScript = "/usr/bin/python $SRCROOT/Scripts/versiongenerate.py
$SRCROOT/../ $DERIVED_FILE_DIR\n";
Just to clarify, my concern about the hard coded path was "/usr/bin/python" not
the scripts. =] like this, don't get me wrong, I just wanted to check that
/usr/bin/python is guaranteed by OS X

[Preston, 2008-07-24 15:06:39]

Updated changes.

1. Added a comment to the generated Version.h file reminding its readers that it
isn't a C-header

2. Simplified the build script, and added a comment about its execution
location.

http://codereview.appspot.com/2595/diff/129/120
File trunk/xcode/Scripts/versiongenerate.py (right):

http://codereview.appspot.com/2595/diff/129/120#newcode60
Line 60: // defines some version strings for the Info.plist
On 2008/07/24 14:32:12, chandlerc wrote:
> One final nit, could you mention here that this is not a C/C++ header file,
and
> doesn't need to conform to those standards? Just to prevent someone later from
> coming through and decrying "Why on earth aren't there include guards!! And we
> need quotes around those strings!!" I understand now, but it's sufficiently
> non-obvious for non-xcode-savvy individuals to be worth noting.

Done.

By the way, this file will be generated within the "build" directory and will be
cleaned when Xcode performs a project clean. The user will likely NEVER see this
file.

http://codereview.appspot.com/2595/diff/129/115
File trunk/xcode/gtest.xcodeproj/project.pbxproj (right):

http://codereview.appspot.com/2595/diff/129/115#newcode555
Line 555: shellScript = "/usr/bin/python $SRCROOT/Scripts/versiongenerate.py
$SRCROOT/../ $DERIVED_FILE_DIR\n";
Well..., python is installed in /usr/bin by default on Mac OS X. If you install
your own version, you'll have to either update the symbolic link (at
/usr/bin/python) or fix this script. I would expect developers savvy enough to
install their own python, to fix it. What do you think?

I'm simplifying these scripts back to what they were, but adding a comment
reminding the developer that it is executed from the SRCROOT.

[chandlerc, 2008-07-24 15:17:12]

LGTM! Thanks!!

http://codereview.appspot.com/2595/diff/129/120
File trunk/xcode/Scripts/versiongenerate.py (right):

http://codereview.appspot.com/2595/diff/129/120#newcode60
Line 60: // defines some version strings for the Info.plist
On 2008/07/24 15:06:40, Preston wrote:
> On 2008/07/24 14:32:12, chandlerc wrote:
> > One final nit, could you mention here that this is not a C/C++ header file,
> and
> > doesn't need to conform to those standards? Just to prevent someone later
from
> > coming through and decrying "Why on earth aren't there include guards!! And
we
> > need quotes around those strings!!" I understand now, but it's sufficiently
> > non-obvious for non-xcode-savvy individuals to be worth noting.
> 
> Done.
> 
> By the way, this file will be generated within the "build" directory and will
be
> cleaned when Xcode performs a project clean. The user will likely NEVER see
this
> file.

Yea, this was for the benefit of future editors of this python script, not for
the file itself. It's just convenient to locate them such that both benefit.

http://codereview.appspot.com/2595/diff/129/115
File trunk/xcode/gtest.xcodeproj/project.pbxproj (right):

http://codereview.appspot.com/2595/diff/129/115#newcode555
Line 555: shellScript = "/usr/bin/python $SRCROOT/Scripts/versiongenerate.py
$SRCROOT/../ $DERIVED_FILE_DIR\n";
On 2008/07/24 15:06:40, Preston wrote:
> Well..., python is installed in /usr/bin by default on Mac OS X. If you
install
> your own version, you'll have to either update the symbolic link (at
> /usr/bin/python) or fix this script. I would expect developers savvy enough to
> install their own python, to fix it. What do you think?

Hah! You'd think that, but then you'd meet developers like me who don't know the
mystical ways of Python. In all honesty, I've been bitten by having lots of
different versions of Python installed on OS X before, but I'm not concerned
about that here. It makes sense to write the script against the system-installed
python, and use it directly.

> I'm simplifying these scripts back to what they were, but adding a comment
> reminding the developer that it is executed from the SRCROOT. 

This is fine.