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

Adds rapidcheck gtest/catch integrations #7394

Merged
merged 12 commits into from
Sep 28, 2021

Conversation

mjvankampen
Copy link
Contributor

@mjvankampen mjvankampen commented Sep 23, 2021

Specify library name and version: rapidcheck

Rapidcheck provides integration with multiple unit test frameworks. This enables those integrations within the conan package


  • 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

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@mjvankampen
Copy link
Contributor Author

Excuse the mess, not my finest moment

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

SSE4
SSE4 previously approved these changes Sep 24, 2021
uilianries
uilianries previously approved these changes Sep 24, 2021
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

The component names do not match the official ones 😕

https://github.com/emil-e/rapidcheck/search?q=rapidcheckConfig

  • core --> rapidcheck
  • the frameworks should be prefixed with rapidcheck_

Also why are there no require statements? I would expect "enable_catch" to include catch..
https://github.com/emil-e/rapidcheck/blob/fc616114a95a3198514f8b4ec9e354865fd6ecfc/extras/catch/include/rapidcheck/catch.h#L6

Would fail to compile without it

recipes/rapidcheck/all/conanfile.py Outdated Show resolved Hide resolved
Comment on lines 23 to 27
"enable_catch": [True, False],
"enable_gmock": [True, False],
"enable_gtest": [True, False],
"enable_boost": [True, False],
"enable_boost_test": [True, False]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be one option?

Suggested change
"enable_catch": [True, False],
"enable_gmock": [True, False],
"enable_gtest": [True, False],
"enable_boost": [True, False],
"enable_boost_test": [True, False]
"enable_framework": [False, "catch", "gmock", "gtest", "boost", "boost_test"],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think so as you can enable multiple integrations at the same time.

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@mjvankampen mjvankampen dismissed stale reviews from uilianries and SSE4 via 74c6beb September 25, 2021 02:50
@mjvankampen
Copy link
Contributor Author

mjvankampen commented Sep 25, 2021

Requires: I see, catch and gmock are handled differently from the rest. F.e. gtest requires the consumer to link gtest, include gtest header and then include rapidcheck header which makes sense to not require gtest. Different for catch. Will check it out.

Names: I think the official targets are not namespaced correct? This is why the aliases with the correct names are added without a namespace.

Core is just the name of the component, it cannot be the same name as the package I think. The resulting target is still named rapidcheck::rapidcheck (and aliased to rapidcheck)

@conan-center-bot

This comment has been minimized.

@mjvankampen
Copy link
Contributor Author

Please have a look. I left out boost for now. I think it is not very nice to link gtest/catch to the core lib, but I would like to keep the gtest and such integration libs INTERFACE libraries to not change the code around too much.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

SSE4
SSE4 previously approved these changes Sep 25, 2021
@prince-chrismc
Copy link
Contributor

I think it is not very nice to link gtest/catch to the core lib, but I would like to keep the gtest and such integration libs INTERFACE libraries to not change the code around too much.

I absolutely agree, its completely customer facing.
You should have added it to the framework components not to "core"

I am not sure why you added patches since that was not required in the first place 🤔

The CMake aliases are perfect, my previous note was about the Conan ones... when you go to use them in a downstream conanfile the sames are not very distinctive (they are the names of other projects) without the prefix that upstream uses

@mjvankampen
Copy link
Contributor Author

Please have a look if this addresses your concerns:

  1. I added the external libs to core as they are introduced to into the upstream project as cmake subdirectory. I don't think we can do anything similar on conan so I thought this was closest. I agree it is not.
  2. A patch is required to disable the git submodules. Agreed that the rest of the patch is not required.
  3. Clear updated

@mjvankampen mjvankampen changed the title Adds rapidcheck boost/gtest/catch integrations Adds rapidcheck gtest/catch integrations Sep 27, 2021
@conan-center-bot

This comment has been minimized.

prince-chrismc
prince-chrismc previously approved these changes Sep 27, 2021
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Amazing work!

@uilianries
Copy link
Member

@prince-chrismc Good catch! thanks for reviewing!

Co-authored-by: Uilian Ries <uilianries@gmail.com>
@conan-center-bot
Copy link
Collaborator

All green in build 13 (c55c7509e20d2dbb845ab940430e86c12215d751):

  • rapidcheck/cci.20200131@:
    All packages built successfully! (All logs)

  • rapidcheck/cci.20210702@:
    All packages built successfully! (All logs)

  • rapidcheck/cci.20210107@:
    All packages built successfully! (All logs)

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 87884e7 into conan-io:master Sep 28, 2021
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