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

[gtk] Add a dependency on gettext[tools] to build translation files #24596

Merged
merged 1 commit into from
Jun 17, 2022
Merged

[gtk] Add a dependency on gettext[tools] to build translation files #24596

merged 1 commit into from
Jun 17, 2022

Conversation

mkhon
Copy link
Contributor

@mkhon mkhon commented May 7, 2022

GTK translation files are built only when gettext-tools is present

  • What does your PR fix?

N/A

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

all

Yes

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

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@Neumann-A
Copy link
Contributor

No portfile changes for explicit control?

@FrankXie05 FrankXie05 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:author-response labels May 7, 2022
ports/gtk/vcpkg.json Show resolved Hide resolved
@FrankXie05
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FrankXie05
Copy link
Contributor

cc @Neumann-A

@mkhon
Copy link
Contributor Author

mkhon commented Jun 2, 2022

/azp run

@azure-pipelines
Copy link

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

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Jun 6, 2022
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@@ -17,6 +17,7 @@ vcpkg_find_acquire_program(PKGCONFIG)
get_filename_component(PKGCONFIG_DIR "${PKGCONFIG}" DIRECTORY )
vcpkg_add_to_path("${PKGCONFIG_DIR}") # Post install script runs pkg-config so it needs to be on PATH
vcpkg_add_to_path("${CURRENT_HOST_INSTALLED_DIR}/tools/glib/")
vcpkg_add_to_path("${CURRENT_HOST_INSTALLED_DIR}/tools/gettext/bin")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
vcpkg_add_to_path("${CURRENT_HOST_INSTALLED_DIR}/tools/gettext/bin")

We should instead add msgfmt to ADDITIONAL_NATIVE_BINARIES in vcpkg_configure_meson() below to ensure it's using our copy of gettext:

msgfmt='${CURRENT_HOST_INSTALLED_DIR}/tools/gettext/bin/msgfmt${VCPKG_HOST_EXECUTABLE_SUFFIX}'

Copy link
Contributor

Choose a reason for hiding this comment

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

ADDITIONAL_NATIVE_BINARIES in vcpkg_configure_meson()

... undocumented ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually PATH is altered by ports/gettext/vcpkg-port-config.cmake so I removed this patch hunk.

Additionally, when checking for gettext availability meson checks for xgettext binary, not msgfmt, so your suggestion does not seem to be valid.

@FrankXie05 FrankXie05 removed the info:reviewed Pull Request changes follow basic guidelines label Jun 7, 2022
@FrankXie05 FrankXie05 requested a review from ras0219-msft June 13, 2022 02:52
@FrankXie05 FrankXie05 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 17, 2022
@dan-shaw dan-shaw merged commit 895a82a into microsoft:master Jun 17, 2022
@mkhon mkhon deleted the fix/gtk-translations branch June 17, 2022 22:02
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.

6 participants