dannyedel / dspdfviewer

Dual-Screen PDF Viewer for latex-beamer
http://dspdfviewer.danny-edel.de
GNU General Public License v2.0
218 stars 27 forks source link

Add a windows build to the CI tests with AppVeyor #147

Closed dannyedel closed 8 years ago

dannyedel commented 8 years ago

This PR shall hold the required changes to make AppVeyor able to build dspdfviewer

Closes #142

dannyedel commented 8 years ago

@projekter: I've tried to build it, but it seems to try and link to the debug libraries since they're mentioned in the cmake file.

I simply used cmake --build . to call MSVC, should I use a different command to avoid that behaviour?

Even if that blows up the archive size a lot, I think it would be easier to just include them for now. That is, unless we want to blow up the cmake file with a lot of if windows/debug else statements or add some kind of autodetection instead of the hardcoded paths.

dannyedel commented 8 years ago

To better see the commands called, I've moved them from the appveyor webinterface to the .appveyor.yml file in the repository.

If you see the problem and know how to fix the build, feel free to simply branch off this and send a corrected PR with a changed .appveyor.yml or .cmake file. My idea with the static Qt switch didn't really work out : (

projekter commented 8 years ago

If cmake defaults to debug mode, try -DCMAKE_BUILD_TYPE=Release as a command line option. In the meantime, I will create a complete zip archive.

projekter commented 8 years ago

Now the archive includes everything. With rar, it's not as bad as with zip, only 20 MB.

dannyedel commented 8 years ago

Requesting a rebuild

dannyedel commented 8 years ago

Uh oh, now it spits out loads of linker errors. Please have a look I think it doesnt corrrectly set the "Static" mode. Maybe i was too fast with the "The system qt works" idea?

projekter commented 8 years ago

I made a diff with the output I get when running cmake --build .. Most of the output is identical, but the crucial differences are the cl and link calls. Let me give an example comparison for the CL call of libdspdfviewer:

Apart from those differences, the CL commands are identical. Now the link command:

projekter commented 8 years ago

The release is updated. As it now contains Qt as well, it has to be extracted directly on C:.

dannyedel commented 8 years ago

Wow, thanks for that detailed analysis!

What you wrote about the System Qt being dynamic sounds convincing -- maybe that forces the compiler into dynamic mode, overriding your /MT commands entirely.

And of course I forgot to set the Boost-Linking to static either - and I even made a cmake-time-switch for that...

I will try and patch the commands to make sure it uses the static libraries.

Thanks so much for refreshing the archive again! I will refresh the code. (Lets just hope extrating straight to C:\ doesn't clash with anything they have already preinstalled)

dannyedel commented 8 years ago

After setting the boost static parameters correctly, I got a lot further.

I also figured out why the /MD -> /MT replace didnt work: the variable list didnt contain the _CXX_ version of flags: de550ca

It spits out literally hundreds of warnings about "locally defined symbol imported" (basically it complains on every single function?) so I guess there's just one compile parameter missing to silence these :)

I also made sure in 55787f1 that cmake does not try to link the system qt libraries (it will only use the headers)

However, one problem remains: It seems to be missing a few STL-related QString functions - see line 2434 of the log.

I will try to patch the source so that it doesn't require the QT-STL glue, maybe then it works : )

dannyedel commented 8 years ago

Source patched to not use the STL-Compat-functions of Qt.

If I read the output correctly, it is now "only" missing the QTranslator and QCoreApplication::tr functions.

Any ideas? Maybe I did too much while trying to exclude the system Qt.

dannyedel commented 8 years ago

Final update from last night:

I have patched the source to not use QCoreApplication::tr() directly, but only through QCoreApplication::translate() (which is the proper way, anyway.)

But the remaning undefined references are

So I'm pretty sure that there is one translation unit missing now in the dependency package. Other than that, it seems to build. But it would be nice if we could get away the hundreds of linker warnings, I don't know how though.

projekter commented 8 years ago

So I have updated the dependencies once more, and they're growing... I have now also included the Qt5 include dir, which contains the QTranslator stuff as well. But the library that contains those is Qt5Core, which was already present before. Regarding the warnings, perhaps this inclusion also helps. The warnings are basically related to the fact that functions were marked as "import", but due to static linking, they need not be imported, but are linked directly to the application. This might have come from mixing the headers that were provided with my compiled libs. I have also included .pdb files (which I didn't use in my build); if they are found, this should also reduce the number of warnings. I just wonder if this will work, as the vc120.pdb appears lots of times at different locations - hopefully, the compiler can detect which one to use. If it cannot, I will remove all those files, as they only provide debug information, which are not required for compilation. I did not include the whole Boost headers, as I did not find any warning related to those. As Boost does not provide such a well-separated include directory, I had to include the whole framework (~ 100 MB).

dannyedel commented 8 years ago

Rebuild requested

projekter commented 8 years ago

I think the problem is that still the native headers are loaded:

C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\bin\CL.exe /c /I\Qt\5.5\msvc2013\include /I\Qt\5.5\msvc2013\include\QtCore /I"\Qt\5.5\msvc2013.\mkspecs\win32-msvc2013" /I\Qt\5.5\msvc2013\include\QtGui /I\Qt\5.5\msvc2013\include\QtWidgets /IC:\dspdf\poppler\poppler\include\poppler\qt5 /I\Libraries\boost /IC:\projects\dspdfviewer\build /Zi /W3 /WX- /Od /Ob0 /Oy- /D WIN32 /D _WINDOWS /D _DEBUG /D POPPLER_QT5 /D QT_NO_DEBUG_OUTPUT /D QT_NO_CAST_FROM_ASCII /D "DSPDFVIEWER_VERSION=\"v1.14-61-g5bec9f1\"" /D "CMAKE_INTDIR=\"Debug\"" /D _MBCS /Gm- /EHsc /RTC1 /MTd /GS /fp:precise /Zc:wchar_t /Zc:forScope /GR /Fo"dspdfviewer.dir\Debug\" /Fd"dspdfviewer.dir\Debug\vc120.pdb" /Gd /TP /analyze- /errorReport:queue C:\projects\dspdfviewer\main.cpp C:\projects\dspdfviewer\build\dspdfviewer.qrc.cxx C:\projects\dspdfviewer\build\dspdfviewer_automoc.cp

But all those includes are the wrong ones; there still seems to be some manual detection.

dannyedel commented 8 years ago

I had a lengthy and in-depth answer ready, but then my entire system crashed. Note to self: take the name "Debian unstable" more literally : ) Now posting from "Debian stable" laptop.

First of all, thanks again that you keep trying. My research showed that documentation for "Qt Static with MSVC" is pretty much nonexistent. I found a page about MinGW, but that just warned you that there "will be more work required" when you try this in MSVC. It seems a lot of people are using MinGW to compile Qt on Windows, probably because it's GPL and Qt is allowed to ship the whole compiler along with their library and call it "SDK". But seeing how painless the resulting MSVC-compiled executable works, I think you choose the "more-difficult-but-integrates-better-in-the-end" road by using MSVC instead of MinGW.

Regarding the build failure: It still falls back to the native Qt.

On the Linux builds, Qt (not cmake itself) ships the .cmake files that tell the compiler what includes etc. to use, and we point cmake towards them by using CMAKE_PREFIX_PATH. It seems your rar didn't include them, maybe that's the problem?

Just to try the Vorschlaghammermethode: How big is your c:\qt\static in total? They say "Storage is cheap", so if it's not multiple Gigabytes why not save some man-hours and upload the entire thing (Assuming that all the .cmake files will be inside). I have already seen in the docs that AppVeyor supports a cache mechanism, allowing to mirror large dependencies into the datacenter where the build runs from. (Also, for reference, the Travis MacOS build spends more time downloading and installing dependencies than it actually spends on dspdfviewer)


On the other hand, I don't think you need to include Boost at all. Their default installation ships both static and dynamic libraries, and allows the program to choose via the cmake Variables BOOST_USE_STATIC_LIBS (dll vs. lib I think) and BOOST_USE_STATIC_RUNTIME (MT vs MD if I understood the MSVC terms correctly), and we didn't get any link errors that have anything to do with boost, so far.


vc120.pdb files: The Internet™ suggests to place them in the same directory as the corresponding .lib (in this case popplerd.lib) - it should find them then. The directory it prints in the error message (somewhere within the dspdfviewer build dir) is just the fallback, but it seems confusing that it doesn't print the primary search directory first.

projekter commented 8 years ago

So my VM has finished counting ;): The Qt ordner is 8.82 GiB in total, containing 170'000 files. Including this file system structure, it's 9.07 GiB. If it's just one time copying the whole content, this should work; but if it needs to be extracted every time a rebuild is requested, I would tend not to recommend this approach. I think the files I included should be enough, if the compiler did just use them. Having the whole Qt folder will not make things better if CL still look at the default location.

dannyedel commented 8 years ago

it's 9.07 GiB

Allright. That counts as "too large" I guess... I would have never expected that, to be honest. But now I understand why people look at me strange when I say that I never maxed out my old laptop's 40 GB harddrive : )

In the meantime I RDP'd into the build machine (instructions here in case you want to try from one of your PRs later) and inspected the \qt\5.5\msvc2013\ folder that it keeps linking against.

Full contents uploaded to gist but the intresting part is I think the lib/cmake directory. Its only 394 KB on the build worker, but I suppose those are the files that cmake scans for when detecting a Qt installation, and if not it keeps falling back to \qt\<version>\<compilerID>

Does the qt-static you built come with cmake files in that folder?

projekter commented 8 years ago

Yes, I found those files and included them, together with another cmake folder that I found below qtbase. Now the archive is supposed to contain all cmakes.

dannyedel commented 8 years ago

While in the interactive RDP session, I figured out that the correct parameter to pass is CMAKE_PREFIX_PATH=\qt\static\qtbase. Now that it actually finds the \lib\cmake subdirectory, it starts to make more-or-less sensible error messages : )

CMake Error at C:/Qt/Static/qtbase/lib/cmake/Qt5Core/Qt5CoreConfig.cmake:15 (message):
  The imported target "Qt5::Core" references the file

     "C:/Qt/Static/qtbase/bin/qmake.exe"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "C:/Qt/Static/qtbase/lib/cmake/Qt5Core/Qt5CoreConfigExtras.cmake"

  but not all the files it references.
dannyedel commented 8 years ago

I pushed the commands into .appveyor.yml and restarted the build, to make sure I didn't jinx it by doing something interactively.

projekter commented 8 years ago

So this is a real dependency hell. I hope its covered by Qt's license, I also included the binaries. But I strongly suspect that at least these binaries should be the same as in the dynamically linked version. Anyways, now the bin/ folder is also included.

dannyedel commented 8 years ago

I hope its covered by Qt's license

I Am Not A Lawyer, but last time I checked the Open-Source part of Qt was licensed LGPL, which does allow distributing binaries compiled from the source code. I doubt .lib vs. .exe makes any difference here btw. The only part you have to watch for is that you have to distribute (or at least offer to do so) the source code that compiled to said binaries. Quoting http://www.qt.io/qt-licensing-terms/

Complete corresponding source code of the library used with the application or the device built using LGPL, including all modifications to the library, should be delivered with the application (or alternatively provide a written offer with instructions on how to get the source code). It should be noted that the complete corresponding source code has to be delivered even if the library has not been modified at all.

Things are probably different for the closed-source additions but I never used them so I don't think we are talking about this.

So this is a real dependency hell.

Oh yes. And it all seems to be because Qt decides that you pick static vs. dynamic linking at library compile time (vs. at application compile time). Compare boost, who just compile and ship each library both as dll and as static lib, allowing me to choose which to link at, via two cmake variables...

Bonus points for only offering dynamically pre-compiled for download, while in the deployment document it seems that "Just compile and link statically" makes life so much easier (2 Paragraphs vs. 10 Pages). In Qt 4.8 the exact words were "If you want to keep things simple"


Its not over...

CMake Error at /Qt/Static/qtbase/lib/cmake/Qt5Gui/Qt5GuiConfig.cmake:15 (message):
  The imported target "Qt5::Gui" references the file

     "/Qt/Static/qtbase/plugins/imageformats/qico.lib"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "/Qt/Static/qtbase/lib/cmake/Qt5Gui/Qt5Gui_QICOPlugin.cmake"

  but not all the files it references.

How do other projects handle this? I can't imagine dspdfviewer is the only Qt application ever to be ported to windows and facing this madness.


On another note, many applications in the Linux environment support a make install DESTDIR=/some/temp/dir command, creating a complete filesystem hierarchy there as needed (for example, it would create /some/temp/dir/usr/bin/myprogram). Package managers call this, and then pack /some/temp/dir into their archive, and unpack it as / on the target machine.

Maybe Qt offers this too from within /Qt/Static/qtbase/?

projekter commented 8 years ago

I included the whole plugins folder in the dependencies archive. I also tried to make install and yes, there is a make file, but it fails very soon. And this post doesn't make me think there is a nice way to provide all those files at once (besides this, make clean also fails).

dannyedel commented 8 years ago

Rebuild requested

dannyedel commented 8 years ago

Qt, are you kidding me. (I am seriously starting to think I chose the wrong gui library...)

Lots of errors like this:

  c:\qt\static\qtbase\include\qtcore\qobject.h(1): fatal error C1083: Cannot open include file: '../../src/corelib/kernel/qobject.h': No such file or directory [C:\projects\dspdfviewer\build\libdspdfviewer.vcxproj]

From the stackoverflow discussion you linked, it seems that Qt does not really want us to "install" on windows...


We already spent a lot more time than I expected trying to get this automatic build to run, and it seems the problem is "distribute static Qt" the entire time.

In my point of view, the main purpose of the AppVeyor integration is to see Fails-To-Build-From-Source and Testsuite-Fails errors that occur when building with MSVC, before even merging merging a pull request.

I fully agree with your method of compiling Qt statically for distributing the dspdfviewer.exe. It works like a charm and since you did that there was not a single problem like #48 since all the libraries are already inside. From what I understand, dynamic linking requires to collect and ship all the DLLs along in the application folder, which may become a pain. But on the system that compiled the program, those DLLs will naturally be there already.

So for me it seems that it might be easier if we let AppVeyor build and link dynamically, against Qt, boost and everything else. I don't know if MSVC is especially picky here, but in my experience with gcc I don't remember that a source code compiled in dynamic mode, but failed in static mode (provided the libraries were also available in a static version, of course).

Do you think that this could work? If so, do you think it would be possible to build the dependencies (without boost or qt) in default (MD/dll) mode so it could be mixed with the system Qt? Or would that be even more time-consuming than "inventing" a make install step for Qt/MT/static?

projekter commented 8 years ago

If now the whole src/ folder is required, this would really lead to uploading Qt. Or at least qtbase, which is 1.6 GiB. A dynamic link will surely work, I "just" have to recompile all dependencies in dynamic mode. This will take some time, but as you I think it will be more promising than trying to get Qt working.

dannyedel commented 8 years ago

To not throw away the massive amount of work already done on the "static build" front, I think it would be good to:

projekter commented 8 years ago

So compiling all dependencies worked, including poppler. I have added a second archive to the release. dspdfviewer also compiles (see dynamicLinking branch) its test files, but the application itself fails at the moment, as it cannot resolve QStaticPlugin. This is obvious, as the necessary libraries (qwindows.lib) do not exist in the precompiled version of Qt, so I think those are only necessary for static builds. And the class name already suggests dspdfviewer should really not make use of those files. But I have not found the reason why they are used and for today, that's enough.

dannyedel commented 8 years ago

Thanks for all that work!

Money Quote:

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:54.51

I found the reason for Qt trying to load the QStaticPlugin, it was in poppler-qt.h and I boxed it into conditionals in 153a867.

Once the last kinks are sorted out and appveyor builds, please try if you can still build this PR on your static build, starting from a fresh git clone, with commands like these:

mkdir build
cd build
cmake .. -DWindowsStaticLink=ON -DBoostStaticLink=ON -DUseQtFive=ON -DDownloadTestPDF=ON
cmake --build .

I added instructions that make WindowsStaticLink search for \qt\static\qtbase in 492f0df but I can't test it. Basically this should remove the need to specify all the Qt Libraries with full path, since their cmake's should™ take care of it.

Right now, it only seems to hang at trying to run the test. I'll have to RDP in and investigate.

dannyedel commented 8 years ago

Ha, ha, ha.

I have to admit, I did NOT expect this.

testrunner2

The program doesn't even know where to find the DLL on the machine that compiled it.

I will leave it at this stage for today.

Before I spend hours on the DLL Hell (I at least hope that this rabbithole doesn't go so deep as the Qt Static Hell)... Do you know any tool that would scan the \qt\5.5 and the \dspdf\poppler folders, collect the linked DLLs and copy them to the build/test folder?

projekter commented 8 years ago

This tool is me... That's what I did when I started before using the static version. But there's Dependency Walker. I have not tried it yet, but this tool should list all loaded DLLs, so you don't have to start the application, check the error message, copy the DLL and restart for the next DLL. Combined with Everything, collecting all required DLLs should be done quickly. Btw., that's the purpose of Path variable: The Qt DLLs are not in any "shared" folder, i.e. a default folder in which Windows will look for those. Simply adding them to Path will work (on command line: set path=%path%;C:/Qt/....; this will be a temporary change that is valid for all aplications that are started by this command line instance; permanently: Control Panel, System, Advanced Settings, Environment Variables, Path variable, which will only affect applications that were started after the change by explorer).

projekter commented 8 years ago

Checking out the branch and including the changes from #150, the following commands lead to a successful compilation:

mkdir build
cd build
cmake .. -DWindowsStaticLink=ON -DBoostStaticLink=ON -DUseQtFive=ON -DDownloadTestPDF=ON -DBOOST_ROOT=C:/dspdf/boost_1_59_0
cmake --build .

(so the only difference is the inclusion of boost root directory, which does not come very unexpectedly)

dannyedel commented 8 years ago

Simply adding them to Path will work (on command line: set path=%path%;C:/Qt/....; this will be a temporary change that is valid for all aplications that are started by this command line instance

Thanks, I did exactly that (in the .appveyor.yml) - now I can start the test program.

Bad news, it crashes with stack overflow (uh-oh....) but I believe I can work with that - at least a lot better than with the DLL Hell : )

I have already tried RDPing into the appveyor machine and fiddling with devenv /debugexe a bit, and as soon as I have long-ish access to a Win VM or real machine, I'm confident I can figure out what's wrong. Somehow it fails as soon as it tries to destroy a pdf page reference.

dannyedel commented 8 years ago

This is interesting:

I was able to reproduce the testsuite crash in a VM - it occurs in the destructor of the QImage I used to store the render result.

From what I gather, the error message _BLOCK_TYPE_IS_VALID(pHead->nBlockUse) generally means a pointer was deleted twice.

I will have to investigate what exactly is going on here, since I doubt Qt's behaviour is (supposed to be) so different on Windows than on Linux.

To be precise, the problem occurs when it tries to decontruct the both variable of type QImage, in testrenderonepage.cc.

dannyedel commented 8 years ago

Update: I traced down the issue.

The problem is that there was a mixup between Debug and Release libraries, that triggered false positives from the MSVC double-free checker:

  1. External library in release mode calls release-style new (Meaning, that this block of memory does not get registered)
  2. We call delete from the debug library
    1. delete (debug) checks if the pointer was alloc'd using debug new
    2. Its not in the list
    3. Program crashes

This happened on nearly every Qt or Poppler variable.

Solution: Make appveyor build in release mode. I'll rebase this onto latest master, and merge. From there on out, PRs will get checked against MSVC too. Happy happy : )

projekter commented 8 years ago

Could you figure out which library was the one called as a Release version? I thought I included both versions of all dependencies.

dannyedel commented 8 years ago

I don't know which it is yet, but I'll reset the VM state tomorrow to "MSVC, boost, cmake and qt installed", and create a .bat script that reproduces the error. I've fiddled around with it too much in the meantime.

But now that I know what I'm looking for, I think I can figure out the exact lib.

For tonight, putting the AppVeyor/MSVC/Release build is achievement enough, and adding AppVeyor/MSVC/Debug can be done in its own Pullrequest. I'll open a ticket so it doesn't get forgotten.