-
Notifications
You must be signed in to change notification settings - Fork 10
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
libxml2: Migrate to CMake #48
Conversation
Co-authored-by: Frederik Seiffert <[email protected]>
Co-authored-by: Frederik Seiffert <[email protected]>
patches/libxml2-windows-icu.patch
Outdated
--- a/win32/Makefile.msvc | ||
+++ b/win32/Makefile.msvc | ||
@@ -64,7 +64,7 @@ LIBS = $(LIBS) iconv.lib | ||
!if "$(STATIC)" == "1" | ||
LIBS = $(LIBS) advapi32.lib sicuuc.lib sicuin.lib sicudt.lib | ||
!else | ||
-LIBS = $(LIBS) icuuc.lib icuin.lib icudt.lib | ||
+LIBS = $(LIBS) icu.lib | ||
!endif | ||
!endif | ||
!if "$(WITH_ZLIB)" == "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the old patch here
Thanks! I added a couple small comments. What was the reason for requiring Windows-provided ICU with this change? I think it’s fine to do this, but then we should also remove the ICU phase script, and probably mention this requirement further up top in the readme (not in the "building the toolchain" section as it’s both a build- and runtime requirement. |
Just out of convenience. I can change it back, but this would require testing correct detection of our ICU build. Should I look into that? |
Ok makes sense. I’ve pushed these changes removing ICU to make this more explicit. Can you please rebase onto the latest master? |
No description provided.