Summary: | [webkitdirs] Use CMake to determine architecture on isCMakeBuild() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Konstantin Tokarev <annulen> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, dbates, gyuyoung.kim, lforschler, mcatanzaro | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Konstantin Tokarev
2016-01-10 09:47:34 PST
Created attachment 268653 [details]
Patch
Created attachment 268655 [details]
Patch
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.
(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. 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.
Comment on attachment 268655 [details] Patch Clearing flags on attachment: 268655 Committed r194849: <http://trac.webkit.org/changeset/194849> All reviewed patches have been landed. Closing bug. 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. 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. (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. (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? One good solution would be to change this condition to: isCMakeBuild() && !isWindows() 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. (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. (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. |