Even though I ran set-webkit-configuration --debug a while back (and build-webkit --chromium respects it), NRWT kept looking for the DRT binary in the Release directory until I passed an explicit --debug flag to it. I suspect http://trac.webkit.org/changeset/68008 (unintentionally?) triggered this behavior change.
Odd ... I thought I had a test for this. Looking into it now; it's certainly possible that that change broke this.
+Adam, since he reviewed r68008. There was a change to chromium.py which is responsible for this: http://trac.webkit.org/changeset/68008/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py Was there a reason why ChromiumPort now sets the default configuration to Release in it constructor? There is code that runs later (_set_up_derived_options in run_webkit_tests.py) which would do the similar initialization: if not options.configuration: options.configuration = port_obj.default_configuration()
(In reply to comment #2) > Was there a reason why ChromiumPort now sets the default configuration to Release in it constructor? I now see that that code was always there, just duplicated in Mac/Linux/Windows subclasses of ChromiumPort. However, if options and not hasattr(options, 'configuration'): options.configuration = 'Release' Whereas now it's more aggressive: if (options and (not hasattr(options, 'configuration') or options.configuration is None)): options.configuration = 'Release' options.configuration is defined by parse_args in run_webkit_tests.py, it's just set to None. Any objection to reverting the options.configuration check to the way it was before?
No objection. This code was accidentally broken. I am in the middle of fixing it properly and writing better unit tests so that it doesn't break again. This turns out to involve a fair amount of yak-shaving, but I should have a patch this evening.
(In reply to comment #4) > I am in the middle of fixing it properly and writing better unit tests so that it doesn't break again. This turns out to involve a fair amount of yak-shaving, but I should have a patch this evening. I was looking into the "right" way of fixing it this afternoon, and it seemed like having the ChromiumPort constructor call self.default_configuration() instead of hardcoding 'Release' would be better, but I wasn't sure what the implications of that would be (for starters, the unit tests didn't pass, since they didn't mock out the configuration reading so were getting it from the filesystem).
Created attachment 68690 [details] Patch
+Tony, since I see he rolled back Dirk's quick fix since it broke NRWT on the canary bots (http://trac.webkit.org/changeset/68234). What exactly broke, and how can I make sure my patch doesn't break things too?
(In reply to comment #7) > +Tony, since I see he rolled back Dirk's quick fix since it broke NRWT on the canary bots (http://trac.webkit.org/changeset/68234). What exactly broke, and how can I make sure my patch doesn't break things too? It failed because of: import pdb pdb.set_trace() Which put the interpreter into interactive mode.
Comment on attachment 68690 [details] Patch Clearing flags on attachment: 68690 Committed r68268: <http://trac.webkit.org/changeset/68268>
All reviewed patches have been landed. Closing bug.
(In reply to comment #6) > Created an attachment (id=68690) [details] > Patch Yup, that'll work, although there's a lot more scaffolding than I would hope to have and it still doesn't test all the way through to make sure that every port is actually hitting the config file to pull the default (i.e., if ChromiumPort accidentally (?) defines a different default_configuration() we won't notice it in these tests). So, I'll keep hacking on the "right" fix but thanks for taking care of this for now. -- Dirk
This change apparently broke nacl_ui_tests in chromium: http://build.chromium.org/buildbot/waterfall/builders/NACL%20Tests/builds/17999
(In reply to comment #12) > This change apparently broke nacl_ui_tests in chromium: > > http://build.chromium.org/buildbot/waterfall/builders/NACL%20Tests/builds/17999 Thanks for letting me know. I've put up a fix at bug 46602.
(In reply to comment #13) > Thanks for letting me know. I've put up a fix at bug 46602. Landed as http://trac.webkit.org/changeset/68365.