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

[azure-iot-sdk-c] Fixed the CMake config export. #11017

Merged
merged 6 commits into from
May 8, 2020
Merged

[azure-iot-sdk-c] Fixed the CMake config export. #11017

merged 6 commits into from
May 8, 2020

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented Apr 25, 2020

The azure-iot-sdk-c port seems not to be really usable by using CMake config files.

When one tries to consume the library by the following block:

find_package(azure_iot_sdks CONFIG REQUIRED)
# Note: 7 target(s) were omitted.
target_link_libraries(main PRIVATE serializer iothub_client prov_auth_client digitaltwin_client)

Soon one will see this error when running CMake:

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
IOTHUB_CLIENT_INCLUDES

From my end, the problem is that iothub_client target is not exported from its CMake configs, but only ${provisioning_libs} get exported.

This patch is attempting to correct this behavior.

@seanyen
Copy link
Contributor Author

seanyen commented Apr 25, 2020

cc @ewertons

Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

include(vcpkg_common_functions) is no longer needed.
Could you please remove it?

ports/azure-iot-sdk-c/CONTROL Outdated Show resolved Hide resolved
ports/azure-iot-sdk-c/CONTROL Outdated Show resolved Hide resolved
ports/azure-iot-sdk-c/portfile.cmake Outdated Show resolved Hide resolved
Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

Could you please update
configure_file(${SOURCE_PATH}/LICENSE ${CURRENT_PACKAGES_DIR}/share/azure-iot-sdk-c/copyright COPYONLY)
as
configure_file(${SOURCE_PATH}/LICENSE ${CURRENT_PACKAGES_DIR}/share/${PORT}/copyright COPYONLY)?

@seanyen
Copy link
Contributor Author

seanyen commented Apr 27, 2020

@NancyLi1013 Thanks for the feedback. I updated the recipe accordingly.

@ewertons Let me know if my patch makes sense or not. Hope the patch can be folded back to the upstream.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

include(vcpkg_common_functions) is no longer needed.
Could you please remove this line?
Sorry, I didn't notice this before.

@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@seanyen
Copy link
Contributor Author

seanyen commented May 5, 2020

include(vcpkg_common_functions) is no longer needed.
Could you please remove this line?
Sorry, I didn't notice this before.

@NancyLi1013 updated. Thanks for the feedback.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added requires:testing Needs tests added before merging and removed waiting for response labels May 8, 2020
@NancyLi1013
Copy link
Contributor

All features have passed on the following triplets:

  • x86-windows
  • x64-windows
  • x64-windows-static

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label May 8, 2020
@NancyLi1013 NancyLi1013 removed the requires:testing Needs tests added before merging label May 8, 2020
@strega-nil
Copy link
Contributor

Cool, thanks @seanyen :D

@strega-nil strega-nil merged commit ce65b33 into microsoft:master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants