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

Allow build with 64bit MinGW (from Qt 5.12) #3961

Merged
merged 2 commits into from
Nov 1, 2018

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Sep 14, 2018

I believe Qt 5.12 will be the next LTS releases and it might come out in time for 3.0. Currently only a beta exists, but it looks promising.

Apparently its 64bit MinGW still does not include support for WebEngine, which I believe was missing from Qt 5.9's 32bit MinGW too and the main reason to switch to MSVC.
Let's hope this gets fixed in the Qt 5.12 final to come... it seems unlikely though, as since Qt 5.11 it is documented that for Windows this component is only available for MSVC 2017 64bit.

Some (more) reasons to support this:
With MSVC I still haven't found a good way to edit *.ui files, with Qt Creator this works out of the box, but there I haven't yet found a way to build MuseScore using the MSVC compiler. Here building with MinGW comes pretty handy. Similar for debugging, being more familiar with the Qt Creator debugger than the MSVC one. And also a MinGW DEBUG build runs much faster than an MSVC DEBUG build.

@@ -52,7 +52,7 @@ EditStringData::EditStringData(QWidget *parent, QList<instrString> * strings, in
strg = (*_strings)[numOfStrings - i - 1];
_stringsLoc.append(strg);
QTableWidgetItem *newCheck = new QTableWidgetItem();
newCheck->setFlags(Qt::ItemFlag(Qt::ItemIsUserCheckable | Qt::ItemIsEnabled));
newCheck->setFlags(Qt::ItemFlags(Qt::ItemIsUserCheckable | Qt::ItemIsEnabled));
Copy link
Contributor Author

@Jojo-Schmitz Jojo-Schmitz Sep 15, 2018

Choose a reason for hiding this comment

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

I don't quite understand how this could have ever worked? Anyway, now it does with Qt 5.12 and 5.9
This part of the PR will definitely be needed for MSVC and Qt 5.12, so is not related to MinGW, but to Qt 5.12 only, see also #3971, which got merged meanwhile

@@ -137,7 +137,11 @@ if (APPLE)
set ( POPPLER_COMPILE_FLAGS "-O2 -Wno-unknown-warning-option -Wno-write-strings -ansi -Wnon-virtual-dtor -Woverloaded-virtual -Wno-unused-private-field -Wno-return-stack-address -Wno-shift-negative-value -std=c++11")
else (APPLE)
if (MINGW)
set (POPPLER_COMPILE_FLAGS "-O2 -Wall -Wextra -Wno-write-strings -ansi -Wnon-virtual-dtor -Woverloaded-virtual -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-but-set-variable -Wno-format -std=c++11")
if (BUILD_64)
set (POPPLER_COMPILE_FLAGS "-O2 -Wall -Wextra -Wno-write-strings -ansi -Wnon-virtual-dtor -Woverloaded-virtual -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-but-set-variable -Wno-format -Wno-shift-negative-value -Wno-stringop-overflow -std=c++11")
Copy link
Contributor Author

@Jojo-Schmitz Jojo-Schmitz Sep 15, 2018

Choose a reason for hiding this comment

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

Not 100% sure whether this is due to 64bit or due to the newer compiler version, could be either or both, but no way to check.

@Jojo-Schmitz Jojo-Schmitz force-pushed the MinGW-64bit branch 2 times, most recently from 15def18 to 5d735af Compare September 15, 2018 20:05
@@ -168,7 +168,7 @@ void EditStringData::newStringClicked()
_stringsLoc.insert(i, strg);
stringList->insertRow(i);
QTableWidgetItem *newCheck = new QTableWidgetItem();
newCheck->setFlags(Qt::ItemFlag(Qt::ItemIsUserCheckable | Qt::ItemIsEnabled));
newCheck->setFlags(Qt::ItemFlags(Qt::ItemIsUserCheckable | Qt::ItemIsEnabled));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here, see above

if (BUILD_64)
install( FILES
${MINGW_ROOT}/bin/libgcc_s_seh-1.dll
${MINGW_ROOT}/lib/libportaudio-x86_64-w64-mingw32.static.dll
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe better separate install(FILES ... for these and a
${MINGW_ROOT}/lib/portaudio.dll RENAME libportaudio-x86_64-w64-mingw32.static.dll
for portaudio. Need to check and test...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, tested, it works, so got implemented

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Sep 18, 2018

Today Qt 5.12.0 Alpha got released. Qt WebEngine continues (for Windows) to be only available for MSVC 2017 64bit, and since Qt 5.11 this is even documented.

@Jojo-Schmitz Jojo-Schmitz force-pushed the MinGW-64bit branch 2 times, most recently from 864b49f to b13a29f Compare September 20, 2018 17:53
@Jojo-Schmitz Jojo-Schmitz changed the title Allow build with Qt 5.12 and its 64bit MinGW Allow build with 64bit MinGW (from Qt 5.12) Sep 21, 2018
@Jojo-Schmitz
Copy link
Contributor Author

@AntonioBL IIRC you've been building the MuseScore 2.x series with a 64bit MinGW, mind to review this PR?

@AntonioBL
Copy link
Contributor

Ok, but I was simply using MinGW 64bit and adjusting library linking and copying. I actually never went into detail with the warnings on third party code, so I don't know if they really highlight portions of code which could generate problems or not (and that's also one of the main reason for the "unsupported" label).

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Sep 21, 2018

I've disabled those warnings in thirdparty, as we're not going to do anything about them anyway.
My question mainly would rather be whether these warnings are down to 64bit (so whether you saw them in 2.x, with Qt 5.4 too) or down to the newer compiler, 7.3.0 (so new to Qt 5.12 and therefor may need a different approach to disable them).
Also I'd like to see what changes you needed for your 64bit builds.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Oct 10, 2018

That fix for the MinGW 64bit compiler warnings also got requested in #4014 and these warnings are apparently also shown on Linux.
That (now former) part of this PR was also in #3958, which just got merged

This PR is still valid with the Qt 5.12 beta1, released Oct 5th. The beta2 is expected next week (Oct 15th), just like Qt 5.9.7.

@Jojo-Schmitz
Copy link
Contributor Author

A question is whether to use the same dependencies folder (for lame, ogg, vorbis, zlib, sndfile and portaudio) here for MinGW that we already use for MSVC, rather than the older method of having those in the Qt tools directory.

@AntonioBL
Copy link
Contributor

In principle, the dependencies used for MSVC compilation should work also for MinGW, so we could probably use the same structure as the one for the MSVC compilation.

I still could not compile this branch yet, but it contains basically the same modifications I had added for the "unsupported 64bit Windows" build, so I don't see evident problems.
In my branch I had also added a -m64 flag, but maybe it is not needed; and I had added a definition -DWIN32 (in case of OS-dependent code relying on the definition of WIN32) which is maybe not needed as well.
I think the warnings in portaudio comes from the fact that it is a 64 bit compilation, while I also can see most of the other warnings also under Linux with a recent version of gcc.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Oct 13, 2018

@AntonioBL those portaudio warnings are also dealt with in #4025, for MSVC. We're not touching that thirdparty code, so better just disable the warnings (here for MinGW only, may be needed for Linux too), or (maybe better?) upgrade to a more recent and 64bit aware portmidi.
I didn't need any -m64 nor -DWIN32, it builds cleanly without and the resulting MuseSCotre.exe works fine without them too.
What do you mean by I still could not compile this branch yet, it didn't work or you didn't try?

@AntonioBL
Copy link
Contributor

I didn't try yet, but I don't think there will be problems :-)
I think it'd be better to update portmidi rather than simply disable the warnings; I see that there is also a pending PR #3999 dealing with this, but maybe simply taking upstream updated code could work.

@Jojo-Schmitz Jojo-Schmitz force-pushed the MinGW-64bit branch 5 times, most recently from 383a938 to 03bbb5e Compare October 14, 2018 21:14
@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Oct 15, 2018

I have some changes going to detect the includes from the MSCV dependencies folder and install the libraries from there, but so far failed for the linker to find them there too, any hints on that?

For updating portmidi see #4028, still warnings with that, disabled for MinGW here, for MSVC there

@Jojo-Schmitz
Copy link
Contributor Author

Still valid, with Qt 5.12 beta 2, released yesterday.

@Jojo-Schmitz Jojo-Schmitz force-pushed the MinGW-64bit branch 2 times, most recently from 21d5272 to 8415ff9 Compare October 25, 2018 13:27
@anatoly-os
Copy link
Contributor

Do we really want to support mingw 64-bit from the box? It still doesn't have QtWebEngine which means we won't ship the build as "supported" one.

@Jojo-Schmitz
Copy link
Contributor Author

It still has it merits when it comes to debugging and for working with the .ui files.
And for it's compiler warnings too ;-)

@Jojo-Schmitz Jojo-Schmitz force-pushed the MinGW-64bit branch 2 times, most recently from 779f77f to 7332911 Compare October 30, 2018 17:11
@anatoly-os anatoly-os merged commit 0128daf into musescore:master Nov 1, 2018
@@ -46,6 +46,7 @@

#include "click.h"

#define OV_EXCLUDE_STATIC_CALLBACKS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea (anymore) why I though this to be needed. Compiles without warning on MinGW 64bit and MSCV

@@ -15,6 +15,7 @@
#include "libmscore/audio.h"
#include "libmscore/score.h"

#define OV_EXCLUDE_STATIC_CALLBACKS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

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.

3 participants