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.

Sign up for GitHub

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

[exiv2] Make add nls feature #18964

Merged
merged 3 commits into from
Jul 19, 2021
Merged

[exiv2] Make add nls feature #18964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e0441d
[exiv2] ios support
m-kuhn Jun 15, 2021
169f622
x-add-version
m-kuhn Jul 15, 2021
6b8048f
Restore previous commit
m-kuhn Jul 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions ports/exiv2/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
xmp EXIV2_ENABLE_XMP
video EXIV2_ENABLE_VIDEO
png EXIV2_ENABLE_PNG
nls EXIV2_ENABLE_NLS
)

string(COMPARE EQUAL "${VCPKG_CRT_LINKAGE}" "dynamic" EXIV2_CRT_DYNAMIC)
Expand All @@ -35,7 +36,6 @@ vcpkg_configure_cmake(
-DEXIV2_BUILD_SAMPLES=OFF
-DEXIV2_BUILD_DOC=OFF
-DEXIV2_ENABLE_EXTERNAL_XMP=OFF
-DEXIV2_ENABLE_NLS=OFF
-DEXIV2_ENABLE_PRINTUCS2=OFF
-DEXIV2_ENABLE_LENSDATA=ON
-DEXIV2_ENABLE_DYNAMIC_RUNTIME=${EXIV2_CRT_DYNAMIC}
Expand All @@ -53,6 +53,11 @@ vcpkg_install_cmake()
vcpkg_fixup_cmake_targets(CONFIG_PATH lib/cmake/exiv2)
vcpkg_fixup_pkgconfig()

if("nls" IN_LIST FEATURES)
set(EXIV2_ENABLE_NLS ON)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem connected to anything? I think you need to move it up and change

-DEXIV2_ENABLE_NLS=OFF
to
-DEXIV2_ENABLE_NLS=${EXIV2_ENABLE_NLS}

Copy link
Member

Choose a reason for hiding this comment

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

Actually I note that this if is already the behavior of the vcpkg_check_features above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it available for configure file:
https://github.com/microsoft/vcpkg/pull/18964/files#diff-1bdf222c873035319124c25b484ba3840ebc8adb43ea4731e43e4627dcf39ac3L56

Being used in here:
https://github.com/microsoft/vcpkg/pull/18964/files#diff-2f0b76215a2d1a149da5bfea386e4e22c09569045184d1b9fbd363fd4b784e5aR5

Is that not the correct approach?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. That makes sense, thanks! I also missed that there is a ${FEATURE_OPTIONS} for the other features so I was mistaken about that too -- thanks GitHub for hiding important context in your code review tool :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks GitHub for hiding important context in your code review tool :(

deciding what's relevant to get the best signal to noise ratio sounds like a non-trivial job ...

but while we are at it, I think this repository could benefit from some ci tooling to detect things like incorrect or missing usage of x-add-version and ideally even some integrated bot magic for fixing these issues...

Copy link
Member

Choose a reason for hiding this comment

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

deciding what's relevant to get the best signal to noise ratio sounds like a non-trivial job ...

The Azure DevOps code review tool just always shows you the whole file, it's never confusing. But there's no way to configure GitHub to do that :(

else()
set(EXIV2_ENABLE_NLS OFF)
endif()
configure_file(
${CMAKE_CURRENT_LIST_DIR}/vcpkg-cmake-wrapper.cmake
${CURRENT_PACKAGES_DIR}/share/${PORT}
Expand All @@ -70,4 +75,4 @@ if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
endif()

# Handle copyright
file(INSTALL ${SOURCE_PATH}/COPYING DESTINATION ${CURRENT_PACKAGES_DIR}/share/${PORT} RENAME copyright)
file(INSTALL ${SOURCE_PATH}/COPYING DESTINATION ${CURRENT_PACKAGES_DIR}/share/${PORT} RENAME copyright)
8 changes: 6 additions & 2 deletions ports/exiv2/vcpkg-cmake-wrapper.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@ _find_package(${ARGS})

if("@VCPKG_LIBRARY_LINKAGE@" STREQUAL "static")
find_package(Iconv REQUIRED)
find_package(Intl REQUIRED)
if(@EXIV2_ENABLE_NLS@)
find_package(Intl REQUIRED)
endif()
if(TARGET exiv2lib)
set_property(TARGET exiv2lib APPEND PROPERTY INTERFACE_LINK_LIBRARIES
Iconv::Iconv
)
target_link_libraries(exiv2lib INTERFACE ${Intl_LIBRARIES})
if(@EXIV2_ENABLE_NLS@)
target_link_libraries(exiv2lib INTERFACE ${Intl_LIBRARIES})
endif()
endif()
endif()
8 changes: 7 additions & 1 deletion ports/exiv2/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
{
"name": "exiv2",
"version": "0.27.4",
"port-version": 1,
"description": "Image metadata library and tools",
"homepage": "https://www.exiv2.org",
"supports": "!uwp",
"dependencies": [
"gettext",
"libiconv"
],
"features": {
"nls": {
"description": "Build native language support",
"dependencies": [
"gettext"
]
},
"png": {
"description": "Build with png support (requires libz)",
"dependencies": [
Expand Down
2 changes: 1 addition & 1 deletion versions/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1894,7 +1894,7 @@
},
"exiv2": {
"baseline": "0.27.4",
"port-version": 0
"port-version": 1
},
"expat": {
"baseline": "2.4.1",
Expand Down
5 changes: 5 additions & 0 deletions versions/e-/exiv2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "1e93f266421999dd82b931b2dbf672d255b14b43",
"version": "0.27.4",
"port-version": 1
},
{
"git-tree": "f716a33e59b24c79140216d836644fe0603e322b",
m-kuhn marked this conversation as resolved.
Show resolved Hide resolved
"version": "0.27.4",
Expand Down