Bug 74679 – Make it easier to add files to XYZ.xcodeproj/project.pbxproj with the commit-queue

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Benjamin Poulain]

When changing project.pbxproj, there is a high chance the file will conflicts with someone else when landing.

Each file is added in 4 sections: PBXBuildFile, PBXFileReference, the file's group and a build section (header or source).
The first 3 section are sorted. The build section is not sorted (probably because people do not know how to get there in XCode). Since files are added at the end of the build section, they always conflict.

I would like it to be easier to commit changes in the project files, and make it easier to merge the file between in branches.

Suggestions:
1) add a check to check-webkit-style to make sure all 4 sections are sorted alphabetically
2) add a script to sort the sections so we don't have to document how to do it in XCode
3) add a Git merge driver to solve the common problems of adding and removing files

Thought?

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> When changing project.pbxproj, there is a high chance the file will conflicts with someone else when landing.
> 
> Each file is added in 4 sections: PBXBuildFile, PBXFileReference, the file's group and a build section (header or source).
> The first 3 section are sorted. The build section is not sorted (probably because people do not know how to get there in XCode). Since files are added at the end of the build section, they always conflict.
> 
> I would like it to be easier to commit changes in the project files, and make it easier to merge the file between in branches.

This would also resolve cq? patches with Xcode project file changes that might otherwise fail to be applied.

> Suggestions:
> 1) add a check to check-webkit-style to make sure all 4 sections are sorted alphabetically

Sounds good.

> 2) add a script to sort the sections so we don't have to document how to do it in Xcode

Didn't I already write this, or does it miss a section?  :)

Tools/Scripts/sort-Xcode-project-file

> 3) add a Git merge driver to solve the common problems of adding and removing files

This might be tricky, but would definitely be worth the effort (for those of us that merge Xcode project file changes regularly :).

[Benjamin Poulain]

Created attachment 121212 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 121212 [details]
Patch

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

r=me, but consider altering the test to make it clear which section is causing the error.

This is a great step in the right direction.  All we need to do is to sort the Xcode project files for these checks to be effective!

> Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:76
> +        If the current match should not be after the previous match, issue and style error."""

Grammar:  "issue and style error."?  Maybe "issue a style error."?

> Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:133
> +                            # FIXME: Use the exact same rules as sort-Xcode-project-file.

Should probably file a bug for this and include it in the comment.

> Tools/Scripts/webkitpy/style/checkers/xcodeproj_unittest.py:122
> +        self.assert_error(['developmentRegion = English;',
> +                              'mainGroup = 00B9318013BA867F0035A948 /* WebKit */;',
> +                              '00B9318013BA867F0035A948 /* WebKit */ = {',
> +                              '    isa = PBXGroup;',
> +                              '    children = (',
> +                              '        00A629C013D0BEC70050AC52 /* MarkupTokenBase.h */',
> +                              '        00022E6813CE1BBA00282D5B /* CharacterReferenceParserInlineMethods.h */,',
> +                              '    );',
> +                              '}',
> +                              'children = (',
> +                              '00A629C013D0BEC70050AC52 /* MarkupTokenBase.h */',
> +                              '00022E6813CE1BBA00282D5B /* CharacterReferenceParserInlineMethods.h */,',
> +                              ');'], 'Resource files not sorted alphabetically: "CharacterReferenceParserInlineMethods.h" < "MarkupTokenBase.h".')

Nit: Might be less confusing to have different hashes/filenames in the outer 'children' section so that you know which one is causing the error.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> (From update of attachment 121212 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121212&action=review
> 
> r=me, but consider altering the test to make it clear which section is causing the error.

Also, I'd feel a bit more comfortable if someone with more experience writing checker patches also reviewed this, but it seems pretty safe on the surface if it passes the all of the existing checker tests.

[Benjamin Poulain]

(In reply to comment #4)
> Also, I'd feel a bit more comfortable if someone with more experience writing checker patches also reviewed this, but it seems pretty safe on the surface if it passes the all of the existing checker tests.

Eric maybe? Eric, would you mind having a look?

[Eric Seidel (no email)]

Comment on attachment 121212 [details]
Patch

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

I'm not sure what this does?  This is for validating XCode project files?  Is there some way we can have XCode do this for us?  Some sort of xcodebuild --lint-project option?

> Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:33
> +def find_main_group_hash(lines):

Free functions. :(

[Benjamin Poulain]

(In reply to comment #6)
> I'm not sure what this does?  This is for validating XCode project files?  Is there some way we can have XCode do this for us?  Some sort of xcodebuild --lint-project option?

The project files are valid, but the resource are unsorted because people forget to sort the files in the targets of  Xcode.

> > Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:33
> > +def find_main_group_hash(lines):
> 
> Free functions. :(

What is the problem with free functions? I kinda like them in Python since everything is scoped by the module. I wasn't aware they are a problem.

[Eric Seidel (no email)]

(In reply to comment #7)
> (In reply to comment #6)
> > I'm not sure what this does?  This is for validating XCode project files?  Is there some way we can have XCode do this for us?  Some sort of xcodebuild --lint-project option?
> 
> The project files are valid, but the resource are unsorted because people forget to sort the files in the targets of  Xcode.

Seems we should just make webkit-patch (or some other pre-submit hook) just auto-sort the file instead?  Why not correct the sort instead of warn about it?

> > > Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:33
> > > +def find_main_group_hash(lines):
> > 
> > Free functions. :(
> 
> What is the problem with free functions? I kinda like them in Python since everything is scoped by the module. I wasn't aware they are a problem.

They're OK, they're just harder to mock.  In general OO programming (which webkit generally subscribes to) discourages free functions.  Just making a sad face, that's all.

[Benjamin Poulain]

(In reply to comment #8)
> > The project files are valid, but the resource are unsorted because people forget to sort the files in the targets of  Xcode.
> 
> Seems we should just make webkit-patch (or some other pre-submit hook) just auto-sort the file instead?  Why not correct the sort instead of warn about it?

I think it would be good but it is an orthogonal issue. Not everyone uses webkit-patch, and the lack of sorting can be intentional (like having the generated files at the end of a group).

I'd like to land this, and try the auto-sorting through webkit-patch in a separate bug.

[David Levin]

Was there a webkit-dev thread about this?

Do people really want this enforced?

[Benjamin Poulain]

(In reply to comment #10)
> Was there a webkit-dev thread about this?
 
No, I did not think that was a big deal.

> Do people really want this enforced?

It will make the commit queue more effective and, in some cases, make our life a lot easier for the Apple port.

What are your concerns regarding this? I am all for having more scripts to simplify the handling of XCode project files.

[David Levin]

(In reply to comment #11)
> (In reply to comment #10)
> > Was there a webkit-dev thread about this?
> 
> No, I did not think that was a big deal.
> 
> > Do people really want this enforced?
> 
> It will make the commit queue more effective and, in some cases, make our life a lot easier for the Apple port.
> 
> What are your concerns regarding this? I am all for having more scripts to simplify the handling of XCode project files.

I just didn't know if people cared that much or would find it more of a pain to have the style checker flag this. I hope that everytime the style checker flags something, people think "I need to fix this." as opposed to "Oh there goes that noisy style checker. It is always saying stuff that no one cares about."

So I didn't know what category this fell in (which honestly I was reluctant to weigh in on this bug for a while.... but I had not articulated it). I know it is supposed to be sorted but it feels like it frequently isn't done by people and folks don't seem to care about it, so I wasn't sure about the value here. That's all :)

If you did this because you heard of a need, then that would be good evidence as well.  I was verbose but I hope that helps clarify what I was thinking.

[David Levin]

Comment on attachment 121212 [details]
Patch

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

fwiw, this checker calls turn_off_line_filtering() which is good because it means that errors will be flagged regardless of the lines that the person changed.

For example, if they added something out of order the item after what they added may be flagged but that would be filtered out normally because it hasn't changed. Since this class calls turn_off_line_filtering(), the error will be displayed.

The downside is that as soon as this is checked in, any existing errors in the xcodeproj files will be flagged whenever someone changes them, so it would be good to make sure that they are properly sorted when checking this in.

Just a few minor comments below. Otherwise it seems fine (espcially if folks want this enforced, which I don't have a good sense of -- see my other comments.)

> Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:4
> +# Copyright (C) 2011 Apple Inc. All rights reserved.

2012

> Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:82
> +            if (current_hash == previous_hash):

No need for parenthesis around the condition.

> Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:99
> +                if (file_section_start_regexp.match(line)):

No need for parenthesis around the condition.

> Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:121
> +                if (children_section_start_regexp.match(line)):

parens.

> Tools/Scripts/webkitpy/style/checkers/xcodeproj.py:140
> +                                pass

Why do a "pass" here as opposed to doing a ! on the condition?

if file_extension and previous_file_extension:
    self._check_resource_order(line_index, regexp_match, previous_match)

[Benjamin Poulain]

> If you did this because you heard of a need, then that would be good evidence as well.

We loose time on a daily basis because the project files almost never merge. I am not doing this patch for fun :)

> I was verbose but I hope that helps clarify what I was thinking.

I know it is already a pain to edit the XCode file and I don't want to make things worse.

I'll give a shot at sorting automatically through webkit-patch and land both patch together.

[Adam Barth]

Maybe we should just sort the project file automagically often?  That seems better than warning the user.

[Adam Barth]

Comment on attachment 121212 [details]
Patch

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

> Tools/Scripts/webkitpy/style/checkers/xcodeproj_unittest.py:77
> +                           ');'], 'Resource files not sorted alphabetically: "CharacterReferenceParserInlineMethods.h" < "MarkupTokenBase.h".')

Maybe these messages should say what to do?

"Resource files not sorted alphabetically. Please run Tools/Scripts/sort-xcodeproj."

[Benjamin Poulain]

Irrelevant now, iOS is upstream.