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

[fmt,folly] backport '/utf-8 only if the compiler is MSVC at build time' #40944

Merged
merged 6 commits into from
Sep 24, 2024

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Sep 12, 2024

Backporting fmtlib/fmt#4159

Fixes #40912

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@miyanyan
Copy link
Contributor

miyanyan commented Sep 13, 2024

there's a folly patch when I update fmt to 11.0.2, https://github.com/microsoft/vcpkg/blob/master/ports/folly/fix-fmt11-cmake.patch

diff --git a/CMake/GenPkgConfig.cmake b/CMake/GenPkgConfig.cmake
index 0e93175..1637d9f 100644
--- a/CMake/GenPkgConfig.cmake
+++ b/CMake/GenPkgConfig.cmake
@@ -93,7 +93,7 @@ function(gen_pkgconfig_vars)
   # Set the output variables
   string(REPLACE ";" " " cflags "${cflags}")
   string(REPLACE ";" " " private_libs "${private_libs}")
-
+  string(REPLACE "<$<COMPILE_LANGUAGE:CXX>:/utf-8>" "/utf-8" cflags "${cflags}")
   # Since CMake 3.18 FindThreads may include a generator expression requiring
   # a target, which gets propagated to us through INTERFACE_COMPILE_OPTIONS.
   # Before CMake 3.19 there's no way to solve this in a general way, so we

I use string(REPLACE "<$<COMPILE_LANGUAGE:CXX>:/utf-8>" "/utf-8" cflags "${cflags}") to avoid cmake error, see facebook/folly#2250, so maybe this patch should be changed also

@WangWeiLin-MV WangWeiLin-MV added the category:port-bug The issue is with a library, which is something the port should already support label Sep 13, 2024
@WangWeiLin-MV
Copy link
Contributor

As @miyanyan said,

maybe this patch should be changed also

@aminya
Copy link
Contributor Author

aminya commented Sep 15, 2024

I removed the folly patch. That patch would cause the same fmt error for folly. The current fmt patch is robust under LLVM or MSVC. I only added it for old CMake versions that do not support generator expressions via file(GENERATE

@BillyONeal BillyONeal changed the title [fmt] pass /utf-8 only if the compiler is MSVC at build time [fmt] backport '/utf-8 only if the compiler is MSVC at build time' Sep 16, 2024
@BillyONeal BillyONeal changed the title [fmt] backport '/utf-8 only if the compiler is MSVC at build time' [fmt,folly] backport '/utf-8 only if the compiler is MSVC at build time' Sep 16, 2024
@BillyONeal
Copy link
Member

Has the folly change been submitted to folly?

@aminya
Copy link
Contributor Author

aminya commented Sep 16, 2024

@BillyONeal Created a PR:
facebook/folly#2293

@WangWeiLin-MV WangWeiLin-MV added the depends:upstream-changes Waiting on a change to the upstream project label Sep 18, 2024
@WangWeiLin-MV
Copy link
Contributor

Please resolve the git conflicts.

@Laro88
Copy link

Laro88 commented Sep 19, 2024

Comment, this would be nice to have resolved properly, fmt is currently breaking out builds.

@aminya
Copy link
Contributor Author

aminya commented Sep 20, 2024

Conflicts fixed

@ysyecust
Copy link

Looking forward to merging it soon. Due to this problem, clang-tidy cannot be used normally on Windows if the fmt library is included.

Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a comment

Choose a reason for hiding this comment

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

Just a suggestion

@aminya
Copy link
Contributor Author

aminya commented Sep 20, 2024

@WangWeiLin-MV I prefer to keep the setup simple. The next version of fmt will remove this patch anyway.

Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a comment

Choose a reason for hiding this comment

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

The port fmt and folly usage tests pass with the following triplets:

  • x64-linux
  • x64-windows

@WangWeiLin-MV WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label Sep 24, 2024
@WangWeiLin-MV
Copy link
Contributor

Tag reviewed for further review.

@JavierMatosD JavierMatosD merged commit d9af3f5 into microsoft:master Sep 24, 2024
16 checks passed
@WangWeiLin-MV WangWeiLin-MV removed the depends:upstream-changes Waiting on a change to the upstream project label Sep 25, 2024
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
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fmt] build failure: clang++: error: no such file or directory: '/utf-8'
7 participants