-
Notifications
You must be signed in to change notification settings - Fork 21
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
WIP: buildenv/1.3.x/win32-static: update dependencies #80
base: master
Are you sure you want to change the base?
WIP: buildenv/1.3.x/win32-static: update dependencies #80
Conversation
a2d0413
to
1cf4f00
Compare
Quick drive-by comment: BTW, the reason some URLs don't use https:// is that on some platforms, i.e. CentOS 5.9, the system OpenSSL only supports TLS 1.0, which fails to negotiate on the web nowadays :) We have the correct hashes (sha1, sha256 and blake2) so it's not a big deal to get the files via http. |
elif [ "${MUMBLE_BUILD_CONFIGURATION}" == "Debug" ]; then | ||
BUILD_TYPE="Debug" | ||
cmd /c $(cygpath -w ${MUMBLE_PREFIX}/cmake/bin/cmake.exe) -G "NMake Makefiles" -DPCRE2_BUILD_PCRE2_8=OFF -DPCRE2_BUILD_PCRE2_16=ON -DPCRE2_BUILD_PCRE2_32=OFF -DCMAKE_BUILD_TYPE=${BUILD_TYPE} -DINSTALL_MSVC_PDB=ON -DCMAKE_INSTALL_PREFIX=$(cygpath -w ${MUMBLE_PREFIX}/pcre) |
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.
???
Why split them in two? And what's this MSVC PDB option?
We're building PCRE statically, so we get object PDBs.
In any case, we'd want PDBs also for release builds. How else are we going to debug?
Please explain.
It seems to me the original way I did it just OK?
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.
You're right, there's no need for -DINSTALL_MSVC_PDB=ON
in our case.
I will move the first line after the if
block and delete the second one.
} | ||
|
||
function prepare { | ||
cp ${MUMBLE_BUILDENV_ROOT}/cmake/harfbuzz-ng-CMakeLists.txt ./CMakeLists.txt |
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.
Does harfbuzz's own CMakeListsts.txt really do a static build? And against a shared CRT? :)
} | ||
|
||
function prepare { | ||
patch -p1 < ${MUMBLE_BUILDENV_ROOT}/patches/libogg-static-vs2010-Zi.patch |
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.
Is it safe to drop this without losing debug symbols?
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.
I don't think so, I have to update the patch.
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-macextras-disable-qmacpasteboardmime.patch | ||
|
||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-force-qtimageformats-jasper-no-libjpeg.patch | ||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-qtimageformats-jasper-4-color-rct-debian.patch |
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.
I'm guessing you've just left out the three patches above because they aren't used on Windows... Or?...
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.
The first patch is not required anymore: https://bugreports.qt.io/browse/QTBUG-35310
The second and third patches should be updated.
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.
Are you sure it's fixed in MSVC 2017?
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.
Ah, per Thiago: "The fix is coming in 15.8." (from the linked bug)
|
||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-configureapp-use-msvc2013-mkspec.patch | ||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-mariadb-support.patch | ||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-mkspecs-common-msvc-desktop-mumble-debug+opt.patch |
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.
I doubt it's correct to drop this outright. Elaborate?
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.
You're right, only the workaround for QTBUG-58318
should be removed.
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-avoid-j-underflow-in-harfbuzz.patch | ||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-we-want-a-buffer-for-named-pipes.patch | ||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-quick-and-dirty-hack-to-avoid-ssl-error-poisoning.patch | ||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt-5.6.1-work-around-client-verification-error.patch |
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.
Don't drop this.
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.
Is it still required?
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.
I don't know for sure. But I don't think just before a release is a time to drop something like this.
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt-5.6.2-qsystemtrayicon-win-nosound.patch | ||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-fix-no-sse2-win32-build.patch | ||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-fix-win32-no-sse2-build-for-angle.patch | ||
patch -p1 < ${MUMBLE_BUILDENV_COMMON}/qt5/patches/qt5-define-using-v110-sdk71-for-rc-exe-when-targetting-winxp.patch |
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.
Dropping this because this version of Qt doesn't support XP?
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.
Exactly.
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.
qt-5.6.2-qsystemtrayicon-win-nosound.patch isn't Windows XP specific though?
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.
It's contained in qt-5.9.6-qsystemtrayicon-win-realtime-nosound.patch
.
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.
Hold on, I'm confused...
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.
I had to use the same file to prevent a merge conflict.
@@ -47,10 +47,6 @@ function install { | |||
# Remove libmariadb .dll and .lib. We don't need/want them in our static build. | |||
rm -rf ${MUMBLE_PREFIX}/mariadbclient/lib/libmariadb.* | |||
|
|||
# Qt, our only user, expects to link against libmysqlclient. |
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.
Qt now links against mariadbclient? ...Or?
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.
I specified the libraries with MYSQL_LIBS="-lmariadbclient -lzlib -lws2_32 -ladvapi32 -lshlwapi"
.
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.
OK.
OK. Done with this round of review. Mostly concerned about potentially missing Qt patches :) |
1cf4f00
to
6b97ad8
Compare
Qt 5 patches: Removed:
Why did you drop this? I don't remember why the patch is there in the first place: Perhaps QMacPasteboardMime just didn't work? I'm worried that the macOS build will break without this, but I guess it just doesn't apply anymore?
This should be kept.
Don't drop.
Don't drop. It don't think it harms anything?
Dropped why?
Dropped why?
I guess this is now upstream?
This is fixed upstream now I guess?
This is fixed upstream now I guess?
This is fixed upstream now I guess? |
The issue is fixed upstream: https://bugreports.qt.io/browse/QTBUG-35310 mumble-releng/buildenv/1.3.x/common/qt5/qt5.build Lines 31 to 34 in 694e6d0
|
ff585d4
to
4b87926
Compare
No description provided.