When applicable it should be run to ensure changes are sorted properly.
Created attachment 317274 [details] Run sort-Xcode-project-file from some webkit-patch commands
Comment on attachment 317274 [details] Run sort-Xcode-project-file from some webkit-patch commands Clearing flags on attachment: 317274 Committed r220295: <http://trac.webkit.org/changeset/220295>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33732709>
This change caused 4 webkitpy test errors: [1468/1638] webkitpy.tool.commands.download_unittest.DownloadCommandsTest.test_land_cowhand erred: Traceback (most recent call last): File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/tool/commands/download_unittest.py", line 182, in test_land_cowhand self.assert_execute_outputs(LandCowhand(), [50000], options=self._default_options(), expected_logs=expected_logs, tool=mock_tool) File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/tool/commands/commandtest.py", line 49, in assert_execute_outputs OutputCapture().assert_outputs(self, command.execute, [options, args, tool], expected_stdout=expected_stdout, expected_stderr=expected_stderr, expected_exception=expected_exception, expected_logs=expected_logs) File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/common/system/outputcapture.py", line 93, in assert_outputs return_value = function(*args, **kwargs) File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/tool/steps/sortxcodeprojectfiles.py", line 48, in run if self._options.sort_xcode_project: AttributeError: 'MockOptions' object has no attribute 'sort_xcode_project' https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/3410
Reverted r220295 for reason: This change introduced 4 errors in webkitpy tests. Committed r220328: <http://trac.webkit.org/changeset/220328>
Created attachment 317443 [details] Run sort-Xcode-project-file from some webkit-patch commands w/test changes Sorry, missed that the webkitpy tests for tool doesn't run by default on Win32 so didn't see the failures.
Comment on attachment 317443 [details] Run sort-Xcode-project-file from some webkit-patch commands w/test changes Clearing flags on attachment: 317443 Committed r220372: <http://trac.webkit.org/changeset/220372>
I don't think this was a good idea. Not everyone uses `webkit-patch`, so for those of us who do, our patches end up with lots of unrelated Xcode project file changes. I'd be fine with having an option to run sort-Xcode-project-file, but I object to this being done by default.
ccing rniwa since he asked for this change.
(In reply to Andy Estes from comment #10)> > I'd be fine with having an option to run sort-Xcode-project-file, but I > object to this being done by default. Reasons for my objection: 1. It makes patches harder to review when they have unrelated Xcode project file changes in them. 2. It makes patches harder to merge to branches (I've already experienced one merge conflict due to this feature). 3. Engineers tend to keep items in sorted order in the UI, which means that sort-Xcode-project-file is often just rearranging lists that aren't visible to the user. Having my patch include unrelated sorting changes to non-user-visible lists in .pbxproj files seems pretty low-value to me.
Maybe we should have the style checker check that the projects are sorted?
Yeah, adding checks to the style checker will prevent this regardless of whether you're using webkit-patch or not. In general, people shouldn't be adding out-of-order files in the first place.
(In reply to Ryosuke Niwa from comment #14) > Yeah, adding checks to the style checker will prevent this regardless of > whether you're using webkit-patch or not. > > In general, people shouldn't be adding out-of-order files in the first place. So what, we are supposed to manually order the raw XML? Which doesn't fit into most screens? I think style checker is fine, as long as it suggests how to run the script to fix the problem. And it should only run if the xcproj files are actually touched.
Perhaps we could server-side commit hook in combination with a tool that sorts the project files. That would require an initial sorting of all project files, but would allow us to guarantee the sorted-ness of project files going forward. The commit hook rejection message could include the location of the tool that will sort project files.
(In reply to Ryosuke Niwa from comment #14) > Yeah, adding checks to the style checker will prevent this regardless of > whether you're using webkit-patch or not. I'd be fine with that. > > In general, people shouldn't be adding out-of-order files in the first place. I agree, but you can add a file to the Xcode UI in correct sorted order and still have sections of the .pbxproj file that aren't sorted correctly.
(In reply to Andy Estes from comment #17) > (In reply to Ryosuke Niwa from comment #14) > > Yeah, adding checks to the style checker will prevent this regardless of > > whether you're using webkit-patch or not. > > I'd be fine with that. > > > > > In general, people shouldn't be adding out-of-order files in the first place. > > I agree, but you can add a file to the Xcode UI in correct sorted order and > still have sections of the .pbxproj file that aren't sorted correctly. In many instances, that ends up in the file shown in a wrong order somewhere else in UI so I'd argue that we should just always sort them in the right order for the sake of simplicity. Figuring out which entry does and doesn't affect UI is way too much work given the inhumane format of xcodeproj files.
(In reply to Brian Burg from comment #15) > (In reply to Ryosuke Niwa from comment #14) > > Yeah, adding checks to the style checker will prevent this regardless of > > whether you're using webkit-patch or not. > > > > In general, people shouldn't be adding out-of-order files in the first place. > > So what, we are supposed to manually order the raw XML? Which doesn't fit > into most screens? ./Tools/Scripts/sort-Xcode-project-file
(In reply to Matthew Hanson from comment #16) > Perhaps we could server-side commit hook in combination with a tool that > sorts the project files. That would require an initial sorting of all > project files, but would allow us to guarantee the sorted-ness of project > files going forward. > > The commit hook rejection message could include the location of the tool > that will sort project files. I actually like this idea even more than a style checker change. With the style checker, I'd still end up ignoring it if it complains about unrelated sorting issues. With a commit hook guaranteeing that projects in the repository are always sorted, I'll know that any issue is due to my patch and not some unrelated change.
I don't think you can have the style checker not warn, but the commit hook failing on an unsorted project. That results in annoyance when doing the commit, and failed commit queue commits.
(In reply to Simon Fraser (smfr) from comment #21) > I don't think you can have the style checker not warn, but the commit hook > failing on an unsorted project. That results in annoyance when doing the > commit, and failed commit queue commits. That's true. Let me rephrase: I think both a style checker warning and a commit hook is better than just a style checker warning.