Bug 174036 – [Xcode] webkit-patch should run sort-Xcode-project-file

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Don Olmstead]

When applicable it should be run to ensure changes are sorted properly.

[Stephan Szabo]

Created attachment 317274 [details]
Run sort-Xcode-project-file from some webkit-patch commands

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33732709>

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r220295 for reason:

This change introduced 4 errors in webkitpy tests.

Committed r220328: <http://trac.webkit.org/changeset/220328>

[Stephan Szabo]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

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.

[Don Olmstead]

ccing rniwa since he asked for this change.

[Andy Estes]

(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.

[Simon Fraser (smfr)]

Maybe we should have the style checker check that the projects are sorted?

[Ryosuke Niwa]

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.

[Blaze Burg]

(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.

[Matthew Hanson]

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.

[Andy Estes]

(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.

[Ryosuke Niwa]

(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.

[Simon Fraser (smfr)]

(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

[Andy Estes]

(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.

[Simon Fraser (smfr)]

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.

[Andy Estes]

(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.