Bug 174036 - [Xcode] webkit-patch should run sort-Xcode-project-file
Summary: [Xcode] webkit-patch should run sort-Xcode-project-file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-30 13:58 PDT by Don Olmstead
Modified: 2017-08-15 13:34 PDT (History)
14 users (show)

See Also:


Attachments
Run sort-Xcode-project-file from some webkit-patch commands (7.82 KB, patch)
2017-08-04 13:24 PDT, Stephan Szabo
no flags Details | Formatted Diff | Diff
Run sort-Xcode-project-file from some webkit-patch commands w/test changes (10.11 KB, patch)
2017-08-07 11:25 PDT, Stephan Szabo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2017-06-30 13:58:39 PDT
When applicable it should be run to ensure changes are sorted properly.
Comment 1 Stephan Szabo 2017-08-04 13:24:05 PDT
Created attachment 317274 [details]
Run sort-Xcode-project-file from some webkit-patch commands
Comment 2 WebKit Commit Bot 2017-08-04 14:45:16 PDT
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>
Comment 3 WebKit Commit Bot 2017-08-04 14:45:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Radar WebKit Bug Importer 2017-08-04 14:47:08 PDT
<rdar://problem/33732709>
Comment 5 Ryan Haddad 2017-08-06 19:40:44 PDT
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
Comment 6 Ryan Haddad 2017-08-06 20:25:00 PDT
Reverted r220295 for reason:

This change introduced 4 errors in webkitpy tests.

Committed r220328: <http://trac.webkit.org/changeset/220328>
Comment 7 Stephan Szabo 2017-08-07 11:25:31 PDT
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 8 WebKit Commit Bot 2017-08-07 17:25:24 PDT
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>
Comment 9 WebKit Commit Bot 2017-08-07 17:25:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Andy Estes 2017-08-15 11:43:08 PDT
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.
Comment 11 Don Olmstead 2017-08-15 11:48:15 PDT
ccing rniwa since he asked for this change.
Comment 12 Andy Estes 2017-08-15 11:53:37 PDT
(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.
Comment 13 Simon Fraser (smfr) 2017-08-15 11:54:01 PDT
Maybe we should have the style checker check that the projects are sorted?
Comment 14 Ryosuke Niwa 2017-08-15 11:57:15 PDT
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.
Comment 15 Brian Burg 2017-08-15 12:00:27 PDT
(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.
Comment 16 Matthew Hanson 2017-08-15 12:05:12 PDT
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.
Comment 17 Andy Estes 2017-08-15 12:09:26 PDT
(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.
Comment 18 Ryosuke Niwa 2017-08-15 12:16:53 PDT
(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.
Comment 19 Simon Fraser (smfr) 2017-08-15 12:43:38 PDT
(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
Comment 20 Andy Estes 2017-08-15 13:24:38 PDT
(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.
Comment 21 Simon Fraser (smfr) 2017-08-15 13:30:43 PDT
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.
Comment 22 Andy Estes 2017-08-15 13:34:44 PDT
(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.