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

[vcpkg_find_acquire_program] add gettext's msgmerge program #13532

Closed

Conversation

wrobelda
Copy link
Contributor

@wrobelda wrobelda commented Sep 15, 2020

Describe the pull request

Adds support for gettext's msgmerge program

Yes. Note that I intentionally did not replace the program name and version in the URL with variables because the upstream zip also includes the iconv binaries, so to not mislead a future updater that updating a version variable is enough.

@wrobelda wrobelda force-pushed the find_acquire_gettext_programs_2 branch from a581097 to cb9ac65 Compare September 15, 2020 01:46
@JackBoosY JackBoosY self-assigned this Sep 15, 2020
@Neumann-A
Copy link
Contributor

theoretically #11776 can built all of gettext but that is currently blocked by #12936

@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Sep 15, 2020
@wrobelda
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 13532 in repo microsoft/vcpkg

@wrobelda
Copy link
Contributor Author

@JackBoosY looks like osx build has stalled and was killed, hence the failure. Mind triggering the build again?

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Sep 16, 2020
@wrobelda
Copy link
Contributor Author

wrobelda commented Sep 17, 2020

On a second thought, while this allows ports to use the msmerge, it will also require a modification of the personal project's CMakeLists, should it also need msgmerge for its own purposes.

Moreover, brew install gettext or apt install gettext is necessary for macOS\Linux hosts, including your CI nodes, which can potentially cause conflicts with the gettext library installed using a portfile as a dependency.

I suppose you can close it for now and we should wait until #11776 gets merged.

@wrobelda
Copy link
Contributor Author

wrobelda commented Sep 17, 2020

theoretically #11776 can built all of gettext but that is currently blocked by #12936

@Neumann-A When you say "theoretically", does you mean that it does that already but e.g. it hasn't been tested, or does it require additional work?

@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Sep 17, 2020
@JackBoosY
Copy link
Contributor

Fine, please ping me if this issue needs to be considered.

@JackBoosY JackBoosY closed this Sep 17, 2020
@wrobelda
Copy link
Contributor Author

wrobelda commented Mar 6, 2021

@JackBoosY unfortunately, the vcpkg's native gettext implementation did not eventually arrive with the generator tools included. I can't find it now, but I saw @Neumann-A confirming it somewhere in #11776 or the related PRs.

From the conversation I had with @ras0219 on Slack, it seems that the current approach remains the same as before, i.e. to use the msys2 gettext package on Windows in case tools are needed:

I believe right now what generally happens is that on Linux/OSX we expect the generator tools from your system and on Windows we get the generator tools from msys2.

Therefore I believe that this PR adding msgmerge to vcpkg_find_acquire_program takes things a step forward. With that change in place, one can use the vcpkg gettext as a dependency and just add these:

vcpkg_find_acquire_program(GETTEXT_MSGMERGE)
get_filename_component(GETTEXT_MSGMERGE_EXE_PATH ${GETTEXT_MSGMERGE} DIRECTORY)
vcpkg_add_to_path(${GETTEXT_MSGMERGE_EXE_PATH})

One thing I also experimented with and tested was adding those to the msys2 default environment. Additionally, all generator tools should be discussed to be added, not just msgmerge – although this only addresses the latter.

I would appreciate your input and hopefully the correct approach to gettext usage can find its way into official Maintainer Guidelines.

@Neumann-A
Copy link
Contributor

cd0c444 adds feature tools to gettext. I don't know when I have time to move it out into a separate PR.
Building all the tools requires some time and also another patch to make autopoint und gettextsize relocatable. (Both are shellscripts hardcoding an absolute prefix.)

@JackBoosY
Copy link
Contributor

See #16612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gettext] msgfmt (and other gettext bin tools) missing on macOS & Windows
3 participants