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

AppVeyor MinGW-w64 test build #870

Merged
merged 3 commits into from
Sep 1, 2017

Conversation

KindDragon
Copy link
Contributor

@KindDragon KindDragon commented Sep 4, 2016

Tests disabled for mingw64

AppVeyor should pass after merging PR #856

appveyor.yml Outdated
- Debug
- Release
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the release build was commented out because of slow builds on appveyor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disabled this configuration when created appveyor.yml. Now I think it's better to check both configurations

@gennadiycivil
Copy link
Contributor

Can we get back to this? May be useful for issue detection

@gennadiycivil
Copy link
Contributor

This is old and needs to be rebased

@KindDragon
Copy link
Contributor Author

Rebased

@gennadiycivil
Copy link
Contributor

One thing I noticed that this takes longer to build on AppVeyor, take a look at https://ci.appveyor.com/project/GoogleTestAppVeyor/googletest/build/522. Why is this taking so long?

@KindDragon
Copy link
Contributor Author

I can disable Release configuration as before if you want

@gennadiycivil
Copy link
Contributor

Lets try and see .. this is useful but we cant be waiting this long each time

@gennadiycivil
Copy link
Contributor

Apveyor Build Failed

@KindDragon KindDragon force-pushed the mingw64-appveyour branch 2 times, most recently from e55fbe5 to afcce71 Compare August 29, 2017 14:43
@KindDragon
Copy link
Contributor Author

Fixed

@gennadiycivil
Copy link
Contributor

This is sadly out of date now and there are merge conflicts.
Please feel free to resolve the merge conflicts and we could take a look

@KindDragon
Copy link
Contributor Author

Done

@KindDragon
Copy link
Contributor Author

@gennadiycivil Can you review this PR? I rebase this branch every day for this

@gennadiycivil
Copy link
Contributor

gennadiycivil commented Aug 31, 2017

Sorry for the delay.
So this is adding one more build, correct? I am not sure why all the changes in the .yml file? Please explain.

- Toolset: v120
- Toolset: v110
- Toolset: v100
- compiler: msvc-14-seh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we are swtiching from matrix to actually spelling out which build are going to go
Why?
Cant we just one more toolset? /I am not very familiar with AppVeyor so forgive me if this is silly/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored created by me appveyor.yml because of old file use platform variable, but it not useful for MinGW.

I just used my file from google/benchmark google/benchmark@3336ea0

@gennadiycivil
Copy link
Contributor

looks good to me. I am a little concerned with additional execution time but we shall see.

@gennadiycivil gennadiycivil merged commit 69e794c into google:master Sep 1, 2017
@KindDragon KindDragon deleted the mingw64-appveyour branch September 1, 2017 10:50
@KindDragon
Copy link
Contributor Author

I am a little concerned with additional execution time but we shall see.

It's almost same:
https://ci.appveyor.com/project/GoogleTestAppVeyor/googletest/build/601 35 min 1 sec
https://ci.appveyor.com/project/GoogleTestAppVeyor/googletest/build/603 34 min 24 sec

@gennadiycivil
Copy link
Contributor

Awesome, totally worth it

@ghost
Copy link

ghost commented Sep 1, 2017

But note that the Mingw builds don't include Google Test's unit tests, what makes them so fast.

@gennadiycivil gennadiycivil mentioned this pull request Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants