Bug 152959 – [webkitdirs] Use CMake to determine architecture on isCMakeBuild()

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

[Konstantin Tokarev]

Use isCMakeBuild() condition instead of isEfl() || isGtk()

[Konstantin Tokarev]

Created attachment 268653 [details]
Patch

[Konstantin Tokarev]

Created attachment 268655 [details]
Patch

[Brent Fulgham]

Comment on attachment 268655 [details]
Patch

I think this looks good. I would like Alex to double-check, but if the Windows EWW passes I think it's appropriate to land this.

[Gyuyoung Kim]

(In reply to comment #3)
> Comment on attachment 268655 [details]
> Patch
> 
> I think this looks good. I would like Alex to double-check, but if the
> Windows EWW passes I think it's appropriate to land this.

LGTM too.

[Gyuyoung Kim]

Comment on attachment 268655 [details]
Patch

This patch passes win-ews which is built using cmake. I think this change is fine for win port as well.

[WebKit Commit Bot]

Comment on attachment 268655 [details]
Patch

Clearing flags on attachment: 268655

Committed r194849: <http://trac.webkit.org/changeset/194849>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

Comment on attachment 268655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268655&action=review

> Tools/Scripts/webkitdirs.pm:342
>          $host_processor = `cmake --system-information | grep CMAKE_SYSTEM_PROCESSOR`;

We must not be running this on Windows.  I don't think we require grep to be installed.  If this doesn't break anything, then I guess it can stay, but I don't think this was a beneficial change.

[Konstantin Tokarev]

Comment on attachment 268655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268655&action=review

>> Tools/Scripts/webkitdirs.pm:342
>>          $host_processor = `cmake --system-information | grep CMAKE_SYSTEM_PROCESSOR`;
> 
> We must not be running this on Windows.  I don't think we require grep to be installed.  If this doesn't break anything, then I guess it can stay, but I don't think this was a beneficial change.

If you want, I can fix it to avoid grep invocation, with pure perl.

[Alex Christensen]

(In reply to comment #9)
> If you want, I can fix it to avoid grep invocation, with pure perl.
Perl is definitely a requirement.  That would be nice, although I'm not sure what if anything that would fix.

[Brent Fulgham]

(In reply to comment #9)
> Comment on attachment 268655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268655&action=review
> 
> >> Tools/Scripts/webkitdirs.pm:342
> >>          $host_processor = `cmake --system-information | grep CMAKE_SYSTEM_PROCESSOR`;
> > 
> > We must not be running this on Windows.  I don't think we require grep to be installed.  If this doesn't break anything, then I guess it can stay, but I don't think this was a beneficial change.
> 
> If you want, I can fix it to avoid grep invocation, with pure perl.

Yes -- we must be running this and just not noticing the failure. Or, Cygwin 'grep' is getting used. We'd like to move off of Cygwin in the future, so making this a pure Perl implementation would be much better.

Could you do that as a separate bug, though?

[Michael Catanzaro]

One good solution would be to change this condition to:

isCMakeBuild() && !isWindows()

[Gyuyoung Kim]

Comment on attachment 268655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268655&action=review

>>>> Tools/Scripts/webkitdirs.pm:342
>>>>          $host_processor = `cmake --system-information | grep CMAKE_SYSTEM_PROCESSOR`;
>>> 
>>> We must not be running this on Windows.  I don't think we require grep to be installed.  If this doesn't break anything, then I guess it can stay, but I don't think this was a beneficial change.
>> 
>> If you want, I can fix it to avoid grep invocation, with pure perl.
> 
> Yes -- we must be running this and just not noticing the failure. Or, Cygwin 'grep' is getting used. We'd like to move off of Cygwin in the future, so making this a pure Perl implementation would be much better.
> 
> Could you do that as a separate bug, though?

I thought that win port is using Cygwin. Sorry for missing the plan. Anyway I just file a new bug for this instead - https://bugs.webkit.org/show_bug.cgi?id=152998.

[Alex Christensen]

(In reply to comment #13)
> I thought that win port is using Cygwin. Sorry for missing the plan.
No problem.  Most of our builders do use Cygwin, but we don't require it and we're trying to (slowly) move away from using it.  The WinCairo bot doesn't use Cygwin.

[Gyuyoung Kim]

(In reply to comment #14)
> (In reply to comment #13)
> > I thought that win port is using Cygwin. Sorry for missing the plan.
> No problem.  Most of our builders do use Cygwin, but we don't require it and
> we're trying to (slowly) move away from using it.  The WinCairo bot doesn't
> use Cygwin.

I see. I will support the plan as well.