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.
Created attachment 50965 [details] Proposed patch
Comment on attachment 50965 [details] Proposed patch Are we using anywhere?
Comment on attachment 50965 [details] Proposed patch LGTM
(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.
Comment on attachment 50965 [details] Proposed patch ok.
Why not just check both instead of adding a commandline flag?
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?
This is just for the image_diff binary right? Seems to me that we should just fallback to Debug.
(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 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.
"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.
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.
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.
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.
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.
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.
(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).
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?
(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.
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.
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.
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.
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.
Created attachment 52103 [details] Document the arguments per comment#23
Comment on attachment 52103 [details] Document the arguments per comment#23 OK.
Committed r56876: <http://trac.webkit.org/changeset/56876>