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.

Sign up for GitHub

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

Jump to bottom

[WinCairo] Build error with CMAKE_DISABLE_PRECOMPILE_HEADERS ON after 257057@main #7185

Closed
wants to merge 1 commit into from
Closed

[WinCairo] Build error with CMAKE_DISABLE_PRECOMPILE_HEADERS ON after 257057@main #7185

wants to merge 1 commit into from

Conversation

dpino
Copy link
Contributor

@dpino dpino commented Dec 6, 2022 •
edited by webkit-early-warning-system

@dpino
[WinCairo] Build error with CMAKE_DISABLE_PRECOMPILE_HEADERS ON after…
d93078b 
… 257057@main

https://bugs.webkit.org/show_bug.cgi?id=248803

Reviewed by NOBODY (OOPS!).

* Source/WebCore/platform/LocalizedStrings.h: Add missing header
  'wtf/text/cf/StringConcatenateCF.h'.
@dpino dpino self-assigned this Dec 6, 2022
@dpino dpino marked this pull request as draft December 6, 2022 06:46
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash d93078b)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

fujii
fujii reviewed Dec 6, 2022
View reviewed changes
Source/WebCore/platform/LocalizedStrings.h
@@ -37,6 +37,10 @@
#include <glib/gi18n-lib.h>
#endif

#if USE(CF) && PLATFORM(WIN)
#include <wtf/text/cf/StringConcatenateCF.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should #include <CoreFoundation/CFString.h>.
Why StringConcatenateCF.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, I will try to build using that header.

I used wtf/text/cf/StringConcatenateCF.h because by grepping source code I saw the class was defined there (although actually is not). Somehow it worked though.

There are still other build errors that's why the PR is still a draft.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 6, 2022
@dpino dpino closed this Dec 29, 2022
@dpino dpino deleted the fix/windows-build-after-257057@main branch December 29, 2022 07:03
@dpino dpino restored the fix/windows-build-after-257057@main branch January 13, 2023 10:22
@dpino
Copy link
Contributor Author

dpino commented Jan 13, 2023

I closed this PR by mistake. Reopening it.

@dpino dpino reopened this Jan 13, 2023
@dpino
Copy link
Contributor Author

dpino commented Apr 13, 2023

Test compilation CMAKE_DISABLE_PRECOMPILE_HEADERS ON on top of main. WinCairo build works now fine. So nothing to do here. Closing PR.

@dpino dpino closed this Apr 13, 2023
@dpino dpino deleted the fix/windows-build-after-257057@main branch May 7, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujii fujii fujii left review comments

Assignees

@dpino dpino

Labels
merging-blocked Applied to prevent a change from being merged
Projects
None yet
Milestone
No milestone
4 participants
@dpino @webkit-early-warning-system @fujii @webkit-ews-buildbot