Skip to content

Fix some issues with generated pkg-config file - #3569

Merged
carlosmn merged 2 commits into
libgit2:masterfrom
arthurschreiber:arthur/fix-windows-issues
Jan 13, 2016
Merged

Fix some issues with generated pkg-config file#3569
carlosmn merged 2 commits into
libgit2:masterfrom
arthurschreiber:arthur/fix-windows-issues

Conversation

@arthurschreiber

Copy link
Copy Markdown
Member

This is related to some issues we're running into when building rugged on windows with SSH support, but is relevant for other platforms as well.

See libgit2/rugged#559 and libgit2/rugged#558 for some further information.

Comment thread CMakeLists.txt Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure about this line here. I think it's wrong to specify this here, but maybe someone else knows better. 😄

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems reasonable to me since you're updating all the other variables here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I dropped this line again. the winhttp stub is only generated because mingw is missing an import library for winhttp, and the generated file/path will only be available during the compile phase of libgit2

@arthurschreiber
arthurschreiber force-pushed the arthur/fix-windows-issues branch from 7eec1db to b41b134 Compare January 6, 2016 15:13
@arthurschreiber

Copy link
Copy Markdown
Member Author

@jacquesg But the perl bindings link dynamically, not statically.

@jacquesg

jacquesg commented Jan 6, 2016

Copy link
Copy Markdown
Contributor

It definitely links libssh2 statically into the perl extension (which itself is a DLL/so) and uses the winhttp import library.

Here's a link to the Windows build: https://ci.appveyor.com/project/jacquesg/p5-git-raw

@carlosmn

carlosmn commented Jan 8, 2016

Copy link
Copy Markdown
Member

CoInitializeEx and CoCreateInstance live in ole32. We use them to figure out whether 'default' auth is available. It is however odd that we don't explicitly link to it. Perhaps one of the other libraries does so for us. But we should still link to it, since we use its functions.

Comment thread CMakeLists.txt Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This might cause the user to link to a different libssh2 than the we one libgit2 was provided with. I think we have this form as a reaction to an issue which was reported. Why change this to require?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This allows pkg-config to automatically identify the linker flags needed for libssh2 when statically linking against a libgit2 which was built with ssh support.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I found the commit explaining why we do this: 790cabf. If we put them in requires, pkg-config will put their private libs in the linker args as well, which we don't want. We want to give tell whoever is embedding libgit2 which options we would have used, were we to create the shared library.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Mh, yeah, that sounds reasonable. Why do we do this for openssl and zlib then?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it just comes down to them not having any extra dependencies. zlib doesn't have dependencies, and OpenSSL only has zlib, which we depend on anyway.

@arthurschreiber

Copy link
Copy Markdown
Member Author

But we should still link to it, since we use its functions.

I added another commit to fix this.

@arthurschreiber
arthurschreiber force-pushed the arthur/fix-windows-issues branch from 7662b9a to 3d57408 Compare January 9, 2016 01:42
@arthurschreiber
arthurschreiber force-pushed the arthur/fix-windows-issues branch from 3d57408 to 768e185 Compare January 9, 2016 01:43
@arthurschreiber

Copy link
Copy Markdown
Member Author

@carlosmn Okay, I changed the issues with this and cleaned up the history.

carlosmn added a commit that referenced this pull request Jan 13, 2016
Fix some issues with generated pkg-config file
@carlosmn
carlosmn merged commit 47cf739 into libgit2:master Jan 13, 2016
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.

5 participants