| Summary: | [resultsdbpy]: Depend on webkitcorepy | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||
| Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | aakash_jain, dewei_zhu, slewis, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Jonathan Bedard
2020-08-17 09:26:48 PDT
Created attachment 406720 [details]
Patch
Created attachment 406721 [details]
Patch
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. 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.... (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. Created attachment 406729 [details]
Patch
Created attachment 406740 [details]
Patch for landing
Committed r265771: <https://trac.webkit.org/changeset/265771> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406740 [details]. |