Skip to content

Add librhash/1.3.8 - #277

Merged
danimtb merged 1 commit into
conan-io:masterfrom
madebr:librhash
Jan 29, 2020
Merged

Add librhash/1.3.8#277
danimtb merged 1 commit into
conan-io:masterfrom
madebr:librhash

Conversation

@madebr

@madebr madebr commented Nov 2, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

All green! 😊

@conan-center-bot

Copy link
Copy Markdown
Contributor

All green! 😊

SSE4
SSE4 previously approved these changes Nov 4, 2019
Comment thread recipes/librhash/all/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.

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?

@madebr madebr Nov 4, 2019

Copy link
Copy Markdown
Contributor Author

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

Comment thread recipes/librhash/all/conanfile.py Outdated
Comment thread recipes/librhash/all/conanfile.py Outdated
@conan-center-bot

Copy link
Copy Markdown
Contributor

All green! 😊

@danimtb
danimtb requested a review from SSE4 November 7, 2019 16:09
Comment thread recipes/librhash/all/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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

All green! 😊

Comment thread recipes/librhash/all/conanfile.py Outdated
SSE4
SSE4 previously approved these changes Nov 27, 2019
@conan-center-bot

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

All green! 😊

@conan-center-bot

Copy link
Copy Markdown
Contributor

All green! 😊

@conan-center-bot

Copy link
Copy Markdown
Contributor

All green! 😊

@danimtb
danimtb requested review from SSE4 and uilianries December 20, 2019 09:53
Comment thread recipes/librhash/all/conanfile.py Outdated
@conan-center-bot

Copy link
Copy Markdown
Contributor

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

@conan-center-bot

Copy link
Copy Markdown
Contributor

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

@conan-center-bot

Copy link
Copy Markdown
Contributor

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

@conan-center-bot

Copy link
Copy Markdown
Contributor

All green in build 15 (afb731cb0dd3614a0101737f4e228c2557bd7f7e)! 😊

@SSE4

SSE4 commented Jan 28, 2020

Copy link
Copy Markdown
Contributor

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

@danimtb
danimtb requested a review from uilianries January 28, 2020 17:11
@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