Bug 197622 – Rewrite generate-xcfilelists in Python

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Keith Rollin]

For a number of reasons, rewrite generate-xcfilelists in Python:

* The previous bash script was large and unmaintainable.
* We have more internal expertise with Python than with bash.
* The bash script used temporary files in /tmp that got stranded and were owned by root, making them awkward to clear out. <rdar://problem/49490262>
* We needed to complete support for engineers that built with Xcode rather than the command line.
* There are some bugs leading to mildly corrupted .xcfilelist files. It’s not apparent the source of the bugs, but one general reason might be due to approaches born of using bash. Rewriting in Python may clear these up.

[Radar WebKit Bug Importer]

<rdar://problem/50508222>

[Keith Rollin]

Created attachment 369193 [details]
Patch

[EWS Watchlist]

Attachment 369193 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:49:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:88:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:89:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:90:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:109:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:110:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:111:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:163:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:164:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:165:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:270:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:271:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:66:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:164:  whitespace before '}'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:592:  too many blank lines (2)  [pep8/E303] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:630:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:646:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:662:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:682:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:694:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:215:  [BaseGenerator.is_valid] Class 'BaseGenerator' has no 'VALID_PLATFORMS' member  [pylint/E1101] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:216:  [BaseGenerator.is_valid] Class 'BaseGenerator' has no 'VALID_CONFIGURATIONS' member  [pylint/E1101] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:227:  [BaseGenerator.report_error] Raising NoneType while only classes, instances or string are allowed  [pylint/E0702] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:493:  [BaseGenerator._get_build_dirs.define_xcode_build_dirs] Undefined variable 'os_path'  [pylint/E0602] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:58:  expected 1 blank line, found 0  [pep8/E301] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:62:  expected 1 blank line, found 0  [pep8/E301] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:168:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:189:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:196:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:212:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:215:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:218:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:221:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:230:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:232:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:233:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:234:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:235:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:263:  [hash_string_for_path] Instance of 'md5' has no 'digest' member  [pylint/E1101] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/sdk.py:37:  expected 2 blank lines, found 1  [pep8/E302] [5]
Total errors found: 40 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 369198 [details]
Address style issues. The remaining two are bogus.

[EWS Watchlist]

Attachment 369198 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:227:  [BaseGenerator.report_error] Raising NoneType while only classes, instances or string are allowed  [pylint/E0702] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:273:  [hash_string_for_path] Instance of 'md5' has no 'digest' member  [pylint/E1101] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 369198 [details]
Address style issues. The remaining two are bogus.

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

Nothing jumps out to me as problematic.  I definitely like that it's not bash.  It wouldn't hurt to have someone who works more on our python tools look it over, but r=me

> Tools/ChangeLog:9
> +        For a number of reasons, rewrite generate-xcfilelists in Python:

No perl?

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:78
> +            # ("",             "iosmac"),

Why are these commented?

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:261
> +# The algorithm is adopted from the following article:

adapted?

[Keith Rollin]

Created attachment 369208 [details]
Patch

[Keith Rollin]

Fixed the nits.

(In reply to Alex Christensen from comment #6)
> > Tools/ChangeLog:9
> > +        For a number of reasons, rewrite generate-xcfilelists in Python:
> 
> No perl?

Hey, I wanted to do it in Ruby. But I was told: Perl or Python. So I picked Python. I'm not at all a fan of Perl. Which has led to my Perl-fu being very rusty.

[EWS Watchlist]

Attachment 369208 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:227:  [BaseGenerator.report_error] Raising NoneType while only classes, instances or string are allowed  [pylint/E0702] [5]
ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:273:  [hash_string_for_path] Instance of 'md5' has no 'digest' member  [pylint/E1101] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 369208 [details]
Patch

Attachment 369208 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12120522

New failing tests:
http/tests/css/filters-on-iframes.html

[EWS Watchlist]

Created attachment 369249 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Jonathan Bedard]

Comment on attachment 369208 [details]
Patch

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

Talking to Keith, some of my comments, particularly on the subprocess bits, may be incorrect because of Python 3 compatibility (which the rest of webkitpy doesn't have)

> Tools/Scripts/generate-xcfilelists:56
> +# Relaunch ourselves if the Internal version of generator-xcfilelists exists.

We don't traditionally do this with scripts living in both Internal and OpenSource. Usually, we'll do it the other way around. (ie, OpenSource is stand alone, Internal calls OpenSource)

This is probably a good candidate for apple_additions, webkitpy/port/ios_simulator.py has some examples of this.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:55
> +    __slots__ = (

I don't see this in our existing Python code (at least, outside of third party libraries). Is this code actually hot enough to justify using this optimization? I suspect not.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:68
> +    # provided here.

Couldn't we just standardize the capitalization? (ie something like, aliases.get(platform.lower(), platform.lower()))

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:90
> +        self.dispatch = {

Quite surprised that the style-check allowed this. Usually, we do 4 spaces and return to the original indent on the final bracket, like this:

self.dispatch = {
    'generate': self._cmd_generate,
    ...
}

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:173
> +  generate              Generate a complete and up-to-date set of .xcfilelist

You shouldn't need all of this in the description. If you use the help argument when adding arguments, you will automagically get all of this.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:221
> +                        [Internal use only] Name of the file used to store

Another candidate for appleadditions

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:241
> +    def _validate_args(self, args):

Usually, when using ArgumentParser, we try to add arguments such that we don't have to do another pass on the args. ArgumentParser is actually super powerful, you can define your own action types. As an example, here is a small action type I defined for supporting --no:

class NoAction(argparse.Action):
    def __init__(self, option_strings, dest, **kwargs):
        super(NoAction, self).__init__(option_strings, dest, nargs=0, **kwargs)

    def __call__(self, parser, namespace, values, option_string):
        setattr(namespace, self.dest, False if option_string.startswith('--no') else True)

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:416
> +        return self.command_file

Bit unclear why we have the accessor here. Any reason we can't just call the command_file directly?

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:116
> +                    self.application._get_generate_xcfilelists_script_path(),

Generally, we try and avoid using _ functions outside of the contained class. You can't enforce it in python, by we basically using _ as a way of indicating that something is what C++ would call a private or protected member.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/sdk.py:38
> +class SDK(object):

This class belongs in webkitpy/xcode. Also needs unit tests.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/sdk.py:60
> +    # @util.LogEntryExitClass

From debugging?

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/sdk.py:109
> +    # @util.LogEntryExitClass

From debugging?

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:46
> +if SHOW_DEBUG_LOGGING:

This should probably being using done using the logging module. With that module, you could unconditionally debug log all of this and then the caller would be responsible for setting the logging level. That's exactly what things like run-webkit-tests do. <https://docs.python.org/2/library/logging.html> probably has way more information than you need on the subject.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:77
> +    class LogEntryHelper(object):

This is an interesting idea (as are all of the logging Context managers you've written here). I could go either way, but this seems like something that perhaps belongs somewhere in webkitpy/common...looking through existing files, perhaps webkitpy/common/system/stack_utils.py is a candidate?

These all deserve some basic unit testing, take a look at how we use the OutputCapture class to testing logging like this.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:172
> +def subprocess_run(args, **kwargs):

I think that this should probably be removed in favor of the Executive class form webkitpy/common/system/executive.py.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:195
> +    return os.environ.get("XCODE_INSTALL_PATH")

I wonder if anything in the PlatformInfo is useful here.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:201
> +class AttributeSaver:

This is interesting and of more general utility, probably belongs in webkitpy/common. Definitely deserves a unit test as well.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:240
> +class CalledProcessError(Exception):

Seems like a reimplementation of ScriptError in webkitpy/common/system/executive.py.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:265
> +def hash_string_for_path(path):

This feels like it belongs somewhere in webkitpy/xcode, and probably deserves a better name (perhaps something like xcode_hash_for_path?)

[Keith Rollin]

(In reply to Jonathan Bedard from comment #12)
> Comment on attachment 369208 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369208&action=review
> 
> Talking to Keith, some of my comments, particularly on the subprocess bits,
> may be incorrect because of Python 3 compatibility (which the rest of
> webkitpy doesn't have)
> 
> > Tools/Scripts/generate-xcfilelists:56
> > +# Relaunch ourselves if the Internal version of generator-xcfilelists exists.
> 
> We don't traditionally do this with scripts living in both Internal and
> OpenSource.

Talked with Jonathan about this, and I'll be taking the pre-existing approach.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:55
> > +    __slots__ = (
> 
> I don't see this in our existing Python code (at least, outside of third
> party libraries). Is this code actually hot enough to justify using this
> optimization? I suspect not.

No, not hot. Just habit. I don't think there's any reason to not do it.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:68
> > +    # provided here.
> 
> Couldn't we just standardize the capitalization? (ie something like,
> aliases.get(platform.lower(), platform.lower()))

I could take a look at this again. There was a reason why I didn't take that approach -- something to do with needing to preserve the strings as provided by the user. But I don't remember the exact details now. I'll either recover that reason, or use the obvious alternative.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:90
> > +        self.dispatch = {
> 
> Quite surprised that the style-check allowed this. Usually, we do 4 spaces
> and return to the original indent on the final bracket, like this:
> 
> self.dispatch = {
>     'generate': self._cmd_generate,
>     ...
> }

I just use what vim gives me, and then adjust according to check-webkit-style.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:173
> > +  generate              Generate a complete and up-to-date set of .xcfilelist
> 
> You shouldn't need all of this in the description. If you use the help
> argument when adding arguments, you will automagically get all of this.

The problem is that Pythons argparse facility doesn't describe these commands if you just say `generate-xcfilelists --help`. You need to query each command individually with `generate-xcfilelists <some-command> --help`. I wanted something better than that.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:221
> > +                        [Internal use only] Name of the file used to store
> 
> Another candidate for appleadditions

Not in this case. Perhaps I should reword. I'm talking about "internal to the operation of the script".

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:241
> > +    def _validate_args(self, args):
> 
> Usually, when using ArgumentParser, we try to add arguments such that we
> don't have to do another pass on the args. ArgumentParser is actually super
> powerful, you can define your own action types. 

I'll see if I can make that work. Right now, I don't have experience with argparse Actions, and your example doesn't seem to fit with what I need to do.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:416
> > +        return self.command_file
> 
> Bit unclear why we have the accessor here. Any reason we can't just call the
> command_file directly?

This can change with the refactoring we talked about. But, for the current implementation, the accessor is so that we know if we're talking about the version of generate-xcfilists in OpenSource or in Safari.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:116
> > +                    self.application._get_generate_xcfilelists_script_path(),
> 
> Generally, we try and avoid using _ functions outside of the contained
> class. You can't enforce it in python, by we basically using _ as a way of
> indicating that something is what C++ would call a private or protected
> member.

Good point. That was fallout from a refactoring, and -- being used to the existing names -- I didn't rename them.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/sdk.py:38
> > +class SDK(object):
> 
> This class belongs in webkitpy/xcode. Also needs unit tests.

OK.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/sdk.py:60
> > +    # @util.LogEntryExitClass
> 
> From debugging?
> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/sdk.py:109
> > +    # @util.LogEntryExitClass
> 
> From debugging?

I wanted to use LogEntryExitClass, but had problems getting it working correctly, and so commented it out until it turned out I really needed it and so needed to get it working.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:46
> > +if SHOW_DEBUG_LOGGING:
> 
> This should probably being using done using the logging module. With that
> module, you could unconditionally debug log all of this and then the caller
> would be responsible for setting the logging level. That's exactly what
> things like run-webkit-tests do.
> <https://docs.python.org/2/library/logging.html> probably has way more
> information than you need on the subject.

Jonathan and I talked about this. One reason for my custom logging was to capture full detail on function calls, including incoming parameters, outgoing results, and whether the function exited normally or not. I also need to ferry the logging information between the "outer" instance of the script and the "inner" instance of the script running inside of Xcode.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:77
> > +    class LogEntryHelper(object):
> 
> This is an interesting idea (as are all of the logging Context managers
> you've written here). I could go either way, but this seems like something
> that perhaps belongs somewhere in webkitpy/common...looking through existing
> files, perhaps webkitpy/common/system/stack_utils.py is a candidate?
> 
> These all deserve some basic unit testing, take a look at how we use the
> OutputCapture class to testing logging like this.

OK.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:172
> > +def subprocess_run(args, **kwargs):
> 
> I think that this should probably be removed in favor of the Executive class
> form webkitpy/common/system/executive.py.

Executive doesn't give what I necessarily need, such as Python 3 compatibility, stdout, and possibly exceptions (not sure about this).

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:195
> > +    return os.environ.get("XCODE_INSTALL_PATH")
> 
> I wonder if anything in the PlatformInfo is useful here.

I don't see what I need in there, which is a way to tell if the script was invoked from within Xcode.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:201
> > +class AttributeSaver:
> 
> This is interesting and of more general utility, probably belongs in
> webkitpy/common. Definitely deserves a unit test as well.

OK.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:240
> > +class CalledProcessError(Exception):
> 
> Seems like a reimplementation of ScriptError in
> webkitpy/common/system/executive.py.

I think ScriptError suffers from the same problem that subprocess.CalledProcessError has and which led me to create my own CalledProcessError, which is that it can't be pickled due to it's custom attributes.

> 
> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:265
> > +def hash_string_for_path(path):
> 
> This feels like it belongs somewhere in webkitpy/xcode, and probably
> deserves a better name (perhaps something like xcode_hash_for_path?)

OK/

[Keith Rollin]

Created attachment 369432 [details]
Patch

[EWS Watchlist]

Attachment 369432 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:230:  [BaseGenerator.report_error] Raising NoneType while only classes, instances or string are allowed  [pylint/E0702] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:37:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:37:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:38:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:38:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:41:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:41:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:44:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:44:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:45:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:45:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:46:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:46:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:47:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:47:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:50:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:50:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:51:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:51:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:54:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:54:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:57:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:57:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:58:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:58:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:61:  indentation contains tabs  [pep8/W191] [5]
ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:61:  indentation contains mixed spaces and tabs  [pep8/E101] [5]
ERROR: Tools/Scripts/webkitpy/xcode/__init__.py:25:  [xcode_hash_for_path] Instance of 'md5' has no 'digest' member  [pylint/E1101] [5]
Total errors found: 28 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

The first and last warnings are the same as before. The bits about the spaces/tabs are OK, since they concern a long string that's used only for testing purposes and is intended to mimic the output of `xcodebuild -showsdks`.

[Jonathan Bedard]

Comment on attachment 369432 [details]
Patch

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

I still need to spend some time with the BaseGenerator to really understand what's going on there, but I want to at least post my comments on the majority of the patch.

> Tools/Scripts/webkitpy/common/attribute_saver_unittest.py:79
> +        self.assertEquals(obj.value, 0)

Not sure what we're testing here. If line 77 was ever run, we would raise an exception and fail the test.

> Tools/Scripts/webkitpy/common/attribute_saver_unittest.py:91
> +            self.assertEquals(obj.value, 0)

This line of code will never be run.

> Tools/Scripts/webkitpy/common/attribute_saver_unittest.py:93
> +    def test_normal_value_with_finally_on_normal_exit(self):

Not clear what this is actually testing....if line 101 wasn't run, that would mean that we threw an exception and we'd fail the test.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:65
> +    platform_aliases = {

Style nit: we usually do a 4 space indent and the final bracket is not indented.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:94
> +        self.project_specific_generators = {

This feels like this should be a class variable, not an instance variable.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:146
> +    # sub-class's __init__.

This is not very intuitive for a C++ programer, but, in Python, you can do something like this:

class Base(class):
    def __init__(self):
        self.consume(self.foo)

class Child(Base):
    def __init__(self):
        self.foo = 'bar'
        super(Child, self).__init__()

Which might be better than this _initialize() method. I don't feel super strongly about this, though, because you have already marked this method with an '_'

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:183
> +        parser.add_argument("command", action=util.CheckCommandAction,

Not something you need to change, but just some food for thought:

Python argparse can create sub parsers, which is usually how I would recommend doing this sort of thing. Surprisingly, we don't use sub parsers in webkitpy at the moment, but that might be the right way to go. You can also set options so that they only belong to a single sub-command. <https://docs.python.org/2/library/argparse.html>

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:232
> +                        [Used internally] Name of the file used to store

Before, you mentioned that this 'Used internally' should really be 'used by script internals'. Given that in WebKit infrastructure, 'Used internally' mean something completely different, we probably shouldn't use that.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:399
> +    def _get_root_dir(self):

I think this should be 'get_root_parent' since we usually use the root to refer to the root of the repository.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:412
> +    # Return the path to the directory containing supporting build scripts.

Comments like this shouldn't be needed, the function name is descriptive enough.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:625
> +    def _get_generate_derived_sources_script(self):

It feels weird that when this is defined, it's always the same. Can it ever be something other than os.path.join(self._get_project_dir(), "Scripts", "generate-derived-sources.sh")?

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:629
> +    def _get_generate_unified_sources_script(self):

It feels weird that when this is defined, it's always the same. Can it ever be something other than os.path.join(self._get_project_dir(), "Scripts", "generate-unified-sources.sh")?

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:1
> +#!/usr/bin/env python

I'm ok with this landing here as is, but I do think that we should move it in the near future to a more generally useful location.

[Keith Rollin]

(In reply to Jonathan Bedard from comment #17)
> I still need to spend some time with the BaseGenerator to really understand
> what's going on there, but I want to at least post my comments on the
> majority of the patch.
> 
> > Tools/Scripts/webkitpy/common/attribute_saver_unittest.py:79
> > +        self.assertEquals(obj.value, 0)
> 
> Not sure what we're testing here. If line 77 was ever run, we would raise an
> exception and fail the test.

I was rather indiscriminate with asserts and threw them all over the place, and left those in place that were inherited from other functions that I copy/pasted. I'm not sure if you're saying you want them taken out or not.

> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:65
> > +    platform_aliases = {
> 
> Style nit: we usually do a 4 space indent and the final bracket is not
> indented.

I just went with what vim gave me and check-webkit-style allowed. I could change this instance, but check-webkit-style should really be updated to catch "gotchas" like this, since there's no place (that I know of) where this is style is documented.

> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:94
> > +        self.project_specific_generators = {
> 
> This feels like this should be a class variable, not an instance variable.

It gets updated in InternalApplication, and so needs to be writeable. I'm not sure you can do that with class variables.

> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:146
> > +    # sub-class's __init__.
> 
> This is not very intuitive for a C++ programer, but, in Python, you can do
> something like this:

Good point. Yes, it's counter-intuitive to a C++ programmer. I think there is one place in some other section where I got this approach correct (checking...class CheckValidItemAction). I can change that here, too, since that makes this class more idiomatic.

> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:183
> > +        parser.add_argument("command", action=util.CheckCommandAction,
> 
> Not something you need to change, but just some food for thought:
> 
> Python argparse can create sub parsers,

I'm familiar with sub-parsers, and originally used them here (along with shared parsers for where we had common sub-options). However, I backed away from that because I didn't like the effect it had on the help system. I wanted to see all options at once.

> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:232
> > +                        [Used internally] Name of the file used to store
> 
> Before, you mentioned that this 'Used internally' should really be 'used by
> script internals'. Given that in WebKit infrastructure, 'Used internally'
> mean something completely different, we probably shouldn't use that.

I mentioned that "Internal use only" should be reworded. "Used internally" is the new wording. I could further change that to "Used by script internals".

> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:399
> > +    def _get_root_dir(self):
> 
> I think this should be 'get_root_parent' since we usually use the root to
> refer to the root of the repository.

OK.

> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:412
> > +    # Return the path to the directory containing supporting build scripts.
> 
> Comments like this shouldn't be needed, the function name is descriptive
> enough.

I was just trying to be complete and comment all of the functions.

> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:625
> > +    def _get_generate_derived_sources_script(self):
> 
> It feels weird that when this is defined, it's always the same. Can it ever
> be something other than os.path.join(self._get_project_dir(), "Scripts",
> "generate-derived-sources.sh")?

Yes. Other generators "override" it and provide different paths. For instance, in one case "Scripts" is "scripts". In another case, the script is named "generate-webdriver-derived-sources" or "generate-derived-sources". 

> > Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:629
> > +    def _get_generate_unified_sources_script(self):
> 
> It feels weird that when this is defined, it's always the same. Can it ever
> be something other than os.path.join(self._get_project_dir(), "Scripts",
> "generate-unified-sources.sh")?

Yes. Ditto.

[Jonathan Bedard]

(In reply to Keith Rollin from comment #18)
> (In reply to Jonathan Bedard from comment #17)
> > I still need to spend some time with the BaseGenerator to really understand
> > what's going on there, but I want to at least post my comments on the
> > majority of the patch.
> > 
> > > Tools/Scripts/webkitpy/common/attribute_saver_unittest.py:79
> > > +        self.assertEquals(obj.value, 0)
> > 
> > Not sure what we're testing here. If line 77 was ever run, we would raise an
> > exception and fail the test.
> 
> I was rather indiscriminate with asserts and threw them all over the place,
> and left those in place that were inherited from other functions that I
> copy/pasted. I'm not sure if you're saying you want them taken out or not.

I'm not against keeping them as is, I just wanted to make sure that you were aware of what was actually getting run here.

For what it's worth, you actually added way more unit tests than expected...I was only expecting maybe 2 or 3 for this class.

> 
> > > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:65
> > > +    platform_aliases = {
> > 
> > Style nit: we usually do a 4 space indent and the final bracket is not
> > indented.
> 
> I just went with what vim gave me and check-webkit-style allowed. I could
> change this instance, but check-webkit-style should really be updated to
> catch "gotchas" like this, since there's no place (that I know of) where
> this is style is documented.

I agree that this is probably a deficiency in check-webkit-style, I think this is somewhere in PEP-8 <https://www.python.org/dev/peps/pep-0008/>, but I haven't actually read through all of it to be sure.

> 
> > > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:94
> > > +        self.project_specific_generators = {
> > 
> > This feels like this should be a class variable, not an instance variable.
> 
> It gets updated in InternalApplication, and so needs to be writeable. I'm
> not sure you can do that with class variables.

This is another thing that isn't intuitive for a C++ programer, class variables are writable in children classes and can be accessed via self.CLASS_VARIABLE. webkitpy/port/base.py and webkitpy/port/ios_simulator.py have a pretty good example of this.

...

> 
> > > Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py:183
> > > +        parser.add_argument("command", action=util.CheckCommandAction,
> > 
> > Not something you need to change, but just some food for thought:
> > 
> > Python argparse can create sub parsers,
> 
> I'm familiar with sub-parsers, and originally used them here (along with
> shared parsers for where we had common sub-options). However, I backed away
> from that because I didn't like the effect it had on the help system. I
> wanted to see all options at once.

Fair enough!
...

[Jonathan Bedard]

(In reply to Build Bot from comment #15)

...

> ERROR: Tools/Scripts/webkitpy/xcode/sdk_unittest.py:37:  indentation
> contains tabs  [pep8/W191] [5]

I know this is for unit tests, but I have a feeling that SVN might actually hard-reject tabs in some circumstances. I'm going to confirm this with some other folks on the operations team.

...

[Alexey Proskuryakov]

Our pre-commit hook will block committing files with tabs unless the file in question has the allow-tabs property set to true.

[Keith Rollin]

(In reply to Jonathan Bedard from comment #19)

> Which might be better than this _initialize() method. I don't feel super strongly about this, though, because you have already marked this method with an '_'

After I thought about it some more, it seems that your suggested approach is the reverse of what I need and doesn't work very elegantly here. What I need to do is post-process the effects of Child's __init__. What's you're suggesting is pre-processing the Base's __init__. The form of this pre-processing is to stash some data in the object for the Base class to detect and respond to. This is awkward, and gets even more awkward the more post-processing that the Child __init__ needs to perform.

I can think of other approaches that don't include an _initialize() utility, but since you're someone OK with the current approach, I'd like to stay with it.

> I agree that this is probably a deficiency in check-webkit-style, I think
> this is somewhere in PEP-8 <https://www.python.org/dev/peps/pep-0008/>, but
> I haven't actually read through all of it to be sure.

I've gone through and dedented a bunch of places where lists, tuples, and dicts were indented by 8. There are other places that are still indented by 8; I left them that way because I don't know the specifics of your style guide. Such places include multi-line return value evaluated in ()'s and multi-line function parameter lists.

> This is another thing that isn't intuitive for a C++ programer, class
> variables are writable in children classes and can be accessed via
> self.CLASS_VARIABLE. webkitpy/port/base.py and
> webkitpy/port/ios_simulator.py have a pretty good example of this.

It's not unintuitive to me; I just thought they were read-only because that's what makes sense to me. Changing the definition of a class -- which acts like a template for instances of that class and so should offer a contract about what it provides -- seems like bad form to me.

> ...I have a feeling that SVN might actually hard-reject tabs in some circumstances.

> Our pre-commit hook will block committing files with tabs unless the file in question has the allow-tabs property set to true.

These tabs are in a string, and are there specifically to represent the output of `xcodebuild -showsdks`. So it seems like the pre-commit hook is being overly-cautious, since (a) I need those tabs, and (b) I'm not violating the reason for the check. Still, I guess I could replace the tabs with \t's.

[Keith Rollin]

Created attachment 369694 [details]
Patch

[Keith Rollin]

Updated for recent comments.

[EWS Watchlist]

Attachment 369694 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:230:  [BaseGenerator.report_error] Raising NoneType while only classes, instances or string are allowed  [pylint/E0702] [5]
ERROR: Tools/Scripts/webkitpy/xcode/__init__.py:25:  [xcode_hash_for_path] Instance of 'md5' has no 'digest' member  [pylint/E1101] [5]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Comment on attachment 369694 [details]
Patch

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

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:114
> +    def generate(self):

The generate vs subgenerate confuses me. I can't intuit which one is the generation which triggers a subprocess and which one is the one that actually does the generation. It seems like subgenerate is actually the one that does the generation. I would call these functions 'generate' and something like 'set_environment_and_generate' or something to that effect to make the difference more clear.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:271
> +            self._replace(input.name, "^JavaScriptCore/",               "$(PROJECT_DIR)/")

This whole section of code feels like a problem waiting to happen. I'm not sure that we're in a place to fix this now (after all, the existing code has the same problem) but I really don't like seeing project specific names in the general class. I wanted to call this out, I don't think it's practical to fix this in this patch.

> Tools/Scripts/webkitpy/generate_xcfilelists_lib/util.py:36
> +PY3 = sys.version_info.major >= 3

This is unused.

> Tools/Scripts/webkitpy/xcode/sdk.py:64
> +    def get_preferred_sdk_for_platform(cls, platform):

What we need to do with this class is pass in an executive object to this function.

That's going to look something like this:

get_preferred_sdk_for_platform(css, platform, executive=None):
    executive = executive or Executive()

The advantage of this approach is that it will remove the need to do the whole 'test_output' thing. Instead, you can just pass a MockExecutive2(output='testing output') when you call this function in unit tests.

> Tools/Scripts/webkitpy/xcode/sdk_unittest.py:69
> +    def tearDpwnClass(cls):

Typo

[Keith Rollin]

Created attachment 369994 [details]
Patch

[EWS Watchlist]

Attachment 369994 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py:230:  [BaseGenerator.report_error] Raising NoneType while only classes, instances or string are allowed  [pylint/E0702] [5]
ERROR: Tools/Scripts/webkitpy/xcode/__init__.py:25:  [xcode_hash_for_path] Instance of 'md5' has no 'digest' member  [pylint/E1101] [5]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 369994 [details]
Patch

Clearing flags on attachment: 369994

Committed r245364: <https://trac.webkit.org/changeset/245364>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

This caused my make-based workflow to break:

PhaseScriptExecution Check .xcfilelists
Traceback (most recent call last):
  File "/Volumes/Users/jer/Projects/WebKit.git/OpenSource/Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py", line 329, in _do_generate_common
  File "/Volumes/Users/jer/Projects/WebKit.git/OpenSource/Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py", line 311, in core_operation
  File "/Volumes/Users/jer/Projects/WebKit.git/OpenSource/Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py", line 180, in generate
  File "/Volumes/Users/jer/Projects/WebKit.git/OpenSource/Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py", line 291, in _generate_derived
  File "/Volumes/Users/jer/Projects/WebKit.git/OpenSource/Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py", line 608, in _get_derived_sources_dir
  File "/Volumes/Users/jer/Projects/WebKit.git/OpenSource/Tools/Scripts/webkitpy/generate_xcfilelists_lib/application.py", line 459, in get_xcode_built_products_dir
AssertionError

Where that assertion error is:

    def get_xcode_built_products_dir(self):
>       assert util.is_running_under_xcode()
        return self._getenv("BUILT_PRODUCTS_DIR")

We should be able to build without running Xcode.

[Jer Noble]

(In reply to Jer Noble from comment #31)
> This caused my make-based workflow to break:

Keith and I figured out that this was because my Xcode doesn't define the XCODE_INSTALL_PATH environment variable in its subshell. Keith is going to take a look at my Xcode's list of environment variables to see if there's a safer one to assert upon.