WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
74679
Make it easier to add files to XYZ.xcodeproj/project.pbxproj with the commit-queue
https://bugs.webkit.org/show_bug.cgi?id=74679
Summary
Make it easier to add files to XYZ.xcodeproj/project.pbxproj with the commit-...
Benjamin Poulain
Reported
2011-12-15 19:47:52 PST
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?
Attachments
Patch
(14.05 KB, patch)
2012-01-04 20:55 PST
,
Benjamin Poulain
ddkilzer
: review+
ddkilzer
: commit-queue-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
David Kilzer (:ddkilzer)
Comment 1
2011-12-15 21:39:00 PST
(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
Comment 2
2012-01-04 20:55:14 PST
Created
attachment 121212
[details]
Patch
David Kilzer (:ddkilzer)
Comment 3
2012-01-07 22:27:03 PST
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)
Comment 4
2012-01-07 22:28:40 PST
(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
Comment 5
2012-01-07 22:35:01 PST
(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 6
2012-01-07 23:14:10 PST
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
Comment 7
2012-01-07 23:27:40 PST
(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)
Comment 8
2012-01-07 23:38:36 PST
(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
Comment 9
2012-01-08 15:04:21 PST
(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
Comment 10
2012-01-08 18:52:26 PST
Was there a webkit-dev thread about this? Do people really want this enforced?
Benjamin Poulain
Comment 11
2012-01-08 18:58:20 PST
(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
Comment 12
2012-01-08 19:03:24 PST
(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 13
2012-01-08 19:17:00 PST
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
Comment 14
2012-01-08 19:20:41 PST
> 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
Comment 15
2012-01-08 22:17:44 PST
Maybe we should just sort the project file automagically often? That seems better than warning the user.
Adam Barth
Comment 16
2012-01-08 22:18:40 PST
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
Comment 17
2014-02-03 13:52:23 PST
Irrelevant now, iOS is upstream.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug