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

fix: pass /utf-8 only if the compiler is MSVC at build time #4159

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Sep 11, 2024

Fixes #4158

Fmt has started adding /utf-8 flags when installed through vcpkg on Windows even though it's not supported by Clang.
https://github.com/aminya/cpp_vcpkg_project/actions/runs/10810623863/job/29988266523?pr=55#step:6:213

Here's a full reproduction:
https://github.com/aminya/cpp_vcpkg_project/tree/a53c2ce04dd16a26a60a3c1d85d2df212977319a

Here are some generated code in build/vcpkg_installed/x64-windows/share/fmt/fmt-targets.cmake

# Create imported target fmt::fmt
add_library(fmt::fmt SHARED IMPORTED)

set_target_properties(fmt::fmt PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "FMT_SHARED"
  INTERFACE_COMPILE_FEATURES "cxx_std_11"
  INTERFACE_COMPILE_OPTIONS "\$<\$<COMPILE_LANGUAGE:CXX>:/utf-8>"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
)

# Create imported target fmt::fmt-header-only
add_library(fmt::fmt-header-only INTERFACE IMPORTED)

set_target_properties(fmt::fmt-header-only PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "FMT_HEADER_ONLY=1"
  INTERFACE_COMPILE_FEATURES "cxx_std_11"
  INTERFACE_COMPILE_OPTIONS "\$<\$<COMPILE_LANGUAGE:CXX>:/utf-8>"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
)

This should be conditional. Although vcpkg might use its own toolchain, it doesn't mean the cmake file will be used with MSVC for all the users of fmt. This includes clang-tidy usage (different at build time than configuration time).

@vitaut vitaut merged commit f924d16 into fmtlib:master Sep 11, 2024
44 checks passed
@vitaut
Copy link
Contributor

vitaut commented Sep 11, 2024

Thanks for the fix!

@aminya
Copy link
Contributor Author

aminya commented Sep 12, 2024

@vitaut you're welcome. Is there a way I can backport this to a previous version as a hotfix? It's blocking fmt's usage through vcpkg on Windows with LLVM.
Or when is the expected date for the next release?

@aminya
Copy link
Contributor Author

aminya commented Sep 12, 2024

For now, I am adding a patch to vcpkg
microsoft/vcpkg#40944

aminya added a commit to aminya/vcpkg that referenced this pull request Sep 12, 2024
@vitaut
Copy link
Contributor

vitaut commented Sep 14, 2024

We could make a new release but since you've already submitted a patch for vcpkg let's just go with it.

facebook-github-bot pushed a commit to facebook/folly that referenced this pull request Sep 30, 2024
Summary:
Workaround for #2250.

Ideally, the code should support the generator expression, but this workaround fixes the problem for fmt.

Also, see fmtlib/fmt#4159 and microsoft/vcpkg#40944

Pull Request resolved: #2293

Reviewed By: Gownta

Differential Revision: D62785183

Pulled By: Orvid

fbshipit-source-id: d45768f12d28f53122fdedfc396f1d27c7259d19
facebook-github-bot pushed a commit to facebook/hhvm that referenced this pull request Sep 30, 2024
Summary:
Workaround for facebook/folly#2250.

Ideally, the code should support the generator expression, but this workaround fixes the problem for fmt.

Also, see fmtlib/fmt#4159 and microsoft/vcpkg#40944

X-link: facebook/folly#2293

Reviewed By: Gownta

Differential Revision: D62785183

Pulled By: Orvid

fbshipit-source-id: d45768f12d28f53122fdedfc396f1d27c7259d19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot compile with LLVM/Ninja on Windows due to /utf-8
2 participants