[gettext] Add gettext cmake wrapper #18159

dg0yt · 2021-05-27T20:11:31Z

What does your PR fix?

This PR adds a cmake config wrapper which covers find_package(Gettext).
Task: [gettext] msgfmt (and other gettext bin tools) missing on macOS & Windows #13518.
Which triplets are supported/not supported? Have you updated the CI baseline?

all, no
Does your PR follow the maintainer guide?

yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

yes

Neumann-A · 2021-05-27T20:21:22Z

I think that just needs a change at

Lines 548 to 554 in 9d7244f

    
           list(APPEND CMAKE_PROGRAM_PATH "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/tools") 
        
           file(GLOB Z_VCPKG_TOOLS_DIRS "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/tools/*") 
        
           foreach(Z_VCPKG_TOOLS_DIR IN LISTS Z_VCPKG_TOOLS_DIRS) 
        
               if(IS_DIRECTORY "${Z_VCPKG_TOOLS_DIR}") 
        
                   list(APPEND CMAKE_PROGRAM_PATH "${Z_VCPKG_TOOLS_DIR}") 
        
               endif() 
        
           endforeach()

dg0yt · 2021-05-27T20:37:41Z

I think that just needs a change at ... (vcpkg.cmake)

I think the same, however minus the "just". It is the widest possible change, it will rebuild the world and affect virtually all ports if done naively, leading to all types of non-obvious side effects.
(My proposal is to extent the path variables only with regard to the declared dependencies. But I don't want to go ahead before someone would at least attempt to answer questions about filesystem layout.)
In the meantime, I offer this PR as a more explicit but narrow change.

dg0yt · 2021-05-28T05:23:06Z

kf5i18n:x64-windows-static:

-- Found Gettext: D:/downloads/tools/msys2/958982de8242b3f9/mingw32/bin/msgmerge.exe (found version "0.19.8.1")

i.e. successful, but not from vcpkg. (tools/gettext/bin/msgmerge.exe exists.)

This seems to cause the build failure:

Installing in D:/packages/kf5i18n_x64-windows-static. Run D:/buildtrees/kf5i18n/x64-windows-static-rel/prefix.sh to set the environment for KI18n.
-- Looking for __GLIBC__
-- Looking for __GLIBC__ - not found
CMake Error at D:/installed/x64-windows-static/share/ECM/kde-modules/KDEClangFormat.cmake:67 (configure_file):
  No error
Call Stack (most recent call first):
  D:/installed/x64-windows-static/share/ECM/kde-modules/KDEFrameworkCompilerSettings.cmake:79 (include)
  CMakeLists.txt:17 (include)

dg0yt · 2021-05-28T11:40:42Z

So, this works now. I will rearrange and re-run x-add-version later today.

However, I wonder if the additional msys2 packages are needed for kf5i18n. At least for mingw, they invite mixing different versions of compiler and libs when they should actually match closely.

ports/kf5i18n/vcpkg.json

wrobelda · 2021-05-31T22:18:09Z

I tried using this in #16562. However, the config log showed that vcpkg kept using the apt-provided gettext binaries, so I force-removed getext with sudo dpkg -r --force-depends "gettext". The build fails now with:

CMake Error at /home/cromo/vcpkg/downloads/tools/cmake-3.20.2-linux/cmake-3.20.2-linux-x86_64/share/cmake-3.20/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Gettext (missing: GETTEXT_MSGMERGE_EXECUTABLE
  GETTEXT_MSGFMT_EXECUTABLE)
Call Stack (most recent call first):
  /home/cromo/vcpkg/downloads/tools/cmake-3.20.2-linux/cmake-3.20.2-linux-x86_64/share/cmake-3.20/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
  /home/cromo/vcpkg/downloads/tools/cmake-3.20.2-linux/cmake-3.20.2-linux-x86_64/share/cmake-3.20/Modules/FindGettext.cmake:81 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)

I then tried doing the same with your kf5i18n, and am getting the same error. So it doesn't look like it works actually, at least not on Linux.

dg0yt · 2021-05-31T22:52:05Z

So it doesn't look like it works actually, at least not on Linux.

Or it works as expected: On Linux, gettext is an empty package for all features. This should be obvious from the head of the portfile, and this PR didn't promise to change that.

I do have a commit which actually builds tools also for Linux, but I'm not sure when to add it. (It wasn't working before #17970.)

wrobelda · 2021-05-31T23:02:21Z

Fair enough. I assumed that gettext tools were being built for Linux as well by now.

JackBoosY · 2021-06-01T02:12:29Z

@wrobelda Please ping me if this PR is ready for review.

wrobelda · 2021-06-01T04:43:27Z

@wrobelda Please ping me if this PR is ready for review.

@JackBoosY well, I am not the author, but it does work for the kf5i18n in this PR, and it does work in #16562, so I'd say it's ready for review.

dg0yt · 2021-06-01T05:38:48Z

kf5i18n is running GETTEXT_MSGFMT_EXECUTABLE on the host:
https://github.com/KDE/ki18n/blob/a07971a404f36da295d57660e7338323292b2645/cmake/KF5I18nMacros.cmake.in#L139-L145

dg0yt · 2021-06-01T05:41:05Z

@JackBoosY It is ready for review.

ports/gettext/gettext-cmake-wrapper.cmake

azure-pipelines · 2021-06-24T02:45:51Z

Azure Pipelines successfully started running 1 pipeline(s).

JackBoosY · 2021-07-19T09:49:25Z

Can you please resolve the file conflict?

…ext-wrapper

dg0yt · 2021-07-20T06:19:28Z

Can you please resolve the file conflict?

@JackBoosY Done.

vicroms

This LGTM as it is but I would like a more expert opinion.

ras0219-msft · 2021-07-23T23:08:19Z

I'm testing this locally with just a few minor changes -- if everything is good I'll push those and merge when CI turns green.

ras0219-msft

Final tweaks:

We should still perform the if (NOT EXISTS "/usr/include/libintl.h") when the tools feature is selected for consistency with when the feature is not selected.
Avoid copying the Makefile; since it can be anywhere, just reference it from the port directory
Consider backslashes in the path regexes
Do not deploy share/intl/vcpkg-cmake-wrapper.cmake on Linux -- this wrapper is only for if we're providing the runtime libraries.

LGTM, let's ship it!

ports/gettext/Makefile

ras0219-msft · 2021-07-24T20:11:11Z

Thanks again @dg0yt for the continued awesome work!

[gettext] Add gettext cmake wrapper (microsoft#18159)

JackBoosY · 2021-09-07T09:16:05Z

ports/gettext/portfile.cmake

+        OPTIONS
+            ${OPTIONS}
+    )
+    vcpkg_install_make(SUBPATH "/intl")


Why do you need to use this? Can we manually set the target intl instead of doing this?
If the intl's makefile is generated, then it should be related to the target in the makefile in the root directory.

Why do you need to use this? Can we manually set the target intl instead of doing this?

It was there before this PR.
Anyway, the gettext build system is complex and slow, and the gettext port has seen many attempts to shorten build times. The top-level Makefile (for ${SOURCE_PATH}/gettext-runtime!) will pull in additional build and install steps. If you don't like the SUBPATH approach, we might perhaps add entry points to an/the external Makefile provided by vcpkg - similar to what we already do for gettext-tools.

I wish to reduce the vcpkg_configure_cmake options.

Fine for me. Shall I try to prepare a PR for the gettext Makefile approach? (Assuming we are not in a hurry.)

@dg0yt Yes, also I will have a try.

wrobelda · 2021-09-19T14:51:13Z

@dg0yt this works for all the ports that do find_package(KF5I18n), yet somehow, when I try to build a project that does the same, it fails in VS 2019 with:

1> [CMake] CMake Error at C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
1> [CMake]   Could NOT find Gettext (missing: GETTEXT_MSGMERGE_EXECUTABLE
1> [CMake]   GETTEXT_MSGFMT_EXECUTABLE)
1> [CMake] Call Stack (most recent call first):
1> [CMake]   C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
1> [CMake]   C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/FindGettext.cmake:81 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
1> [CMake]   C:/vcpkg/scripts/buildsystems/vcpkg.cmake:786 (_find_package)
1> [CMake]   out/build/x64-Debug/vcpkg_installed/x64-windows/share/kf5i18n/KF5I18nMacros.cmake:5 (find_package)
1> [CMake]   out/build/x64-Debug/vcpkg_installed/x64-windows/share/kf5i18n/KF5I18nConfig.cmake:33 (include)

(where KF5I18nMacros.cmake:5 is find_package(Gettext REQUIRED))

, as if the wrapper was actually not being used at all in this case. This is very odd, me and @hellozee both having same issue on our ends. Any idea why this could be?

dg0yt · 2021-09-19T16:26:37Z

The gettext tools path is added by vcpkg-cmake-config.cmake, in response to the vcpkg tool telling ports.cmake that there is host dependency on gettext[tools].
When your project isn't a port, you lost. Even with the cmake toolchain. Same for all other host tools, such as pkgconfig (cf. #17487 (comment)).
At least for the cmake toolchain, we need a way to reliably get the host tools path (or at least the host triplet).

wrobelda · 2021-09-20T13:23:55Z

@dg0yt thanks for the explanation, it makes sense now. It's also a pretty bad situation – without explicit workarounds in my project's CMakeLists, I can't get it to work. And that kind of kills the whole joy of vcpkg being "transparent".

dg0yt force-pushed the gettext-wrapper branch from 5fee6f1 to 6088035 Compare May 27, 2021 20:40

JonLiu1993 assigned JackBoosY May 28, 2021

JonLiu1993 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 28, 2021

dg0yt force-pushed the gettext-wrapper branch from 6088035 to e9f186b Compare May 28, 2021 05:24

dg0yt added 4 commits May 28, 2021 18:38

Add gettext cmake wrapper

92149d4

x-add-version

df48398

Make kf5i18n use gettext[tools]:host

7bb50e0

x-add-version

8d50643

dg0yt force-pushed the gettext-wrapper branch from 5fc9d22 to 8d50643 Compare May 28, 2021 16:43

dg0yt marked this pull request as ready for review May 29, 2021 06:11

wrobelda reviewed May 29, 2021

View reviewed changes

ports/kf5i18n/vcpkg.json Show resolved Hide resolved

wrobelda mentioned this pull request May 29, 2021

[kf5configwidgets] new port #16562

Merged

wrobelda mentioned this pull request May 31, 2021

[kf5iconthemes] new port #16567

Merged

5 tasks

JackBoosY suggested changes Jun 1, 2021

View reviewed changes

ports/gettext/gettext-cmake-wrapper.cmake Outdated Show resolved Hide resolved

dg0yt added 2 commits June 1, 2021 20:38

Revise wrapper source names

895ef31

Update git-tree

55b90a2

dg0yt force-pushed the gettext-wrapper branch from 67250e9 to 55b90a2 Compare June 1, 2021 18:39

JackBoosY approved these changes Jun 2, 2021

View reviewed changes

JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jun 2, 2021

dg0yt marked this pull request as ready for review June 24, 2021 03:41

dg0yt added 3 commits July 19, 2021 15:35

Merge branch 'master' of https://github.com/microsoft/vcpkg into gett…

9fc5649

…ext-wrapper

Update port-version

95b64ac

x-add-version

4080bcd

dg0yt force-pushed the gettext-wrapper branch from 11578f7 to 4080bcd Compare July 19, 2021 13:38

JackBoosY approved these changes Jul 20, 2021

View reviewed changes

JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jul 20, 2021

vicroms approved these changes Jul 23, 2021

View reviewed changes

[gettext] Address PR comments

119461d

ras0219-msft approved these changes Jul 23, 2021

View reviewed changes

ports/gettext/Makefile Show resolved Hide resolved

ras0219-msft merged commit 7d2541c into microsoft:master Jul 24, 2021

sthagen added a commit to sthagen/microsoft-vcpkg that referenced this pull request Jul 25, 2021

Merge pull request #166 from microsoft/master

55905d9

[gettext] Add gettext cmake wrapper (microsoft#18159)

Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Jul 25, 2021

Merge pull request #685 from microsoft/master

52ee27f

[gettext] Add gettext cmake wrapper (microsoft#18159)

This was referenced Jul 25, 2021

[hunspell] build failure #17374

Closed

[gettext] msgfmt (and other gettext bin tools) missing on macOS & Windows #13518

Closed

dg0yt deleted the gettext-wrapper branch July 29, 2021 05:46

This was referenced Jul 30, 2021

[gettext] Add "autopoint" tool to gettext[core] #17317

Merged

[gettext] build failure #19337

Closed

JackBoosY reviewed Sep 7, 2021

View reviewed changes

ras0219-msft mentioned this pull request Oct 13, 2021

[gettext] find_package(Gettext) does not work due to missing host tools #20728

Open

akrieger mentioned this pull request Nov 27, 2022

Build Windows Release with CMake (part 2) CleverRaven/Cataclysm-DDA#61880

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gettext] Add gettext cmake wrapper #18159

[gettext] Add gettext cmake wrapper #18159

dg0yt commented May 27, 2021

Neumann-A commented May 27, 2021

dg0yt commented May 27, 2021 •

edited

Loading

dg0yt commented May 28, 2021

dg0yt commented May 28, 2021

wrobelda commented May 31, 2021 •

edited

Loading

dg0yt commented May 31, 2021

wrobelda commented May 31, 2021 •

edited

Loading

JackBoosY commented Jun 1, 2021

wrobelda commented Jun 1, 2021

dg0yt commented Jun 1, 2021

dg0yt commented Jun 1, 2021

azure-pipelines bot commented Jun 24, 2021

JackBoosY commented Jul 19, 2021

dg0yt commented Jul 20, 2021

vicroms left a comment

ras0219-msft commented Jul 23, 2021

ras0219-msft left a comment

ras0219-msft commented Jul 24, 2021

JackBoosY Sep 7, 2021

dg0yt Sep 7, 2021

JackBoosY Sep 8, 2021

dg0yt Sep 8, 2021

JackBoosY Sep 9, 2021

wrobelda commented Sep 19, 2021 •

edited

Loading

dg0yt commented Sep 19, 2021

wrobelda commented Sep 20, 2021 •

edited

Loading

[gettext] Add gettext cmake wrapper #18159

[gettext] Add gettext cmake wrapper #18159

Conversation

dg0yt commented May 27, 2021

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Neumann-A commented May 27, 2021

dg0yt commented May 27, 2021 • edited Loading

dg0yt commented May 28, 2021

dg0yt commented May 28, 2021

wrobelda commented May 31, 2021 • edited Loading

dg0yt commented May 31, 2021

wrobelda commented May 31, 2021 • edited Loading

JackBoosY commented Jun 1, 2021

wrobelda commented Jun 1, 2021

dg0yt commented Jun 1, 2021

dg0yt commented Jun 1, 2021

azure-pipelines bot commented Jun 24, 2021

JackBoosY commented Jul 19, 2021

dg0yt commented Jul 20, 2021

vicroms left a comment

Choose a reason for hiding this comment

ras0219-msft commented Jul 23, 2021

ras0219-msft left a comment

Choose a reason for hiding this comment

ras0219-msft commented Jul 24, 2021

JackBoosY Sep 7, 2021

Choose a reason for hiding this comment

dg0yt Sep 7, 2021

Choose a reason for hiding this comment

JackBoosY Sep 8, 2021

Choose a reason for hiding this comment

dg0yt Sep 8, 2021

Choose a reason for hiding this comment

JackBoosY Sep 9, 2021

Choose a reason for hiding this comment

wrobelda commented Sep 19, 2021 • edited Loading

dg0yt commented Sep 19, 2021

wrobelda commented Sep 20, 2021 • edited Loading

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

dg0yt commented May 27, 2021 •

edited

Loading

wrobelda commented May 31, 2021 •

edited

Loading

wrobelda commented May 31, 2021 •

edited

Loading

wrobelda commented Sep 19, 2021 •

edited

Loading

wrobelda commented Sep 20, 2021 •

edited

Loading