Bug 35062 – check sys deps better on chromium ports

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

check sys deps better on chromium ports

[Dirk Pranke]

Created attachment 48943 [details]
Patch

[Dirk Pranke]

David, Eric, can I get one of you two to review this?

[Eric Seidel (no email)]

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.

[Ojan Vafai]

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.

[Dirk Pranke]

(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

[Dirk Pranke]

Created attachment 49046 [details]
patch updated w/ eric, ojan's feedback

adding revised patch w/ eric and ojan's feedback.

[Ojan Vafai]

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.

[Dirk Pranke]

(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 [e-mail] added this back in september. Adam, any insight here?

[Ojan Vafai]

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 49052 [details]
patch revised to make _check_build_up_to_date() shared by chromium ports

[Adam Langley]

> It looks like [e-mail] 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.

[Ojan Vafai]

Thanks for making all those changes. lgtm.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 49239 [details]
update w/ typo fix, updated changelog date.

[WebKit Commit Bot]

Comment on attachment 49239 [details]
update w/ typo fix, updated changelog date.

Rejecting patch 49239 from review queue.

[e-mail] 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 [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your reviewer rights.

[Eric Seidel (no email)]

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.

[Dimitri Glazkov (Google)]

Comment on attachment 49239 [details]
update w/ typo fix, updated changelog date.

rs=me.

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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!

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.