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 librhash/1.3.8 #277

Merged
merged 1 commit into from
Jan 29, 2020
Merged

Add librhash/1.3.8 #277

merged 1 commit into from
Jan 29, 2020

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Nov 2, 2019

Specify library name and version: librhash/1.3.8

Tested on Linux 32- and 64-bit

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot
Copy link
Collaborator

Some configurations of 'librhash/1.3.8' have failed:

  • Linux x86_64, Release, gcc 5, libstdc++ . Options: librhash:shared-True
    • Hooks errors detected:
      • [HOOK - conan-center.py] post_package(): ERROR: [PACKAGE LICENSE (KB-H012)] No 'licenses' folder found in package: /tmp/c3ipr/pr_277_1_0_0/.conan/data/librhash/1.3.8/_/_/package/b34df74c5d19404b38553e37011cc9839b6cc02c (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H012)
    • Log download
    • Log error Download
    • Profile
  • Access to all the logs

@conan-center-bot
Copy link
Collaborator

Some configurations of 'librhash/1.3.8' have failed:

@madebr madebr force-pushed the librhash branch 2 times, most recently from 12ddebb to 7543db1 Compare November 2, 2019 19:31
@conan-center-bot
Copy link
Collaborator

Some configurations of 'librhash/1.3.8' have failed:

  • Linux x86_64, Release, gcc 5, libstdc++ . Options: librhash:shared-True
    • Hooks errors detected:
      • [HOOK - conan-center.py] post_package(): ERROR: [CMAKE-MODULES-CONFIG-FILES (KB-H016)] The conan-center repository doesn't allow the packages to contain CMake find modules or config files. The packages have to be located using generators and the declared cpp_infoinformation (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H016)
      • [HOOK - conan-center.py] post_package(): ERROR: [CMAKE-MODULES-CONFIG-FILES (KB-H016)] Found files:
    • Log download
    • Log error Download
    • Profile
  • Access to all the logs

@madebr madebr force-pushed the librhash branch 2 times, most recently from aad0afc to 2095881 Compare November 2, 2019 20:05
@conan-center-bot
Copy link
Collaborator

All green! 😊

@conan-center-bot
Copy link
Collaborator

All green! 😊

SSE4
SSE4 previously approved these changes Nov 4, 2019
@@ -0,0 +1,102 @@
cmake_minimum_required(VERSION 3.1.2)
Copy link
Member

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?

Copy link
Contributor Author

@madebr madebr Nov 4, 2019

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.

Copy link
Member

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

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

ping @madebr

Copy link
Contributor Author

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.

recipes/librhash/all/conanfile.py Show resolved Hide resolved
recipes/librhash/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot
Copy link
Collaborator

All green! 😊

@danimtb danimtb requested a review from SSE4 November 7, 2019 16:09
set(LIBRHASH_CMAKE_NAMESPACE LibRHash::)

add_library(${LIBRHASH_LIBRARY}
"${SOURCE_SUBFOLDER}/calc_sums.c"
Copy link
Member

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

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 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.

Copy link
Contributor Author

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?

@conan-center-bot
Copy link
Collaborator

All green! 😊

SSE4
SSE4 previously approved these changes Nov 27, 2019
@conan-center-bot
Copy link
Collaborator

All green! 😊

@madebr madebr force-pushed the librhash branch 2 times, most recently from ae187c6 to 2593688 Compare November 27, 2019 14:28
@conan-center-bot
Copy link
Collaborator

All green! 😊

@conan-center-bot
Copy link
Collaborator

All green! 😊

@conan-center-bot
Copy link
Collaborator

All green! 😊

@conan-center-bot
Copy link
Collaborator

Some configurations of 'librhash/1.3.9' failed in build 12 (c558869ba84a940842b0dd6d7f67a51d6b57861a):

@conan-center-bot
Copy link
Collaborator

Some configurations of 'librhash/1.3.9' failed in build 13 (c558869ba84a940842b0dd6d7f67a51d6b57861a):

@conan-center-bot
Copy link
Collaborator

Some configurations of 'librhash/1.3.9' failed in build 14 (6eeb1efcf32bcbbe047a240eb54a4e77235e40e1):

@conan-center-bot
Copy link
Collaborator

All green in build 15 (afb731cb0dd3614a0101737f4e228c2557bd7f7e)! 😊

@SSE4
Copy link
Contributor

SSE4 commented Jan 28, 2020

@uilianries I think it should be good now (all comments were addressed) - let's move forward this one

@danimtb danimtb self-assigned this Jan 29, 2020
@danimtb danimtb merged commit 94cdd67 into conan-io:master Jan 29, 2020
@madebr madebr deleted the librhash branch January 29, 2020 15:55
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