check sys deps better on chromium ports
Created attachment 48943 [details] Patch
David, Eric, can I get one of you two to review this?
Comment on attachment 48943 [details] Patch I plan to add PrettyPatch support (a ruby version of wdiff which is inluded in webkit.org's repository) to the base.Port at some point. Then ChromiumMac can use that instead of wdiff if they so desire. We eventually need to rename "target" to "configuration" to match webkit/mac terminology: 188 return self._build_path(self._options.target, 'image_diff') I'm surprised that the _build_path function doesn't automatically include the target already: 182 return self._build_path(self._options.target, 'test_shell') seems silly to have to include it everywhere. Maybe you just want a _current_build_path or _build_path when passed "none" for the "target" option should default to the current? Why are both debug and release required? 106 def _check_build_up_to_date(self, target): We tend to avoid space-based indent alignment in webkit, because you always end up having to change it later: 59 result = self._check_lighttpd_install() and result 60 result = self._check_apache_install() and result 61 result = self._check_wdiff_install() and result Shouldn't this just be on the base port? 45 def check_file_exists(path_to_file, str): Otherwise looks OK.
Comment on attachment 48943 [details] Patch > def check_sys_deps(self): > - # We have no platform-specific dependencies to check. > - return True > + result = chromium.ChromiumPort.check_sys_deps(self) > + result = self._check_lighttpd_install() and result > + result = self._check_apache_install() and result > + result = self._check_wdiff_install() and result > + result = self._check_build_up_to_date(self._options.target) and result > + if not result: > + logging.error('For complete Linux build requirements, please see:') > + logging.error('') > + logging.error(' http://code.google.com/p/chromium/wiki/' > + 'LinuxBuildInstructions') > + return result Here and elsewhere, it would be nice if we only checked for these things if they're actually going to be used. For example, only one of lighttpd or apache is needed, and only if we're going to be running http tests. Also, wdiff is only needed if the appropriate flag is turned on. Can we pass that info into check_sys_deps? Most of that info is in the commandline options, so we should be able to just pass that.
(In reply to comment #3) > (From update of attachment 48943 [details]) > I plan to add PrettyPatch support (a ruby version of wdiff which is inluded in > webkit.org's repository) to the base.Port at some point. Then ChromiumMac can > use that instead of wdiff if they so desire. > Cool. I'll take a look at PrettyPatch. > We eventually need to rename "target" to "configuration" to match webkit/mac > terminology: Okay. > 188 return self._build_path(self._options.target, 'image_diff') > > I'm surprised that the _build_path function doesn't automatically include the > target already: > 182 return self._build_path(self._options.target, 'test_shell') > It did before, actually. I just changed it for this purpose. > seems silly to have to include it everywhere. Maybe you just want a > _current_build_path or _build_path when passed "none" for the "target" option > should default to the current? > Yeah, that might be better. > Why are both debug and release required? > 106 def _check_build_up_to_date(self, target): > They aren't. If one doesn't exist, you get the OSError exception raised, and we fall out happily. > We tend to avoid space-based indent alignment in webkit, because you always end > up having to change it later: > 59 result = self._check_lighttpd_install() and result > 60 result = self._check_apache_install() and result > 61 result = self._check_wdiff_install() and result > Yeah, I could see that. > > Shouldn't this just be on the base port? > 45 def check_file_exists(path_to_file, str): > It could be. I resisted putting it on the base port just b/c I wasn't sure if others would want it outside of Chromium. (And I would've had to import another file). -- Dirk
Created attachment 49046 [details] patch updated w/ eric, ojan's feedback adding revised patch w/ eric and ojan's feedback.
Comment on attachment 49046 [details] patch updated w/ eric, ojan's feedback > + def _check_build_up_to_date(self, target): > + if target in ('Debug', 'Release'): > + try: > + debug_path = self._build_path('Debug', 'test_shell') > + release_path = self._build_path('Release', 'test_shell') > + > + debug_mtime = os.stat(debug_path).st_mtime > + release_mtime = os.stat(release_path).st_mtime > + > + if (debug_mtime > release_mtime and target == 'Release' or > + release_mtime > debug_mtime and target == 'Debug'): > + logging.warning('You are not running the most ' > + 'recent test_shell binary. You need to ' > + 'pass --debug or not to select between ' > + 'Debug and Release.') > + logging.warning('') > + # This will fail if we don't have both a debug and release binary. > + # That's fine because, in this case, we must already be running the > + # most up-to-date one. > + except OSError: > + pass > + return True Why do we check this? And why only on Linux? I can imagine cases where I'd want to run a binary that's not the most recent (e.g. if I don't want to spend the time to rebuild just to test something quickly in a release or debug build.
(In reply to comment #7) > (From update of attachment 49046 [details]) > > + def _check_build_up_to_date(self, target): > > + if target in ('Debug', 'Release'): > > + try: > > + debug_path = self._build_path('Debug', 'test_shell') > > + release_path = self._build_path('Release', 'test_shell') > > + > > + debug_mtime = os.stat(debug_path).st_mtime > > + release_mtime = os.stat(release_path).st_mtime > > + > > + if (debug_mtime > release_mtime and target == 'Release' or > > + release_mtime > debug_mtime and target == 'Debug'): > > + logging.warning('You are not running the most ' > > + 'recent test_shell binary. You need to ' > > + 'pass --debug or not to select between ' > > + 'Debug and Release.') > > + logging.warning('') > > + # This will fail if we don't have both a debug and release binary. > > + # That's fine because, in this case, we must already be running the > > + # most up-to-date one. > > + except OSError: > > + pass > > + return True > > Why do we check this? And why only on Linux? I can imagine cases where I'd want > to run a binary that's not the most recent (e.g. if I don't want to spend the > time to rebuild just to test something quickly in a release or debug build. Agreed. Presumably that's why it's a warning and not an error. Here I am merely cloning over code that existed in platform_utils_linux.py (and which wasn't in the _mac or _win ports). I personally am indifferent to this feature. It looks like agl@chromium.org added this back in september. Adam, any insight here?
(In reply to comment #8) > Agreed. Presumably that's why it's a warning and not an error. Here I am merely > cloning over code that existed in platform_utils_linux.py (and which wasn't in > the _mac or _win ports). I personally am indifferent to this feature. Ah. Sorry, I missed that it's a warning. That's nice actually. That code looks cross-platform to me. Could this be moved into the cross-platform code? In either case, this lgtm.
(In reply to comment #9) > (In reply to comment #8) > > Agreed. Presumably that's why it's a warning and not an error. Here I am merely > > cloning over code that existed in platform_utils_linux.py (and which wasn't in > > the _mac or _win ports). I personally am indifferent to this feature. > > Ah. Sorry, I missed that it's a warning. That's nice actually. That code looks > cross-platform to me. Could this be moved into the cross-platform code? > > In either case, this lgtm. Yes, there's no reason this couldn't be moved into the cross-platform code.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Agreed. Presumably that's why it's a warning and not an error. Here I am merely > > > cloning over code that existed in platform_utils_linux.py (and which wasn't in > > > the _mac or _win ports). I personally am indifferent to this feature. > > > > Ah. Sorry, I missed that it's a warning. That's nice actually. That code looks > > cross-platform to me. Could this be moved into the cross-platform code? > > > > In either case, this lgtm. > > Yes, there's no reason this couldn't be moved into the cross-platform code. I take that back (slightly). The code above assumes the names of the test_shell binary (and the path). You'd have to make that generic, probably ideally by adding a target parameter to _path_to_driver(). I will do that and upload a new patch.
Created attachment 49052 [details] patch revised to make _check_build_up_to_date() shared by chromium ports
> It looks like agl@chromium.org added this back in september. Adam, any insight > here? It resulted from far too many times when the layout tests were running a different binary from the one that I thought they were. I only added it for Linux because I wasn't sure that it would be universally appreciated, but I could poll all the Linux people in a couple of seconds. If you think it would be good on other platforms than that would be great.
Thanks for making all those changes. lgtm.
Comment on attachment 48943 [details] Patch Cleared Eric Seidel's review+ from obsolete attachment 48943 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 49052 [details] patch revised to make _check_build_up_to_date() shared by chromium ports Looks prematurely wrapped: 66 result = check_file_exists(test_shell_binary_path, 67 'test driver') We'll eventually change this to match the rest of WebKit: 178 logging.warning('You are not running the most ' 179 'recent test_shell binary. You need to ' 180 'pass --debug or not to select between ' 181 'Debug and Release.') (webKit scripts respect set-webkit-configuration, and takes either --debug or --release on the command line.) This is sorta confusing to not tell them why it failed: 58 result = self._check_wdiff_install() and result 59 if not result: 60 logging.error('For complete Mac build requirements, please see:') 61 logging.error('') 62 logging.error(' http://code.google.com/p/chromium/wiki/' 63 'MacBuildInstructions') This seems like it could cause confusion with out of date binaries in webkit: def _build_path(self, *comps): 97 # FIXME(dpranke): allow for builds under 'chrome' as well. 98 return self.path_from_chromium_base('webkit', self._options.target, 99 *comps) 104 p = self.path_from_chromium_base('webkit', *comps) 105 if os.path.exists(p): 106 return p 107 return self.path_from_chromium_base('chrome', *comps) This looks no worse than current. so rs=me. Please consider the above comments.
(In reply to comment #16) > (From update of attachment 49052 [details]) > Looks prematurely wrapped: > 66 result = check_file_exists(test_shell_binary_path, > 67 'test driver') > Fixed. Not sure how I missed this. > We'll eventually change this to match the rest of WebKit: > 178 logging.warning('You are not running the most ' > 179 'recent test_shell binary. You need to > ' > 180 'pass --debug or not to select between > ' > 181 'Debug and Release.') > > (webKit scripts respect set-webkit-configuration, and takes either --debug or > --release on the command line.) > That's up to you :) > This is sorta confusing to not tell them why it failed: > 58 result = self._check_wdiff_install() and result > 59 if not result: > 60 logging.error('For complete Mac build requirements, please > see:') > 61 logging.error('') > 62 logging.error(' http://code.google.com/p/chromium/wiki/' > 63 'MacBuildInstructions') > The error message is logged as part of _check_wdiff_install(). > This seems like it could cause confusion with out of date binaries in webkit: > def _build_path(self, *comps): > 97 # FIXME(dpranke): allow for builds under 'chrome' as well. > 98 return self.path_from_chromium_base('webkit', self._options.target, > 99 *comps) > 104 p = self.path_from_chromium_base('webkit', *comps) > 105 if os.path.exists(p): > 106 return p > 107 return self.path_from_chromium_base('chrome', *comps) > Agreed. It is what we currently do on windows. I wish we had a single build dir, but we don't.
Created attachment 49239 [details] update w/ typo fix, updated changelog date.
Comment on attachment 49239 [details] update w/ typo fix, updated changelog date. Rejecting patch 49239 from review queue. dpranke@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed). Due to bug 30084 the commit-queue will require a restart after your change. Please contact eseidel@chromium.org to request a commit-queue restart. After restart the commit-queue will correctly respect your reviewer rights.
Comment on attachment 49239 [details] update w/ typo fix, updated changelog date. You don't have to set r+ for things to get committed. The commit-queue will find this patch with just the cq+ and commit it. It won't commit patches with r=? or r-, and if patches have OOPS in the reviewer line it won't fix them.
Comment on attachment 49239 [details] update w/ typo fix, updated changelog date. rs=me.
I was demonstrating to Dirk that the commit-queue would work w/o an r+ :) But OK. The commit-queue (really svn-apply) won't really do anything with the r+ on this patch, since the reviewer line is already filled in.
(In reply to comment #22) > I was demonstrating to Dirk that the commit-queue would work w/o an r+ :) But > OK. The commit-queue (really svn-apply) won't really do anything with the r+ > on this patch, since the reviewer line is already filled in. Good to know, thanks!
Comment on attachment 49239 [details] update w/ typo fix, updated changelog date. Clearing flags on attachment: 49239 Committed r55123: <http://trac.webkit.org/changeset/55123>
All reviewed patches have been landed. Closing bug.