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

MinGW port #64

Merged
merged 10 commits into from
Mar 31, 2015
Merged

MinGW port #64

merged 10 commits into from
Mar 31, 2015

Conversation

mattyclarkson
Copy link
Contributor

So I've patched up the library to compile with gcc 4.9.2:

cmake -g "MinGW Makefiles"
mingw32-make

I've just gone through and fixed up the compiler warnings/errors at the moment. I've tried to make the commits as fine grained as possible in case you would like to git cherry-pick them onto master.

The library doesn't seem to currently work due to hangs inside WaitForMultipleObjects in the Windows kernel. The backtrace looks like it is stalling inside the MinGW implementation of pthread_cond_init which is wrapped by std::condition_variable. Thought I'd publish the branch in case anyone can provide any pointers?

I'll try to move forward with getting it compiling with VS2013 Express.

@dmah42
Copy link
Member

dmah42 commented Nov 13, 2014

I have no way to test this so I can only trust that it's an improvement over the current state (and not a regression for other platforms).

It seems to build for me, but I'm going to wait for #66 to land to get this PR run through the full matrix, if you don't mind.

@mattyclarkson
Copy link
Contributor Author

@dominichamon, #66 will be great to get in! I'll wait for that to touchdown before continuing with this. I've tested the patches on Arch, Windows. unfortunately I don't have access to MacOS so can't test that. Having travis do that would be great. Does travis support Windows?

@dmah42
Copy link
Member

dmah42 commented Nov 14, 2014

I wonder if I comment if it will bump the PR and kick off a travis build...

@@ -6,7 +6,9 @@
!/cmake/*.cmake
*~
/test/benchmark_test
/test/benchmark_test.exe
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need these? I don't think we should be building in-tree like this.
Either way the stuff already seems to be there for Unix so I guess this is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some people do build in-tree, though, even if it's not the best idea...

Me, I'd make a different comment: this should be a *.exe entry, the same way object files are handled.

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 think we shouldn't build in tree. I tried to follow the current .gitignore style that was specifically adding the built executables. I think a catchall *.exe would be much more useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically adding the built executables makes sense for Unix platforms that don't have an executable extensions, but no need to duplicate this if there's an extension, just add a catchall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for the advice.

@mattyclarkson
Copy link
Contributor Author

That branch builds on Windows 8.1 and passes all tests with x86_64-4.9.2-release-posix-seh-rt_v4-rev2 using the following commands:

cmake -G "MinGW Makefiles" -DCMAKE_BUILD_TYPE=DEBUG
mingw32-make

It would be cool to set up appveyor to build this on Windows continuously. Any thoughts on that?

@mattyclarkson
Copy link
Contributor Author

Just having a go at setting up appveyor on my personal repo. Will create a PR on top of this once it is merged.

@dmah42
Copy link
Member

dmah42 commented Mar 27, 2015

i don't know what appveyor is, but anything that builds windows continuously would be great.

#ifdef OS_WINDOWS
#include <Windows.h>
#endif

namespace benchmark {
#ifdef OS_WINDOWS
// Window's _sleep takes milliseconds argument.
Copy link
Member

Choose a reason for hiding this comment

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

nit: update the comment please

@mattyclarkson
Copy link
Contributor Author

@dominichamon, it's a cloud based windows build service (it's free for open source projects). I'm most of the way with the integration - should be able to finish it up on Monday. Would be really good to get that in!

@dmah42 dmah42 added this to the v0.2.0 milestone Mar 27, 2015
@mattyclarkson
Copy link
Contributor Author

The appveyor stuff has taken much longer than I thought it would - I needed to write a script that could download a specific version of MinGW depending on the version, threading model and exception model. The appveyor YAML file took a bit of cajoling but got there in the end. I'll push up the final changes today to the nt branch - I'm currently just working on the appveyor branch.

@mattyclarkson
Copy link
Contributor Author

Got the build working for mingw-4.9.2-posix-seh, Now just building up the matrix to cover some other common build situations.

We use the SHGetValueA on Windows to retrieve the MHz of the processor
but this requires the shlwapi library. Previous to this patch the
library was linked with a MSVC specific pragma but there is no
guarantee that on Windows we will be using MSVC. Therefore, it is much
compile agnostic to use the standard CMAKE library linking mechanism
to provide the definition of SHGetValueA
For cross platform and cross compiler portability we use the
standard integer type for a 64-bit integer. MinGW on Windows doesn't
have the definition for `int64`.
The children CPU usage doesn't seem to have a equivalent on NT systems
so it just returns zero.
The python script provides a method to get the repository of mingw-builds
gcc compilers and download one of them. This is useful for providing a
matrix of compilations on appveyor.

The versions of compilers are seperated by multiple things:

  - version
  - threading model
  - exception model
  - revision

All four of those things need to be matched if using the libraries built
by MinGW. The script allows you to specify all of those variations. If
a variation isn't defined it picks the most common or latest settings.

For example, if the version isn't specified the latest will be selected
and if the exception model isn't defined then the zero exception model
(seh) will be selected if available.
This file provides scripting to build the benchmark library in the cloud
on the appveyor build system. It provides a matrix of configurations to
cover as many possibilities as it can. Eventually MSVC can be added to the
matrix to provide coverage of the Visual Studio solutions.
@mattyclarkson
Copy link
Contributor Author

I edited the history to resolve your comments @dominichamon.

I've pushed up the appveyor.yml configuration file on top of the MinGW port commits. It currently is conservative with what it compiles for as the build times are lengthy due to downloading a specific version of MinGW. You can see a completed build here. I've told appveyor to cache the download of the MinGW compiler so that should decrease build times. I cannot set up the appveyor for google/benchmark as I don't have the necessary admin rights to give to appveyor to set up the web hooks. Would it be possible for one of the benchmark team to set this up?

@dmah42
Copy link
Member

dmah42 commented Mar 31, 2015

Nice work! I'll take a look at the patch.

I don't know if I have permission either - it may have to be a Google org admin. I'll poke around.

root_dir = root(location = args.location, arch = args.arch,
version = args.version, threading = args.threading,
exceptions = args.exceptions, revision = args.revision,
log = logger)
Copy link
Member

Choose a reason for hiding this comment

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

given you always pass this, you can kill the EmptyLogger above.

honestly, you can probably make the logger global and not even pass it through everywhere :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasons it's there is so someone can use that python module as a library and turn logging on (or leave it off) when using the methods individually. For example I can then do:

import mingw
versions = mingw.repository()
root = mingw.root(version = (4, 9, 2))

And that will give me all the mingw operations without logging anything or I could pass my own logger in.

That being said - completely happy to remove all that stuff if you don't want it in there! I tried to make the script as reusable as possible, if people want to use it in other projects for appveyor support or integrate it into python based tool chains.

Copy link
Member

Choose a reason for hiding this comment

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

fair enough. i tend to err on the side of less code, minimal functionality, to avoid any subtle issues.

however, this isn't on the critical path so it seems reasonable.

@mattyclarkson
Copy link
Contributor Author

Cool - if the appveyor backend can get set up I can tune the configuration file to give speedy builds for MinGW.

@dmah42
Copy link
Member

dmah42 commented Mar 31, 2015

I also don't have access to the google-level appveyor. i'll ask around but in the meantime we should get this merged (assuming you're happy with it).

dmah42 pushed a commit that referenced this pull request Mar 31, 2015
@dmah42 dmah42 merged commit 65ed470 into google:master Mar 31, 2015
@mattyclarkson
Copy link
Contributor Author

👍

@tamarlev
Copy link

Im currently trying to compile google benchmark for MingW and failing for c++11 features as mutex and threads, do you know how to resolve that?

@dmah42
Copy link
Member

dmah42 commented Jan 12, 2018 via email

@mattyclarkson
Copy link
Contributor Author

@tamarlev you can use the downloading script to get the correct MinGW version in this repository.

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

Successfully merging this pull request may close these issues.

None yet

6 participants