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

[corrade] Add components + build_module + modernize #6290

Merged
merged 13 commits into from
Aug 25, 2021

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Jul 12, 2021

Specify library name and version: corrade

  • add components
  • modernize
  • add build_modules

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@jgsogo jgsogo marked this pull request as ready for review July 13, 2021 07:52
@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 requested a review from uilianries July 14, 2021 06:15
@conan-center-bot conan-center-bot requested a review from czoido July 15, 2021 11:57
@jgsogo
Copy link
Contributor Author

jgsogo commented Jul 17, 2021

Kindly requesting another review round. Thanks!

conan_basic_setup()
conan_basic_setup(TARGETS)

find_package(Corrade REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

This pr adds the Corrace::rc excutable target.
Do we want to test it?
Does it work with shared libs on Linux/Windows/Macos in cmake?

Copy link
Contributor

Choose a reason for hiding this comment

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

how do you usually test such kinds of targets? do you have a good example?
I mean, you don't usually do target_link_library(${PROJECT_NAME} <my_executable_target>

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'm not adding an rc component, amI? Conan is not creating that target

Copy link
Contributor

@madebr madebr Aug 17, 2021

Choose a reason for hiding this comment

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

You're creating the target in conan-corrade-vars.cmake.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://cmake.org/cmake/help/latest/command/execute_process.html

🤔

get_target_property(CORRADE_RC_PROGRAM Corrade::rc IMPORTED_LOCATION)
execute_process(${CORRADE_RC_PROGRAM} --version) # Idk the command line but just a suggestions

@conan-center-bot

This comment has been minimized.

@conan-center-bot conan-center-bot requested a review from SSE4 July 23, 2021 11:59
@jgsogo jgsogo dismissed stale reviews from uilianries and SSE4 via 98e2a35 August 17, 2021 15:59
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
@conan-center-bot

This comment has been minimized.

uilianries
uilianries previously approved these changes Aug 17, 2021
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@jgsogo
Copy link
Contributor Author

jgsogo commented Aug 18, 2021

If with_utility=False, the rest of the options should be False as well and the package contains basically nothing. Nevertheless, I've prepared the test_package to support this situation, it will just test that an include file is available (in version 2019.10 nothing from this file is actually usable).

@conan-center-bot
Copy link
Collaborator

All green in build 10 (d7e6c08077f182a6f033be71fdfa83cf594b7557):

  • corrade/2020.06@:
    All packages built successfully! (All logs)

  • corrade/2019.10@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit e0a2f08 into conan-io:master Aug 25, 2021
@jgsogo jgsogo deleted the corrade-fix-modernize branch August 26, 2021 14:06
@jgsogo jgsogo mentioned this pull request Aug 28, 2021
4 tasks
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.

7 participants