Bug 120870

Summary: GTest: add support for FreeBSD and Hurd
Product: WebKit Reporter: Alberto Garcia <berto>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, gustavo, pochu27, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch gustavo: review+

Description Alberto Garcia 2013-09-06 11:33:22 PDT
This is necessary for the FreeBSD and Hurd builds.
Comment 1 Alberto Garcia 2013-09-06 11:34:38 PDT
Created attachment 210775 [details]
Patch
Comment 2 WebKit Commit Bot 2013-09-06 11:37:18 PDT
Attachment 210775 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ChangeLog', u'Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h']" exit_code: 1
Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h:499:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alberto Garcia 2013-09-06 11:39:43 PDT
(In reply to comment #2)
> Attachment 210775 [details] did not pass style-queue:

That's the second line here:

-#if (GTEST_OS_LINUX || GTEST_OS_MAC || GTEST_OS_CYGWIN || GTEST_OS_SOLARIS || \
+#if (GTEST_OS_LINUX || GTEST_OS_FREEBSD || GTEST_OS_HURD || \
+     GTEST_OS_MAC || GTEST_OS_CYGWIN || GTEST_OS_SOLARIS || \
      (GTEST_OS_WINDOWS_DESKTOP && _MSC_VER >= 1400) || \
      GTEST_OS_WINDOWS_MINGW || GTEST_OS_AIX)

I decided to keep the same spacing as the rest of the expression, I
guess that's what makes sense here?
Comment 4 Emilio Pozuelo Monfort 2013-09-06 11:41:38 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Attachment 210775 [details] [details] did not pass style-queue:
> 
> That's the second line here:
> 
> -#if (GTEST_OS_LINUX || GTEST_OS_MAC || GTEST_OS_CYGWIN || GTEST_OS_SOLARIS || \
> +#if (GTEST_OS_LINUX || GTEST_OS_FREEBSD || GTEST_OS_HURD || \
> +     GTEST_OS_MAC || GTEST_OS_CYGWIN || GTEST_OS_SOLARIS || \
>       (GTEST_OS_WINDOWS_DESKTOP && _MSC_VER >= 1400) || \
>       GTEST_OS_WINDOWS_MINGW || GTEST_OS_AIX)
> 
> I decided to keep the same spacing as the rest of the expression, I
> guess that's what makes sense here?

gtest is an external project so we shouldn't follow webkit's style but be consistent with the rest of the file AFAIK.

Thanks for submitting this upstream!
Comment 5 Zan Dobersek 2013-09-06 11:42:36 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Attachment 210775 [details] [details] did not pass style-queue:
> 
> That's the second line here:
> 
> -#if (GTEST_OS_LINUX || GTEST_OS_MAC || GTEST_OS_CYGWIN || GTEST_OS_SOLARIS || \
> +#if (GTEST_OS_LINUX || GTEST_OS_FREEBSD || GTEST_OS_HURD || \
> +     GTEST_OS_MAC || GTEST_OS_CYGWIN || GTEST_OS_SOLARIS || \
>       (GTEST_OS_WINDOWS_DESKTOP && _MSC_VER >= 1400) || \
>       GTEST_OS_WINDOWS_MINGW || GTEST_OS_AIX)
> 
> I decided to keep the same spacing as the rest of the expression, I
> guess that's what makes sense here?

This is a third-party source code, so we shouldn't really enforce WebKit style guidelines in there.
Comment 6 Zan Dobersek 2013-09-06 11:46:46 PDT
(In reply to comment #4)
> gtest is an external project so we shouldn't follow webkit's style but be consistent with the rest of the file AFAIK.

Didn't see that, sorry for the extra noise.
Comment 7 Gustavo Noronha (kov) 2013-09-06 13:19:10 PDT
Comment on attachment 210775 [details]
Patch

Should probably submit that to Google, so it's not lost when it's synced from there?
Comment 8 Alberto Garcia 2013-09-06 13:30:26 PDT
Committed r155210: <http://trac.webkit.org/changeset/155210>
Comment 9 Alberto Garcia 2013-09-09 02:33:23 PDT
(In reply to comment #7)
> (From update of attachment 210775 [details])
> Should probably submit that to Google, so it's not lost when it's synced from there?

I guess so, what do you think Emilio?
Comment 10 Emilio Pozuelo Monfort 2013-09-09 02:48:01 PDT
Absolutely. Upstream is at https://code.google.com/p/googletest/source/browse/trunk/include/gtest/internal/gtest-port.h

I can have a look at it later this week, but feel free to beat me to it.