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

Enable LINK_STATIC_LIBPROTOBUF option on MINGW #83

Merged

Conversation

msakai
Copy link
Member

@msakai msakai commented Sep 3, 2018

This PR enables LINK_STATIC_LIBPROTOBUF option on MINGW and adds a AppVeyor job for building a binary that statically links libprotobuf, libstdc++, and libgcc.

This PR is intended to be merged after #82 is merged, since this branch is forked from that branch.

This is because protobuf-2.6.1 is too old that bundled config.guess
cannot detect MINGW+MSYS2 environment.
@msakai msakai requested a review from okapies September 3, 2018 01:22
Copy link
Member

@okapies okapies left a comment

Choose a reason for hiding this comment

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

Thanks! I'd like to note that the statically linked binary still requires external libstdc++ at the moment because mkldnn requires it. I'm waiting for mkldnn's static linking support. Do you want to merge it anyway?

set(PROTOBUF_URL "https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOBUF_VERSION}/protobuf-${PROTOBUF_VERSION}.tar.gz")
set(PROTOBUF_HASH MD5=f3916ce13b7fcb3072a1fa8cf02b2423)
if(UNIX OR MINGW)
set(PROTOBUF_VERSION_STATIC "3.6.1")
Copy link
Member

Choose a reason for hiding this comment

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

I faced this issue when using 3.6.1 in my environment (3.5.1 seems to work). Could you check that built binaries work in your environment?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, 3.6.1 seems to work in Ubuntu 16.04.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, menoh_test succeeded on Travis-CI and Appveyor, but I didn't tested it on other environments.

BTW:
If a desirable version of protobuf is changed, I will update PR #82 and rebase this PR on top of it.

@msakai
Copy link
Member Author

msakai commented Sep 3, 2018

Yes, I have noticed that mkldnn still depends on external libstdc++.
I linked libstdc++ statically mainly for testing if the option works as expected.
But please feel free to modify it for the sake of building binary package.

Copy link
Member

@okapies okapies left a comment

Choose a reason for hiding this comment

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

Ok, I'll implement LINK_STATIC_LIBMKLDNN later and I think we can merge this PR now.

@msakai
Copy link
Member Author

msakai commented Sep 3, 2018

Thanks!
I'm going to merge the PR.
If there are problems with protobuf 3.6.1, let's change the version later.

@msakai msakai merged commit da9348d into pfnet-research:master Sep 3, 2018
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.

2 participants