Bug 36245 – rebaseline_chromium_webkit_test.py should fallback to debug if release image diff does not exist

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

[Victor Wang]

rebaseline_chromium_webkit_test.py uses image_diff.exe to compare png diffs. Sometime, person who uses the tool only has debug or release binary, not both. Add an option to the script allow specifying which target to be used by the tool.

[Victor Wang]

Created attachment 50965 [details]
Proposed patch

[Dimitri Glazkov (Google)]

Comment on attachment 50965 [details]
Proposed patch

Are we using anywhere?

[Dirk Pranke]

Comment on attachment 50965 [details]
Proposed patch

LGTM

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 50965 [details])
> Are we using anywhere?

This bug was triggered by Jian Li and Albert, who only had Debug builds and couldn't run the rebaselining tool (which only looked for Release binaries). Note that the old downstream code actually defaulted to Debug, not Release, in this use case, but we'll leave the default to be Release to be consistent with the other tools.

[Dimitri Glazkov (Google)]

Comment on attachment 50965 [details]
Proposed patch

ok.

[Ojan Vafai]

Why not just check both instead of adding a commandline flag?

[Victor Wang]

Yes, we could modify the chromium port code to check whether the binary exists and fallback to another target if it does not.

(In reply to comment #6)
> Why not just check both instead of adding a commandline flag?

[Ojan Vafai]

This is just for the image_diff binary right? Seems to me that we should just fallback to Debug.

[Dirk Pranke]

(In reply to comment #8)
> This is just for the image_diff binary right? Seems to me that we should just
> fallback to Debug.

The old version of the code exposed the image_diff_path() to code that was only called by the rebaselining tool, so the rebaselining tool could fall back w/o affecting the rest of the code base (e.g., paths to test_shell, etc).

The new version of the code does not expose this hook, and I don't really want to do that because it feels ugly. I am okay with providing code that figures out which platform to use if it isn't specified, but I think having a --target is probably a good idea anyway, in case the user is working on image diff or has both builds or something.

[Victor Wang]

(In reply to comment #9)
> (In reply to comment #8)
> > This is just for the image_diff binary right? Seems to me that we should just
> > fallback to Debug.
> 
> The old version of the code exposed the image_diff_path() to code that was only
> called by the rebaselining tool, so the rebaselining tool could fall back w/o
> affecting the rest of the code base (e.g., paths to test_shell, etc).
> 
> The new version of the code does not expose this hook, and I don't really want
> to do that because it feels ugly. I am okay with providing code that figures
> out which platform to use if it isn't specified, but I think having a --target
> is probably a good idea anyway, in case the user is working on image diff or
> has both builds or something.

In case user does not specify "--target" option, providing port interface so rebaseline tool can figure out which platform to use sounds good to me.

[Eric Seidel (no email)]

"target" is actually supposed to be "configuration" or just --release or --debug to match XCode terminology (which is why WebKit follows, for better or worse).  But we can update that later.

[Victor Wang]

Created attachment 52065 [details]
Proposed patch

Added code to set option target based on which version of image_diff binary exists in case user does not specify the option.

[Eric Seidel (no email)]

Comment on attachment 52065 [details]
Proposed patch

Why is this special-cased for release?  And why would this sort of logic go on the port object itself?  Seems that the port object already does this sort of check it just does it in one big method.  We could easily break out the image diff part of that check into a separate method callable from here.  The goal is easy code re-use, and this doesn't look very re-useable as-is.

[Victor Wang]

Per my conversation with Dirk before, he prefer to not having these logics inside port object. If target is not specified, the rebaseline tool checks whether the release version exists, and use that if it does. Otherwise, set to debug. In case none of debug or release versions exists AND there is new image baseline, it will error out later.

I will update check_build method to reuse the check_image_diff method.

(In reply to comment #13)
> (From update of attachment 52065 [details])
> Why is this special-cased for release?  And why would this sort of logic go on
> the port object itself?  Seems that the port object already does this sort of
> check it just does it in one big method.  We could easily break out the image
> diff part of that check into a separate method callable from here.  The goal is
> easy code re-use, and this doesn't look very re-useable as-is.

[Victor Wang]

Created attachment 52088 [details]
Proposed patch

Updated check_build to re-use the image check port.

Eric,
Figuring out whether image diff exists and setting the target (if it is empty) based
on the result is kind of rebaseline tool specific so I agree with Dirk here on not having
these logics inside port object. let me know if you disagree and strongly prefer
we should move these logics into port.

[Eric Seidel (no email)]

I don't know if they belong in port, but certainly figuring out the default configuration is not specific to rebaseline.py.  on the perl side of things set-webkit-configuration handles setting the default and webkitdirs.pm handles reading from the default configuration (what you're calling target in this patch).  webkitdirs.pm also provides global support for handling --release and --debug flags which set the configuration.

[Dirk Pranke]

(In reply to comment #16)
> I don't know if they belong in port, but certainly figuring out the default
> configuration is not specific to rebaseline.py.  on the perl side of things
> set-webkit-configuration handles setting the default and webkitdirs.pm handles
> reading from the default configuration (what you're calling target in this
> patch).  webkitdirs.pm also provides global support for handling --release and
> --debug flags which set the configuration.

This patch isn't figuring out the default configuration. This patch is attempting to make the rebaselining too use *whatever* ImageDiff it can find.

The problem is that we default to Release, but users who only have Debug builds are asking that we automatically find their binaries without needing to specify a flag.

Your suggestion that we use something like set-webkit-configuration is another way to address the desired end result; whether or not it makes the users in question happy, I don't know (because now they have to remember to call that).

[Eric Seidel (no email)]

You are correct that some sort of "try Release" and then "Try debug" option is unique to rebaseline.py.  Does it only apply to the ImageDiff binary, or is it more global?  Why would a user have a built copy of ImageDiff debug?  Why wouldn't rebaseline just build image_diff/ImageDiff?

[Dirk Pranke]

(In reply to comment #18)
> You are correct that some sort of "try Release" and then "Try debug" option is
> unique to rebaseline.py.  Does it only apply to the ImageDiff binary, or is it
> more global?  Why would a user have a built copy of ImageDiff debug?  Why
> wouldn't rebaseline just build image_diff/ImageDiff?

Historically, the chromium test scripts don't ever try to build things. We could certainly change that.

Some developers apparently only have Debug builds, in which case having a rebaseline tool that defaults to Release won't find the binary they need. If they were to do rebaseline --debug, they would be fine (or if they used set-webkit-configuration). Which is why we added the --target option in the first place.

[Eric Seidel (no email)]

I'm confused then.  This --target option is only used for falback?

Why wouldn't we just always fallback then?  Why require an option?

It seems Ojan already suggested this in comment #6.

[Victor Wang]

Yes, this patch basically implements what Ojan suggested. The "target" option is an extra stuff in case user want to specify which image diff to use. It just provides extra control but if you feel it is not necessary, we could simply remove it.

(In reply to comment #20)
> I'm confused then.  This --target option is only used for falback?
> 
> Why wouldn't we just always fallback then?  Why require an option?
> 
> It seems Ojan already suggested this in comment #6.

[Victor Wang]

Created attachment 52098 [details]
Proposed patch

Remove the "target" option. The rebaseline tool already checks release image diff binary and fallback to debug if the release version does not exist. Having the extra option may confuse the user and it does not seem to be useful.

[Eric Seidel (no email)]

Comment on attachment 52098 [details]
Proposed patch

Please document the arguments in:
 1011     if not port_obj.check_image_diff(None, False):
by naming them in the callsite.

Otherwise this looks OK.

[Victor Wang]

Created attachment 52103 [details]
Document the arguments per comment#23

[Eric Seidel (no email)]

Comment on attachment 52103 [details]
Document the arguments per comment#23

OK.

[Victor Wang]

Committed r56876: <http://trac.webkit.org/changeset/56876>