-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 librhash/1.3.8 #277
Add librhash/1.3.8 #277
Conversation
Some configurations of 'librhash/1.3.8' have failed:
|
Some configurations of 'librhash/1.3.8' have failed:
|
12ddebb
to
7543db1
Compare
Some configurations of 'librhash/1.3.8' have failed:
|
aad0afc
to
2095881
Compare
All green! 😊
|
All green! 😊
|
recipes/librhash/all/CMakeLists.txt
Outdated
@@ -0,0 +1,102 @@ | |||
cmake_minimum_required(VERSION 3.1.2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this migration to a custom CMake file to build this library? Shouldn't we add a comment here about it? Would users be concerned of this change as something critical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
librhash provides a configure script that works fine on Linux.
For Visual Studio, it provides a vcxproj file, that does enable setting the Visual Studio compiler runtime or shared/static library option.
The vcxproj itself looks very generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this library doesn't support visual studio or doesn't support shared or specific runtime, I would make those configurations as invalid instead of creating a new CMake file for it. Completely changing the build system to a library is a major change IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still think this is a very important change. @SSE4 @uilianries WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I investigated librhash yesterday, the project is simple in terms of build. I don't like keeping a custom cmake file here if we can invoke autotools and msbuild, so I would prefer dropping any invalid configuration, if we can't use MSBuild. However, I opened a PR to author (rhash/RHash#103), offering CMake support, which can be used by any user, including in our recipes. In summary, use autotools and msbuild for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @madebr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uilianries There is already a cmake pr rhash/RHash#71
I don't see them accepting a pr in the near future.
All green! 😊
|
recipes/librhash/all/CMakeLists.txt
Outdated
set(LIBRHASH_CMAKE_NAMESPACE LibRHash::) | ||
|
||
add_library(${LIBRHASH_LIBRARY} | ||
"${SOURCE_SUBFOLDER}/calc_sums.c" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not confident using this part for a generic recipe, it may change on a next version. You can try something similar what vcpkg did: https://github.com/microsoft/vcpkg/blob/master/ports/rhash/CMakeLists.txt They extract the source file list from the Makefile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think vcpkg is wrong here because it is extracting the SOURCES
variable where I think it should extract LIBRHASH_FILES
.
The files in the SOURCES
variable are for the rhash
executable.
I changed it into something similar but I do the parsing in the conanfile.py
recipe because the regex would become too complicated (files are spread over multiple lines).
I checked rhash/RHash#71, where they also make the distinction between files in the root folder and files in the librhash subfolder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, I am passing the sources as an argument to the cmake file during configuration.
Would this be fine? Generating a CMakeLists.txt
during source()
that contains the sources and add that file to the recipe. Or would you generate this cmake file during the build step? Or is the current approach fine?
All green! 😊
|
All green! 😊
|
ae187c6
to
2593688
Compare
All green! 😊
|
All green! 😊
|
All green! 😊
|
Some configurations of 'librhash/1.3.9' failed in build 12 (
|
Some configurations of 'librhash/1.3.9' failed in build 13 (
|
Some configurations of 'librhash/1.3.9' failed in build 14 (
|
All green in build 15 (
|
@uilianries I think it should be good now (all comments were addressed) - let's move forward this one |
Specify library name and version: librhash/1.3.8
Tested on Linux 32- and 64-bit
conan-center hook activated.