Bug 61773 – Write a tools library to manipulate Xcode project files

• Component: Tools / Tests
• Status: RESOLVED LATER

[Roland Steiner]

The webkit-file tool project requires a module to manipulate XCode project files.

[Roland Steiner]

Created attachment 95410 [details]
patch, first version

[Roland Steiner]

(In reply to comment #1)

First version of the module for XCode project file handling. I'm far from a Python-wizard, so any suggestions how to improve the quality or performance of the code are highly welcome!

The initial version contains all that is necessary to read and parse a project file, add new files and project groups, and write out a project file. 'children' and 'files' lists within the project are implicitly sorted on writing.

Just reading and writing a project file already gives better results IMHO than Tools/Scripts/sort-XCode-project-file (better handling of files without extensions and variant groups).

[Eric Seidel (no email)]

Comment on attachment 95410 [details]
patch, first version

View in context: https://bugs.webkit.org/attachment.cgi?id=95410&action=review

Was some of this taken from xcodebodge? or did you write this all yourself?

> Tools/Scripts/webkitpy/common/project/__init__.py:4
> +__all__ = [
> +  'xcodeproj']

Why the strange wrap?  I don't remember what __all__ does. I don't think we've used it in any other __init__ files, but maybe we should?

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:45
> +    def __init__(self, msg):
> +        self._msg = msg

You could pass args and kwargs through, but I doubt it matters.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:51
> +class XCodeString(object):

Why our own separate string class?

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:54
> +    def __init__(self, strval=''):

If you're ever modifying self._string, than having a default value like this is bad, since the default is a global shared value.  Paranoid python programmers always use =None as a default value and then do if not arg: arg = "default value"

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:76
> +    def __init__(self, uuidstr='000000000000000000000000'):

Same problem as above with default values.  Here especailly would make sense to define a cls._zero_string or cls.zero.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:119
> +            if uuid != XCodeUUID('000000000000000000000000') and uuid != XCodeUUID('FFFFFFFFFFFFFFFFFFFFFFFF'):

I might have used '0' * 24 or 'F' * 24 to make it easier to realize I hadn't missed one.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:147
> +        self._uuid2buildphaseUUID = {}
> +        self._buildConfList2UUID = {}
> +        self._uuid2commentCache = {}
> +        self._mainGroupUUID = XCodeUUID()
> +        self._mainProjectNativeTarget = None
> +        self._mainProjectSourcesBuildPhaseUUID = XCodeUUID()
> +        self._mainProjectHeadersBuildPhaseUUID = XCodeUUID()
> +        self._mainProjectResourcesBuildPhaseUUID = XCodeUUID()
> +        self._mainProjectFrameworksBuildPhaseUUID = XCodeUUID()

I think you're used to google-style python naming here.  We try to follow PEP8 (with the exception of the 80c rule) in WebKit.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:814
> +            self._file.write('"')
> +            self._file.write(str(value))
> +            self._file.write('"')

Why 3 lines?

[Eric Seidel (no email)]

Comment on attachment 95410 [details]
patch, first version

Many unanswered questions in the previou review, so marking r-.  If this does a better job than sort-XCode-project-file, seems it might make sense to re-write sort-XCode-project-file to use it in some future patch.

[Roland Steiner]

(In reply to comment #3)

Thanks for the great feedback!

> Was some of this taken from xcodebodge? or did you write this all yourself?

All written by myself (I didn't even know of xcodebodge unil now... :P)
 
> > Tools/Scripts/webkitpy/common/project/__init__.py:4
> > +__all__ = [
> > +  'xcodeproj']
> 
> Why the strange wrap?  I don't remember what __all__ does. I don't think we've used it in any other __init__ files, but maybe we should?

It's so one can use 'from webkitpy.common.project import *' that I foresee being useful for the final tool that adds files to all different project files. The wrap is because there will be more files added over time.

> > Tools/Scripts/webkitpy/common/project/xcodeproj.py:51
> > +class XCodeString(object):
> 
> Why our own separate string class?

XCodeString and XCodeUUID are used to remember the type of the original values and distinguish them from IDs, paths and integers (all simply stored as string). When writing back the project file, UUIDs need additional comments, and XCodeString adds the enclosing quotes, except if used within a comment. OTOH, integers, paths and IDs are just written as-is.

(I should document this in the source as well.)

> I think you're used to google-style python naming here.  We try to follow PEP8 (with the exception of the 80c rule) in WebKit.

Will change.

[Adam Barth]

> It's so one can use 'from webkitpy.common.project import *' that I foresee being useful for the final tool that adds files to all different project files. The wrap is because there will be more files added over time.

That's discouraged by PEP8 and we don't allow it outside of unit tests.

[Roland Steiner]

Created attachment 95553 [details]
new patch

Hopefully with improved PEP8 compliance :)

[Adam Barth]

Comment on attachment 95553 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95553&action=review

This is a lot of code, and I've very happy you're doing this work.  The big thing missing from this patch (aside from the nits below) is tests!  We greatly prefer this code to be tested using Python unit tests so we can make changes to it without breaking things.  You can see lot of examples of other tests in webkitpy in the files named _unittest.py.  If you write a unit test in a file with such a name, it will be automagically discovered and run as part of the test-webkitpy testing harness.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:42
> +    """Class used for exceptions."""

This docstring is meaningless.  Of course its a class used for exceptions.  It's a subclass of Exception.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:45
> +    def __init__(self, msg):
> +        self._msg = msg

Sorry to keep poking you with style nits, but we prefer variables to be full english works, like we do in regular WebKit code, so this should be _message.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:58
> +    def __init__(self, strval):

strval => string_value or just string

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:274
> +        # could not resolve path, return non-empty sub-path together with current group

Also like regular WebKit code, we prefer comments to be English sentences with initial caps and a trailing period.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:618
> +    def parse_project_file(self, filePath, project_name=''):

filePath => file_path

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:627
> +        with codecs.open(filePath, 'r', 'utf8', 'replace') as self._file:
> +            self._content = self._file.read()

Please use the filesystem abstraction for interacting with the file system.  (Also, we require compatibility with Python 2.5, so you can't use "with" without importing it from the future.)

[Eric Seidel (no email)]

Comment on attachment 95553 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95553&action=review

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:33
> +from __future__ import with_statement

He did import with. :)

[Adam Barth]

Ah.  I fail at reading.  :)

[Adam Barth]

This whole approach is super exciting to me.  I've hoped we'd do this for a long time, but I was scared of trying to write code that interacts with the xcodeproj files.

[Tony Chang]

(In reply to comment #11)
> This whole approach is super exciting to me.  I've hoped we'd do this for a long time, but I was scared of trying to write code that interacts with the xcodeproj files.

FWIW, we used to have a similar python script in Chromium (I think jrg wrote it):
  http://codereview.chromium.org/2167003

[Mark Mentovai]

aharper wrote xcodebodge.

[Mark Rowe (bdash)]

Comment on attachment 95553 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95553&action=review

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:31
> +# This is a very simple script designed to crawl the build directory
> +# for visual studio express build logs and print them to stdout.

…

[Mark Rowe (bdash)]

One general comment:  It’s Xcode, not “XCode”.  There are numerous instances of this misspelling in the patch.

[Mark Rowe (bdash)]

Comment on attachment 95553 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95553&action=review

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:78
> +class XCodeUUID(object):
> +    """Class containing a single XCode UUID.

It’s a bit misleading to refer to this as a UUID.  They’re unique identifiers, but they do not take the form of a UUID (24 hex characters vs the usual 32).

[Roland Steiner]

(In reply to comment #12)
> FWIW, we used to have a similar python script in Chromium (I think jrg wrote it):
>   http://codereview.chromium.org/2167003

Eric also mentioned it. Why was it removed?

[Mark Mentovai]

(In reply to comment #17)
> (In reply to comment #12)
> > FWIW, we used to have a similar python script in Chromium (I think jrg wrote it):
> >   http://codereview.chromium.org/2167003
> 
> Eric also mentioned it. Why was it removed?

The changelist description in the link you quoted explains it succinctly, but to expand…

We removed xcodebodge when we moved to GYP. We no longer modify .xcodeprojs directly, so xcodebodge was effectively obsolete for our purposes. Since it was only intended as a quick-and-dirty stopgap to begin with, we didn’t consider it worth maintaining in our tree, and removed it.

When we did this, we were fully aware that if anyone ever wanted to resurrect it, they could just pull it out of our version control history.

Here’s the history: http://src.chromium.org/viewvc/chrome/trunk/src/tools/xcodebodge/xcodebodge.py?view=log&pathrev=48277

[Roland Steiner]

Created attachment 100072 [details]
work-in-progress

[Roland Steiner]

Work-in-progress update:

-) stole a few ideas from webkitbodge.py (more to be stolen later)
-) factored out common interfaces into base classes (see bug 64148)
-) added a unittest stub file

[Roland Steiner]

Created attachment 132042 [details]
work-in-progress

expanded and refactored work-in-progress. The idea being to make the system flexible enough to do all kinds of file manipulations.

[Roland Steiner]

Created attachment 134771 [details]
patch, requires 82595

first review version. Unit tests are subsumed by unit tests for webkit-file.py (see bug 81317)

[Anders Carlsson]

Comment on attachment 134771 [details]
patch, requires 82595

Nobody is working on this and it's cluttering up the Bugzilla review queue so I'm going to go ahead and close this.