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

[qt5compat] add proper error message #30829

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

autoantwort
Copy link
Contributor

The iconv feature does not work when qtbase is build with icu. Add an explicit error message.

@@ -19,7 +19,12 @@ INVERTED_FEATURES

#For iconv feature to work the following must be true:
#CONDITION NOT FEATURE_icu AND FEATURE_textcodec AND NOT WIN32 AND NOT QNX AND NOT ANDROID AND NOT APPLE AND WrapIconv_FOUND
#TODO: check if qtbase was built with ICU and fail if iconv is given here.
if("iconv" IN_LIST FEATURES)
file(READ ${CURRENT_INSTALLED_DIR}/share/qtbase/vcpkg_abi_info.txt qtbase_abi_info)
Copy link
Member

Choose a reason for hiding this comment

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

I think we are opposed to digging around in vcpkg_abi_info.txt like this because it makes that contractual in a way it isn't currently.

Yes I know vtk does that, I'm not sure we should have done it there either...

Copy link
Contributor

Choose a reason for hiding this comment

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

we can install a vcpkg-port-config.cmake to make this stuff visible for downstream ports.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that these features act as alternatives is bad but given that this is the status quo not going to ask you to rip that out for now; but if you're wanting a dependent package to publish configuration information please make that explicit as @Neumann-A suggests rather than reverse engineering it based on an internal file format and the feature's name.

@@ -19,7 +19,12 @@ INVERTED_FEATURES

#For iconv feature to work the following must be true:
#CONDITION NOT FEATURE_icu AND FEATURE_textcodec AND NOT WIN32 AND NOT QNX AND NOT ANDROID AND NOT APPLE AND WrapIconv_FOUND
#TODO: check if qtbase was built with ICU and fail if iconv is given here.
if("iconv" IN_LIST FEATURES)
file(READ ${CURRENT_INSTALLED_DIR}/share/qtbase/vcpkg_abi_info.txt qtbase_abi_info)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that these features act as alternatives is bad but given that this is the status quo not going to ask you to rip that out for now; but if you're wanting a dependent package to publish configuration information please make that explicit as @Neumann-A suggests rather than reverse engineering it based on an internal file format and the feature's name.

@JonLiu1993 JonLiu1993 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:author-response labels Apr 14, 2023
@autoantwort autoantwort force-pushed the qt5compat-error branch 4 times, most recently from 1395ee3 to 35b232e Compare April 17, 2023 15:46
@BillyONeal BillyONeal merged commit c0173d3 into microsoft:master Apr 19, 2023
@BillyONeal
Copy link
Member

Thank you!

@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants