Add the AppleAdditions class to webkitpy to allow Internal tools to be used from webkitpy. Provide needed hooks for on-device testing.
Created attachment 306183 [details] Patch
<rdar://problem/31433077>
The proposed patch exposes safaripy and a dependency on its organization. This approach is not consistent with the approach I discussed with you most recently two weeks ago. In that discussion I suggested that we assume that the AppleInternalAdditions module be in the Python module search path (PYTHONPATH) and using a try/catch to try to load the module. The benefit of this approach is that we avoid making webkitpy dpeendent on safaripy or the hierarchy of safaripy. What issue(s) did you run into that led you to your proposed patch?
My proposal also did not need to introduce DefaultAppleAdditions. I suggested using the existence of the module as a means to trigger a different code path.
(In reply to Daniel Bates from comment #3) > ... This would require changing how import paths work in safaripy, as all files assume the import path starts in the folder containing safaripy. There is a work-around for this, but it would require modifying the system path in the internal code in a way I would consider undesirable. The stub methods provided in this patch encapsulate the specifics of importing this optional module. I used this technique to ensure that the insertion of the Apple Additions only needs occur in a single place. These stub methods also allow WebKit developers to clearly see which functionality the Apple Additions module will be providing.
(In reply to Jonathan Bedard from comment #5) > (In reply to Daniel Bates from comment #3) > > ... > > This would require changing how import paths work in safaripy, as all files > assume the import path starts in the folder containing safaripy. There is a > work-around for this, but it would require modifying the system path in the > internal code in a way I would consider undesirable. Can we solve this issue by adding safaripy to PYTHONPATH? > > The stub methods provided in this patch encapsulate the specifics of > importing this optional module. I do not see the need for such stubs. I elaborate further below. > I used this technique to ensure that the insertion of the Apple Additions only needs occur in a single place. We can accomplish the same goal by defining a free function in webkitpy/config/config.py, say have_apple_internal_additions(), that checks if the module AppleInternalAdditions was loaded or tries to load it. Something like: @memoized def have_apple_internal_additions(): if 'AppleInternalAdditions' in sys.modules: return True try: module = __import__('AppleInternalAdditions', globals(), locals(), [ ], 0) # Absolute import globals()[module.__name__] = module except ImportError: return False return True (*) Then code that needs to make use of the AppleInternalModule will look like: if config.have_apple_internal_additions(): AppleInternalAdditions.some_internal_function() If we are worried about polluting the global namespace with AppleInternalAdditions (I am not worried; should I be worried?) then we could rename and modify have_apple_internal_additions() return a module object instead of a boolean result. > These stub methods also allow WebKit developers to clearly see which functionality > the Apple Additions module will be providing. This is unnecessary using the approach I described to you in person two weeks ago and by (*). Looking at call sites like (*) it will be obvious to a reader what function/constant that needs to be implemented and the syntax used in (*) means that a reader can find all the functions/constants that need to be implemented by searching the code for "AppleInternalAdditions". Moreover having stub functions/constants and using the AppleInternalAdditions loading approach in the proposed patch (attachment 306183 [details]) is error prone for Apple engineers that work on the Python code because they will need to remember to always add a stub function/constant corresponding to a function/constant added to AppleInternalAdditions or the Open Source implementation will break (if one ever manifests itself). Should an Open Source replacement of AppleInternalAdditions be attempted I expect it will manifest as just that, a replacement. That is, code that calls AppleInternalAdditions will be replaced with code that lives in webkitpy. Such replacement can be done (as in coded and enabled by default immediately) iteratively when the code is structured as in (*) as opposed to the all-or-nothing approach in the proposed patch (since switching Apple engineers to the Open Source code requires removing the logic to load the AppleInternalAdditions module and this would not be attempted until all functionality is replaced - a difficult task for an Open Source contributor). Therefore, the stubs methods do not provide any benefit and hamper a future Open Source replacement.
(In reply to Daniel Bates from comment #6) > (In reply to Jonathan Bedard from comment #5) > > (In reply to Daniel Bates from comment #3) > > > ... > > > > This would require changing how import paths work in safaripy, as all files > > assume the import path starts in the folder containing safaripy. There is a > > work-around for this, but it would require modifying the system path in the > > internal code in a way I would consider undesirable. > > Can we solve this issue by adding safaripy to PYTHONPATH? > In short, no. Adding safaripy to PYTHONPATH would mean that import paths for modules from safaripy would start in safaripy as opposed to the directory containing safaripy, which is where they should start. Instead, the provided approach adds the directory containing safaripy to PYTHONPATH, that way all imports from webkitpy or safaripy will be of the form: import <toplevel>.<file> import <toplevel>.<directory>.<file> where <toplevel> is either webkitpy or safaripy. Adding safaripy to PYTHONPATH would break this structure. The breakage does not occur in this file, but rather, the files which this file imports. > > ... > > > We can accomplish the same goal by defining a free function in > webkitpy/config/config.py, say have_apple_internal_additions(), that checks > if the module AppleInternalAdditions was loaded or tries to load it. > Something like: > > @memoized > def have_apple_internal_additions(): > if 'AppleInternalAdditions' in sys.modules: > return True > try: > module = __import__('AppleInternalAdditions', globals(), locals(), [ > ], 0) # Absolute import > globals()[module.__name__] = module > except ImportError: > return False > return True > > (*) Then code that needs to make use of the AppleInternalModule will look > like: > > if config.have_apple_internal_additions(): > AppleInternalAdditions.some_internal_function() I don't think the verbose version of import is needed. If the correct directory is in the system path, a try/except should be sufficient. My understanding is that __import__ is used when the module which needs to be imported is defined by a variable. > If we are worried about polluting the global namespace with > AppleInternalAdditions (I am not worried; should I be worried?) then we > could rename and modify have_apple_internal_additions() return a module > object instead of a boolean result. > > > These stub methods also allow WebKit developers to clearly see which functionality > > the Apple Additions module will be providing. > > This is unnecessary using the approach I described to you in person two > weeks ago and by (*). Looking at call sites like (*) it will be obvious to a > reader what function/constant that needs to be implemented and the syntax > used in (*) means that a reader can find all the functions/constants that > need to be implemented by searching the code for "AppleInternalAdditions". > Moreover having stub functions/constants and using the > AppleInternalAdditions loading approach in the proposed patch (attachment > 306183 [details]) is error prone for Apple engineers that work on the Python > code because they will need to remember to always add a stub > function/constant corresponding to a function/constant added to > AppleInternalAdditions or the Open Source implementation will break (if one > ever manifests itself). Should an Open Source replacement of > AppleInternalAdditions be attempted I expect it will manifest as just that, > a replacement. That is, code that calls AppleInternalAdditions will be > replaced with code that lives in webkitpy. Such replacement can be done (as > in coded and enabled by default immediately) iteratively when the code is > structured as in (*) as opposed to the all-or-nothing approach in the > proposed patch (since switching Apple engineers to the Open Source code > requires removing the logic to load the AppleInternalAdditions module and > this would not be attempted until all functionality is replaced - a > difficult task for an Open Source contributor). Therefore, the stubs methods > do not provide any benefit and hamper a future Open Source replacement. I think you bring up a good point here about fragility. I will upload a cleaner version of this change which removes stub functions and defines AppleAdditions as a module instead of a static class contained in a module. My version will be less verbose then some of the code you've suggested, as I think Python allows for this to be done more succinctly.
Created attachment 306425 [details] Patch
(In reply to Jonathan Bedard from comment #7) > (In reply to Daniel Bates from comment #6) > > (In reply to Jonathan Bedard from comment #5) > > > (In reply to Daniel Bates from comment #3) > > > > ... > > > > > > This would require changing how import paths work in safaripy, as all files > > > assume the import path starts in the folder containing safaripy. There is a > > > work-around for this, but it would require modifying the system path in the > > > internal code in a way I would consider undesirable. > > > > Can we solve this issue by adding safaripy to PYTHONPATH? > > > > In short, no. Adding safaripy to PYTHONPATH would mean that import paths > for modules from safaripy would start in safaripy as opposed to the > directory containing safaripy, which is where they should start. I meant to ask: Can we solve this issue by adding the directory containing safaripy to PYTHONPATH? And to be clear I mean that the caller of run-webkit-tests sets up such a PYTHONPATH. > Instead, the provided approach adds the directory containing safaripy to PYTHONPATH, > that way all imports from webkitpy or safaripy will be of the form: > Where? > import <toplevel>.<file> > import <toplevel>.<directory>.<file> > > where <toplevel> is either webkitpy or safaripy. Adding safaripy to > PYTHONPATH would break this structure. The breakage does not occur in this > file, but rather, the files which this file imports. > > > > > ... > > > > > > We can accomplish the same goal by defining a free function in > > webkitpy/config/config.py, say have_apple_internal_additions(), that checks > > if the module AppleInternalAdditions was loaded or tries to load it. > > Something like: > > > > @memoized > > def have_apple_internal_additions(): > > if 'AppleInternalAdditions' in sys.modules: > > return True > > try: > > module = __import__('AppleInternalAdditions', globals(), locals(), [ > > ], 0) # Absolute import > > globals()[module.__name__] = module > > except ImportError: > > return False > > return True > > > > (*) Then code that needs to make use of the AppleInternalModule will look > > like: > > > > if config.have_apple_internal_additions(): > > AppleInternalAdditions.some_internal_function() > > I don't think the verbose version of import is needed. If the correct > directory is in the system path, a try/except should be sufficient. My > understanding is that __import__ is used when the module which needs to be > imported is defined by a variable. Notice that I am importing AppleInternalAdditions from a function into the global namespace. The scope of a function defines a local namespace and the import statement imports into the namespace it is invoked in. So, calling import from inside the function will import the namespace defined by the function. But I want to import AppleInternalAdditions into the global namespace. From reading the documentation, this can be done using __import__() or importlib.import_module(). I chose the former because it does not require importing another module, importlib, and it allows me to force absolute path importing instead of relative path importing to make the code less error prone in the unlikely event that 'AppleInternalAdditions' be changed to a relative path, which it should never be changed to because we are assuming it can be found in PYTHONPATH.
(In reply to Daniel Bates from comment #9) > (In reply to Jonathan Bedard from comment #7) > > (In reply to Daniel Bates from comment #6) > > > (In reply to Jonathan Bedard from comment #5) > > > > (In reply to Daniel Bates from comment #3) > > > > > ... > > > > > > > > This would require changing how import paths work in safaripy, as all files > > > > assume the import path starts in the folder containing safaripy. There is a > > > > work-around for this, but it would require modifying the system path in the > > > > internal code in a way I would consider undesirable. > > > > > > Can we solve this issue by adding safaripy to PYTHONPATH? > > > > > > > In short, no. Adding safaripy to PYTHONPATH would mean that import paths > > for modules from safaripy would start in safaripy as opposed to the > > directory containing safaripy, which is where they should start. > > I meant to ask: > > Can we solve this issue by adding the directory containing safaripy to > PYTHONPATH? You already answered this question. It would work.
Comment on attachment 306425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306425&action=review > Tools/Scripts/webkitpy/common/apple_additions.py:26 > +try: > + import safaripy.apple_additions as AppleAdditions > +except ImportError: > + AppleAdditions = None This still exposes unnecessary details about safaripy and it is weird semantically that even if a file imports this module (as seen on line 39 of factory.py) that accessing the module, AppleAdditions, may return None.
(In reply to Daniel Bates from comment #10) > (In reply to Daniel Bates from comment #9) > > (In reply to Jonathan Bedard from comment #7) > > > (In reply to Daniel Bates from comment #6) > > > > (In reply to Jonathan Bedard from comment #5) > > > > > (In reply to Daniel Bates from comment #3) > > > > > > ... > > > > > > > > > > This would require changing how import paths work in safaripy, as all files > > > > > assume the import path starts in the folder containing safaripy. There is a > > > > > work-around for this, but it would require modifying the system path in the > > > > > internal code in a way I would consider undesirable. > > > > > > > > Can we solve this issue by adding safaripy to PYTHONPATH? > > > > > > > > > > In short, no. Adding safaripy to PYTHONPATH would mean that import paths > > > for modules from safaripy would start in safaripy as opposed to the > > > directory containing safaripy, which is where they should start. > > > > I meant to ask: > > > > Can we solve this issue by adding the directory containing safaripy to > > PYTHONPATH? > > > You already answered this question. It would work. So yes, adding both paths to the PYTHONPATH would allow everything to work. But having both of these techniques seems very wrong. When we add safaripy to the PYTHONPATH, we're essentially throwing out the established scoping pattern already used in webkitpy. I don't see a good reason to break the organizational assumptions of the project just to leave out 'safaripy.'
(In reply to Daniel Bates from comment #9) > (In reply to Jonathan Bedard from comment #7) > > Instead, the provided approach adds the directory containing safaripy to PYTHONPATH, > > that way all imports from webkitpy or safaripy will be of the form: > > > > Where? > This should read: Instead, the provided approach assumes the directory containing safaripy is in the PYTHONPATH Sorry for the confusion.
(In reply to Daniel Bates from comment #11) > Comment on attachment 306425 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306425&action=review > > > Tools/Scripts/webkitpy/common/apple_additions.py:26 > > +try: > > + import safaripy.apple_additions as AppleAdditions > > +except ImportError: > > + AppleAdditions = None > > This still exposes unnecessary details about safaripy and it is weird > semantically that even if a file imports this module (as seen on line 39 of > factory.py) that accessing the module, AppleAdditions, may return None. Your right that this isn't an idiom we currently use since we don't have optional imports. But this seems like the most Pythonic way to do this. We could (as you suggest) export the module to the global namespace. But I don't think that is an intuitive way to achieve an optional module, it would break break IDEs and decrease code readability in the places the global module is used. It seems better to explicitly pass an optional module as 'None.' It will be very obvious when it is used that it is undefined and it will also be obvious why it is undefined.
(In reply to Jonathan Bedard from comment #12) > (In reply to Daniel Bates from comment #10) > > (In reply to Daniel Bates from comment #9) > > > (In reply to Jonathan Bedard from comment #7) > > > > (In reply to Daniel Bates from comment #6) > > > > > (In reply to Jonathan Bedard from comment #5) > > > > > > (In reply to Daniel Bates from comment #3) > > > > > > > ... > > > > > > > > > > > > This would require changing how import paths work in safaripy, as all files > > > > > > assume the import path starts in the folder containing safaripy. There is a > > > > > > work-around for this, but it would require modifying the system path in the > > > > > > internal code in a way I would consider undesirable. > > > > > > > > > > Can we solve this issue by adding safaripy to PYTHONPATH? > > > > > > > > > > > > > In short, no. Adding safaripy to PYTHONPATH would mean that import paths > > > > for modules from safaripy would start in safaripy as opposed to the > > > > directory containing safaripy, which is where they should start. > > > > > > I meant to ask: > > > > > > Can we solve this issue by adding the directory containing safaripy to > > > PYTHONPATH? > > > > > > You already answered this question. It would work. > > So yes, adding both paths to the PYTHONPATH would allow everything to work. > > But having both of these techniques seems very wrong. When we add safaripy > to the PYTHONPATH, we're essentially throwing out the established scoping > pattern already used in webkitpy. The "established scoping pattern" is an artifact of how module importation in Python works, effectively forcing what we have today: a directory, webkitpy, that contains all-the-code. (E.g. There is no logical reason for webkitpy to contain multiple main() entry points for tools, including run-webkit-tests and webkit-patch, other than because the way Python supports importing makes it simpler to share Python code if it is all under a single rooted hierarchy than if it were not). The safaripy code is explicitly not in the Open Source tree (maybe one day it could be, but likely there will always be a reasons that is cannot be) and it should be treated analogous to a system module, disjoint. We should not put up a facade to make safaripy fit into the webkitpy scoping pattern. > I don't see a good reason to break the organizational assumptions of the project just to leave out 'safaripy.' The organization assumptions are not logical and are largely forced upon us as an artifact of how importing works in Python. I give an example of how our current organization is illogical (above) with respect to run-webkit-tests and webkit-patch.
(In reply to Daniel Bates from comment #15) > (In reply to Jonathan Bedard from comment #12) > > (In reply to Daniel Bates from comment #10) > > > (In reply to Daniel Bates from comment #9) > > > > (In reply to Jonathan Bedard from comment #7) > > > > > (In reply to Daniel Bates from comment #6) > > > > > > (In reply to Jonathan Bedard from comment #5) > > > > > > > (In reply to Daniel Bates from comment #3) > ... > > The "established scoping pattern" is an artifact of how module importation > in Python works, effectively forcing what we have today: a directory, > webkitpy, that contains all-the-code. (E.g. There is no logical reason for > webkitpy to contain multiple main() entry points for tools, including > run-webkit-tests and webkit-patch, other than because the way Python > supports importing makes it simpler to share Python code if it is all under > a single rooted hierarchy than if it were not). The safaripy code is > explicitly not in the Open Source tree (maybe one day it could be, but > likely there will always be a reasons that is cannot be) and it should be > treated analogous to a system module, disjoint. We should not put up a > facade to make safaripy fit into the webkitpy scoping pattern. > > > I don't see a good reason to break the organizational assumptions of the project just to leave out 'safaripy.' > > The organization assumptions are not logical and are largely forced upon us > as an artifact of how importing works in Python. I give an example of how > our current organization is illogical (above) with respect to > run-webkit-tests and webkit-patch. Except that abandoning the scoping pattern means that we now have to be careful about how things are named. In fact, this change assumes that the module being imported form safaripy is apple_additions. It is currently being imported from apple_additions. If the line reads like this: import apple_additions as AppleAdditions as you are suggesting, Python will import the Open Source module as AppleAdditions, even if the PYTHONPATH is set correctly. We could rename things to avoid this conflict, or preform this import from a function. But the fact that this conflict could exist demonstrates that we should be using some sort of scoping to disambiguate the import.
Created attachment 306918 [details] Patch
Created attachment 307389 [details] Patch
After discussing with Dan in person yesterday, I've made some changes to how this import works.
(In reply to Jonathan Bedard from comment #19) > After discussing with Dan in person yesterday, I've made some changes to how > this import works. This patch (attachment #307389 [details]) does not represent changes we discussed yesterday (04/17), which were in part a reiteration of my comments in this bug. In particular, this patch does not make use of the suggestions I gave in comment #6. And no explanation for such divergence has been given.
Created attachment 307442 [details] Patch
This patch (attachment 307442 [details]) has brought this import more along the lines of what Dan suggested, although, there are some differences (namely, this patch still isn't using the global namespace) and some possible scoping issues. This patch doesn't use the global namespace because I think that approach is confusing. Instead, places which use this function access the package returned by the function directly, I think this is more clear. Using the __import__() call instead of the import call means that we are targeting absolute paths only, an apple_additions module or package sitting in webkitpy/port will not be loaded. However, if an apple_additions module or package is sitting next to the entry point, this will always be loaded over one in another directory on the system path. This indicates a larger issue with the architecture of webkitpy. Lastly, this change implements the device polling and access on the apple_additions level instead of the IOSDeviceDelegate level. I'm not sure this is the best place for this. While these functions operate on global objects, they will only be used in the context of the iOSDevicePort.
Created attachment 308451 [details] Patch
Some changes to apple_additions have changed the interface required in webkitpy. I don't think the IOSDeviceDelegate is the correct way to implement this anymore. Apple additions should provide all of the needed functions.
Comment on attachment 308451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308451&action=review > Tools/Scripts/webkitpy/port/config.py:61 > + return __import__('apple_additions', globals(), None, [], 0) I suggest that we use named parameters for all arguments except the first. We should explain the purpose of the last argument, 0, in a comment even if we use a named parameter calling style as the name of the last parameter, level, is not very meaningful. > Tools/Scripts/webkitpy/port/ios_device.py:55 > + return apple_additions().ios_device_driver() if apple_additions() else super(IOSDevicePort, self)._driver_class() Does it make sense to call the base class implementation if we do not have Apple Additions? I mean, would using the base class driver class ever work? If we need to call the base class implementation then I would write this as: if apple_additions(): return apple_additions().ios_device_driver() return super(IOSDevicePort, self)._driver_class() > Tools/Scripts/webkitpy/port/ios_device.py:60 > + iphoneos_sdk_version = host.platform.xcode_sdk_version(IOSDevicePort.SDK) I suggest that we access the SDK class variable using the argument cls instead of hardcoding the class name. The benefit of doing this is the same as the benefit of using super() to call the base class implementation. > Tools/Scripts/webkitpy/port/ios_device.py:67 > + # FIXME: These need device implementations <rdar://problem/30497991> Minor: Missing period at the end of this sentence. > Tools/Scripts/webkitpy/port/ios_device.py:82 > + return ['--sdk', IOSDevicePort.SDK] + (['ARCHS=%s' % self.architecture()] if self.architecture() else []) Ditto. > Tools/Scripts/webkitpy/port/ios_device.py:89 > + raise RuntimeError('On-device testing is not supported on this machine') We could move the runtime error string literal into a class variable and avoid the need to duplicate it here and in _device_for_worker_number_map(). > Tools/Scripts/webkitpy/port/ios_device_unittest.py:40 > + # FIXME: Update tests when <rdar://problem/30497991> is completed Minor: Missing a period at the end of this sentence.
Created attachment 308627 [details] Patch for landing
Comment on attachment 308627 [details] Patch for landing Clearing flags on attachment: 308627 Committed r215965: <http://trac.webkit.org/changeset/215965>
All reviewed patches have been landed. Closing bug.