Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: disable pthreads on GTest project top-level when using MinGW #8

Closed
wants to merge 1 commit into from
Closed

Conversation

andoks
Copy link

@andoks andoks commented Aug 3, 2016

in commit a634042 "Implement threading
support for gtest on Windows", testing of internal threading in google
test does not work on MinGW. The commit itself contains a comment
indicating that pthreads is not to be used under MinGW due to

// On MinGW, we can have both GTEST_OS_WINDOWS and GTEST_HAS_PTHREAD
// defined, but we don't want to use MinGW's pthreads implementation which
// has conformance problems with some versions of the POSIX standard.

The first comment on google#363 might indicate the reason for this breakage.

This commit assures that pthreads is disabled on a top-level by cmake

in commit a634042 "Implement threading
support for gtest on Windows", testing of internal threading in google
test does not work on MinGW. The commit itself contains a comment
indicating that pthreads is not to be used under MinGW due to

> // On MinGW, we can have both GTEST_OS_WINDOWS and GTEST_HAS_PTHREAD
> // defined, but we don't want to use MinGW's pthreads implementation which
> // has conformance problems with some versions of the POSIX standard.

The first comment on google#363 might indicate the reason for this breakage.

This commit assures that pthreads is disabled on a top-level by cmake
@ruslo
Copy link

ruslo commented Aug 3, 2016

@andoks This is not related to Hunter so you should send this patch upstream.

@andoks
Copy link
Author

andoks commented Aug 3, 2016

Ok. There are some discussions going on regarding this issue. But as it has been broken since a year ago, I thought it might be nice to have a workaround in hunter until it has been fixed in upstream.

@andoks andoks closed this Aug 3, 2016
@ruslo
Copy link

ruslo commented Aug 3, 2016

I thought it might be nice to have a workaround in hunter until it has been fixed in upstream

In Hunter for MinGW toolchain used previous version of GTest that works fine: https://ci.appveyor.com/project/ingenue/hunter/build/1.0.724

There are some discussions going on regarding this issue

According to the comment used approach doesn't work for some version of MinGW so this is why patch not accepted as far as I understand. It should be fixed appropriately.

@andoks
Copy link
Author

andoks commented Aug 3, 2016

The problem with the older version of GTest used in hunter, is that GMock is not available as far as I can see.

@ruslo
Copy link

ruslo commented Aug 3, 2016

The problem with the older version of GTest used in hunter, is that GMock is not available

@andoks Oh... I see. Fine, if you manage to create version that will not break current CI state of GTest I'm okay with merging it.

Though if you can work on appropriate patch and merge it upstream it's always better.

@andoks
Copy link
Author

andoks commented Aug 3, 2016

The problem with the older version of GTest used in hunter, is that GMock is not available

@andoks Oh... I see. Fine, if you manage to create version that will not break current CI state of GTest I'm okay with merging it.

https://github.com/ingenue/hunter/branches/all?utf8=%E2%9C%93&query=pkg.gtest
https://ci.appveyor.com/project/ingenue/hunter/build/1.0.724
https://travis-ci.org/ingenue/hunter/builds/147497136

Great! I'll try to get it past CI

Though if you can work on appropriate patch and merge it upstream it's always better.

I wholeheartedly agree.

@andoks andoks reopened this Aug 3, 2016
@andoks
Copy link
Author

andoks commented Aug 24, 2016

google#721 has been merged in GTest upstream, and should make by work-around obsolete.

Note that I haven't tested it.

@ruslo
Copy link

ruslo commented Aug 24, 2016

should make by work-around obsolete

I can close this one?

@andoks
Copy link
Author

andoks commented Aug 25, 2016

I'll do it for you ;-)

I created #9 to track when the fix is imported. Hope that's ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants