Bug 91890 – nrwt: never finds binaries in the 'out' dir on chromium win

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: never finds binaries in the 'out' dir on chromium win

[Dirk Pranke]

Created attachment 153560 [details]
Patch

[Dirk Pranke]

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.

[Tony Chang]

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.

[Dirk Pranke]

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

[Tony Chang]

Sounds cleaner in the long run since Linux/Mac check sconsbuild (hah!) and xcodebuild first.

[Dirk Pranke]

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

[Tony Chang]

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.

[Dirk Pranke]

Created attachment 153584 [details]
Patch

[Dirk Pranke]

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

[Tony Chang]

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

[Dirk Pranke]

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

[Dirk Pranke]

Created attachment 153591 [details]
add _build_path_with_configuration

[Dirk Pranke]

Created attachment 153594 [details]
fix changelog

[Tony Chang]

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?

[Tony Chang]

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.

[Dirk Pranke]

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

[Tony Chang]

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.

[Dirk Pranke]

Created attachment 153837 [details]
patch for landing

[Dirk Pranke]

Committed r123360: <http://trac.webkit.org/changeset/123360>

[Dirk Pranke]

Reverted r123360 for reason:

broke the chromium bots

Committed r123390: <http://trac.webkit.org/changeset/123390>

[Dirk Pranke]

Created attachment 154145 [details]
Patch

[Dirk Pranke]

Committed r123530: <http://trac.webkit.org/changeset/123530>