Bug 215573 – [resultsdbpy]: Depend on webkitcorepy

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

[Jonathan Bedard]

There are a few parts of resultsdbpy that should really be broken into independent packages because other services need them.  The first step in that process is for resultsdbpy to depend on the library that some tooling is to be moved to.

[Radar WebKit Bug Importer]

<rdar://problem/67250272>

[Jonathan Bedard]

Created attachment 406720 [details]
Patch

[Jonathan Bedard]

Created attachment 406721 [details]
Patch

[Aakash Jain]

Comment on attachment 406721 [details]
Patch

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

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/__init__.py:25
> +

Better to make below code a method, like: https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py#L17 _add_webkitpy_to_sys_path()

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/__init__.py:26
> +# Hopefully we're beside webkitcorepy, otherwise it will need to be installed

'it' is ambiguous. Please replace with 'webkitcorepy'.
Nit: missing full-stop at the end.

Also, can you check if it's install and print a nice message if it isn't installed?

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/__init__.py:27
> +containing = os.path.dirname(os.path.dirname(os.path.abspath(os.path.dirname(__file__))))

'containing' should be renamed to name of actual folder, e.g.: libraries_path

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/__init__.py:28
> +path_addition = os.path.join(containing, 'webkitcorepy')

should be 'webkitcorepy_path'
'addition' seems out of context.

[Jonathan Bedard]

Comment on attachment 406721 [details]
Patch

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

>> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/__init__.py:26
>> +# Hopefully we're beside webkitcorepy, otherwise it will need to be installed
> 
> 'it' is ambiguous. Please replace with 'webkitcorepy'.
> Nit: missing full-stop at the end.
> 
> Also, can you check if it's install and print a nice message if it isn't installed?

Other than the error message you'll get on line 32 when you try and use the library?

When we have this situation with other packages we own, I intend to have sort of "include or auto-install" function, but we obviously can't auto-install the library that has the auto-installer in it....

[Aakash Jain]

(In reply to Jonathan Bedard from comment #5)
> > Also, can you check if it's install and print a nice message if it isn't installed?
> 
> Other than the error message you'll get on line 32 when you try and use the library?
> 
> When we have this situation with other packages we own, I intend to have sort of "include or auto-install" function, but we obviously can't auto-install the library that has the auto-installer in it....

Yeah, i am not referring to auto-installing webkitcorepy. I am only referring to better error message when import fails. e.g.: https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/test/main.py#L260 

Something like:
try:
    import package_name
except ImportError:
    _log.error('Failed to import webkitcorepy:, Please do XYZ to install this package')


Although it's ok without custom error message as well.

[Jonathan Bedard]

Created attachment 406729 [details]
Patch

[Jonathan Bedard]

Created attachment 406740 [details]
Patch for landing

[EWS]

Committed r265771: <https://trac.webkit.org/changeset/265771>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406740 [details].