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

add cmake Find/Use for system libraries #160

Merged
merged 1 commit into from
Apr 29, 2016
Merged

add cmake Find/Use for system libraries #160

merged 1 commit into from
Apr 29, 2016

Conversation

rainbreak
Copy link
Contributor

Will allow us to more easily reason with system dependencies, with the aim of allowing full static linking.

@rainbreak rainbreak mentioned this pull request Apr 29, 2016
5 tasks
@rainbreak
Copy link
Contributor Author

rainbreak commented Apr 29, 2016

The only problem with this one is in UseZLIB:

if (ZLIB_FOUND)
        target_include_directories(${TARGET} SYSTEM PUBLIC ${ZLIB_INCLUDE_DIRS})
        # target_link_libraries(${TARGET} ${ZLIB_LIBRARIES})
        target_link_libraries(${TARGET} z)

The commented out target_link_lib line is what I think it should be, but the actual line is what worked for me (Alpine Linux).

The problem with the commented line is that it tries to link -lzlib which isn't valid. The uncommented line gives us -lz as desired.

In principle this should all be handled in FindZLIB, which is included with cmake and has set(ZLIB_NAMES z zlib zdll zlib1).

@bobsummerwill bobsummerwill merged commit 47b4803 into ethereum:develop Apr 29, 2016
@rainbreak rainbreak deleted the add-cmake-finders branch April 29, 2016 21:18
@bobsummerwill
Copy link
Contributor

RE: zlib. Right - thanks for the info.

Well, it can stick as it is for now, because we need to get better conditionals higher up before we even hit this issue in the code which is currently committed. Maybe have a Google around for this one, which I would imagine other people have hit too.

"CMake zlib library name inconsistent" or whatever?

Or we just hardcode it as you have, with some comment on why.

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