The webkit-file tool project requires a module to manipulate XCode project files.
Created attachment 95410 [details] patch, first version
(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).
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?
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.
(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.
> 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.
Created attachment 95553 [details] new patch Hopefully with improved PEP8 compliance :)
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.)
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. :)
Ah. I fail at reading. :)
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.
(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
aharper wrote xcodebodge.
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. …
One general comment: It’s Xcode, not “XCode”. There are numerous instances of this misspelling in the patch.
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).
(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?
(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
Created attachment 100072 [details] work-in-progress
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
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.
Created attachment 134771 [details] patch, requires 82595 first review version. Unit tests are subsumed by unit tests for webkit-file.py (see bug 81317)
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.