nrwt: never finds binaries in the 'out' dir on chromium win
Created attachment 153560 [details] Patch
Tony, what do you think of this fix? Without this, if we build with ninja/winja on win, you always have to specify --build-directory, which is very annoying. We could do a more specific check for the config dir (or a binary), but it seems like this should be safe enough.
Comment on attachment 153560 [details] Patch This seems ok, but how much more invasive is it to check for Release/Debug? Then we wouldn't have to change the check order.
(In reply to comment #3) > (From update of attachment 153560 [details]) > This seems ok, but how much more invasive is it to check for Release/Debug? Then we wouldn't have to change the check order. Somewhat more invasive; the problem is that the _build_path() call that we use doesn't necessarily contain Release/Debug as the first arg and so we'd have to rework that function. Which might be a good thing regardless. Would you like me to work up a patch that does that so we can compare? I had half of one last night before I realized the difficulties and switched, so it wouldn't be that hard to do.
Sounds cleaner in the long run since Linux/Mac check sconsbuild (hah!) and xcodebuild first.
(In reply to comment #5) > Sounds cleaner in the long run since Linux/Mac check sconsbuild (hah!) and xcodebuild first. Yeah, although by the same token maybe I should move 'out' first everywhere?
Looks like it would be sufficient to pass self.get_option('configuration') to _static_build_path in chromium.py. (In reply to comment #6) > (In reply to comment #5) > > Sounds cleaner in the long run since Linux/Mac check sconsbuild (hah!) and xcodebuild first. > > Yeah, although by the same token maybe I should move 'out' first everywhere? I would rather have the bot configuration work by default and require people using a different configuration to manually adjust.
Created attachment 153584 [details] Patch
> (In reply to comment #6) > > (In reply to comment #5) > > > Sounds cleaner in the long run since Linux/Mac check sconsbuild (hah!) and xcodebuild first. > > > > Yeah, although by the same token maybe I should move 'out' first everywhere? > > I would rather have the bot configuration work by default and require people using a different configuration to manually adjust. Fair enough. In working on the second patch and getting all the tests to pass, it turns out that all the non-chromium ports were already including configuration in build_path (so that it wasn't passed in as part of *comps). It is clearly the right thing to do to match that so all the ports are implementing it consistently, so the second patch, allthough more involved, is the way to go.
Comment on attachment 153584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153584&action=review > Tools/ChangeLog:11 > + chromium-win's case, we look in src/build, which always exists because there > + are checked-in files in it, which means we'd always pick that Nit: weird indent > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:389 > + def _build_path(self, *comps, **kwargs): > + return self._static_build_path(self._filesystem, self.get_option('build_directory'), self.path_from_chromium_base(), self.path_from_webkit_base(), configuration=(kwargs.get('configuration') or self.get_option('configuration')), comps=comps) Rather than overloading this functions with kwargs, I think it would be more clear to add a new method _build_path_with_configuration(self, configuration, *comps).
(In reply to comment #10) > (From update of attachment 153584 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153584&action=review > > > Tools/ChangeLog:11 > > + chromium-win's case, we look in src/build, which always exists because there > > + are checked-in files in it, which means we'd always pick that > > Nit: weird indent > > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:389 > > + def _build_path(self, *comps, **kwargs): > > + return self._static_build_path(self._filesystem, self.get_option('build_directory'), self.path_from_chromium_base(), self.path_from_webkit_base(), configuration=(kwargs.get('configuration') or self.get_option('configuration')), comps=comps) > > Rather than overloading this functions with kwargs, I think it would be more clear to add a new method _build_path_with_configuration(self, configuration, *comps). Ok.
Created attachment 153591 [details] add _build_path_with_configuration
Created attachment 153594 [details] fix changelog
Comment on attachment 153594 [details] fix changelog View in context: https://bugs.webkit.org/attachment.cgi?id=153594&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:83 > - def _static_build_path(cls, filesystem, build_directory, chromium_base, webkit_base, *comps): > + def _static_build_path(cls, filesystem, build_directory, chromium_base, webkit_base, configuration, comps): > if build_directory: > return filesystem.join(build_directory, *comps) How does this work without the * in front of comps? > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:393 > + return self._static_build_path(self._filesystem, self.get_option('build_directory'), self.path_from_chromium_base(), self.path_from_webkit_base(), configuration=configuration, comps=comps) Why are configuration and comps named? Shouldn't comps be *comps?
Comment on attachment 153594 [details] fix changelog r- for now, but feel free to r? again if I'm missing something about how tuple args work.
(In reply to comment #14) > (From update of attachment 153594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153594&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:83 > > - def _static_build_path(cls, filesystem, build_directory, chromium_base, webkit_base, *comps): > > + def _static_build_path(cls, filesystem, build_directory, chromium_base, webkit_base, configuration, comps): > > if build_directory: > > return filesystem.join(build_directory, *comps) > > How does this work without the * in front of comps? > taking a tuple and putting the * in front of it basically expands the argument into a varargs list. > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:393 > > + return self._static_build_path(self._filesystem, self.get_option('build_directory'), self.path_from_chromium_base(), self.path_from_webkit_base(), configuration=configuration, comps=comps) > > Why are configuration and comps named? Shouldn't comps be *comps? no particular reason for the names; I actually thought they were all named but I obviously didn't check closely. cf. above for comps vs *comps. I don't like to use the vararg positional parameters in a function that has so many other parameters as well; that said, the change was done originally because of the interaction with the kwargs (since configuration came after comps), so I could back it out now. let me know which you prefer.
Comment on attachment 153594 [details] fix changelog View in context: https://bugs.webkit.org/attachment.cgi?id=153594&action=review >>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:83 >>> return filesystem.join(build_directory, *comps) >> >> How does this work without the * in front of comps? > > taking a tuple and putting the * in front of it basically expands the argument into a varargs list. Ah, right, because we're passing it to a function. >>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:393 >>> + return self._static_build_path(self._filesystem, self.get_option('build_directory'), self.path_from_chromium_base(), self.path_from_webkit_base(), configuration=configuration, comps=comps) >> >> Why are configuration and comps named? Shouldn't comps be *comps? > > no particular reason for the names; I actually thought they were all named but I obviously didn't check closely. cf. above for comps vs *comps. I don't like to use the vararg positional parameters in a function that has so many other parameters as well; that said, the change was done originally because of the interaction with the kwargs (since configuration came after comps), so I could back it out now. > > let me know which you prefer. I would remove the explicit naming since they are not optional parameters and we don't normally name non-optional params.
Created attachment 153837 [details] patch for landing
Committed r123360: <http://trac.webkit.org/changeset/123360>
Reverted r123360 for reason: broke the chromium bots Committed r123390: <http://trac.webkit.org/changeset/123390>
Created attachment 154145 [details] Patch
Committed r123530: <http://trac.webkit.org/changeset/123530>