CMake 3.11 (potential) issues

[adriaandegroot]

Details for the issue

CMake 3.11 produces a mocs_compilation.cpp file that has the includes in a slightly different order than CMake 3.10. This affects the build of the embedded QScintilla, which has a header file declaring enum { SCI_SELECTALL = 2013 } and a different header file doing #define SCI_SELECTEALL 2013 .

There doesn't seem to be a CMake policy to change to the old sorting; you could add a weird hack to the Scintilla headers to force the declaration to be included from the header doing the #defineing -- before the #defines -- to avoid this. I don't know what (if) QScintilla is doing about it.

In the FreeBSD packaging (which is where I noticed this), we have just switched to using the system QScintilla. (Which needed patching -- the FindQScintilla module is a bit wonky).

Useful extra information

Actrual compiler error:

In file included from libs/qscintilla/Qt4Qt5/qscintilla2_autogen/mocs_compilation.cpp:4:
In file included from libs/qscintilla/Qt4Qt5/qscintilla2_autogen/SARX73RSAP/moc_qsciabstractapis.cpp:9:
In file included from libs/qscintilla/Qt4Qt5/qscintilla2_autogen/SARX73RSAP/../../Qsci/qsciabstractapis.h:29:
In file included from libs/qscintilla/Qt4Qt5/Qsci/qsciscintilla.h:32:
In file included from libs/qscintilla/Qt4Qt5/Qsci/qscicommand.h:27:
libs/qscintilla/Qt4Qt5/Qsci/qsciscintillabase.h:121:9: error: expected identifier
        SCI_SELECTALL = 2013,
        ^
libs/qscintilla/Qt4Qt5/../include/Scintilla.h:65:23: note: expanded from macro 'SCI_SELECTALL'
#define SCI_SELECTALL 2013

I'm opening this issue because:

• DB4S is crashing
• DB4S has a bug
• DB4S needs a feature
• DB4S has another problem

I'm using DB4S on:

• Windows: ( version: ___ )
• Linux: ( distro: ___ )
• Mac OS: ( version: ___ )
• Other: FreeBSD

I'm using DB4S version:

• 3.10.1
• 3.10.0
• 3.9.1
• Other: ___

I have also:

• Tried out the latest nightly version: https://github.com/sqlitebrowser/sqlitebrowser#nightly-builds
• Searched for an existing similar issue: https://github.com/sqlitebrowser/sqlitebrowser/issues?utf8=%E2%9C%93&q=is%3Aissue%20

[justinclift]

Thanks @adriaandegroot, looks like an interesting problem. 😉

@lbartoletti Any ideas? Asking just due to the FreeBSD bit, but kind of hoping... 😄

[justinclift]

Heh Heh Heh: https://euroquis.nl/bobulate/?p=1816

sqlitebrowser has a really weird copy of QtScintilla which has both an enum and a list of #defines for the same tokens .. which breaks under some inclusion sequences. (not fixed)

I kind of wonder if we should just be using either the enum or the defines. Doubling up doesn't sound like a good approach. 😉

[lbartoletti]

@justinclift why using a copy of QtScintilla?
For FreeBSD, thanks to @adriaandegroot , we using "external" version (from pkg/ports) and it works.

[justinclift]

Good question. Trying to remember the answer to that... I'm not sure. My suspicion is that we bundle it so that it's easily available to us for compiling across all platforms. eg Windows in particular where 3rd party dependencies are often all over the place

[MKleusberg]

Hmm, not sure if we can do anything about that. You could try moving the mentioned file names up or down in the lists in libs/qscintilla/Qt4Qt5/CMakeLists.txt. Or maybe change line 167 (add_library(qscintilla2 ${QSCINTILLA_SRC} ${QSCINTILLA_HDR} ${QSCINTILLA_MOC_HDR} ${QSCINTILLA_MOC})) to add_library(qscintilla2 ${QSCINTILLA_MOC_HDR} ${QSCINTILLA_MOC} ${QSCINTILLA_SRC} ${QSCINTILLA_HDR}). But unfortunately that is just guessing. I can hardly find any cmake documentation on this. Also we can't open an issue upstream for this because the upstream QScintilla code doesn't include cmake support - it has been added by us.

[MKleusberg]

And Justin is right about the reasons for including a copy of QScintilla. It's all about letting people build DB4S as easily as possible (obviously not thinking about your case here...) without having to install any libraries or setting up any include paths despite Qt and SQLite.

[karim]

I have the same problem too on Win 7. Last known CMake version to build DB4S is 3.10.3. I tried Martin's suggestion and it didn't work.

[karim]

Just thinking out loud here.

This wasn't an issue with older version of CMake, up to 3.10.3. With the introduction of 3.11, DB4S fails to build. So I checked their release notes and it appears they changed something regarding AUTOMOC. The new version will start multiple parallel moc processes to reduce the build time.

Maybe that's the reason behind the change in the order of includes that Adriaan mentioned?

[MKleusberg]

Here's what I found out so far. We get the compile error because there is a #define and an enum constant with the same name. Included in the wrong order the define screws up the enum definition. The #define comes from the Scintilla code, the enum from the QScintilla wrapper. So it's not our fault because that's just how the code is. But it's not QScintilla's fault either because they don't support cmake - we added the cmake file.

Now cmake generates a file called mocs_compilation.cpp (or multiple such files but I'm talking about the one for QScintilla here which is in the build/libs/qscintilla/Qt4Qt5/qscintilla2_autogen/ directory). For me this file looks like this:

// This file is autogenerated. Changes will be overwritten.
#include "EWIEGA46WW/moc_SciClasses.cpp"
#include "EWIEGA46WW/moc_ScintillaQt.cpp"
#include "SARX73RSAP/moc_qsciabstractapis.cpp"
#include "SARX73RSAP/moc_qsciapis.cpp"
#include "SARX73RSAP/moc_qscilexer.cpp"
#include "SARX73RSAP/moc_qscilexercpp.cpp"
#include "SARX73RSAP/moc_qscilexercustom.cpp"
#include "SARX73RSAP/moc_qscilexerhtml.cpp"
#include "SARX73RSAP/moc_qscilexerjavascript.cpp"
#include "SARX73RSAP/moc_qscilexerjson.cpp"
#include "SARX73RSAP/moc_qscilexerpython.cpp"
#include "SARX73RSAP/moc_qscilexersql.cpp"
#include "SARX73RSAP/moc_qscilexerxml.cpp"
#include "SARX73RSAP/moc_qscimacro.cpp"
#include "SARX73RSAP/moc_qsciscintilla.cpp"
#include "SARX73RSAP/moc_qsciscintillabase.cpp"

All the files and directories included there are generated, too. Interestingly the first two lines are in a different directory than the other ones. If I move them to the bottom of the file, it compiles just fine. So the question seems to be: what can we do to let cmake generate the file in such a way that these two files are included last? I guess to answer this we would need to understand why there are even two different subdirectories. The documentation says:

The custom directories with checksum based names help to avoid name collisions for moc files with the same basename.

But I don't see why there should be a name collision here.

I'm going to stop investigating now. For me, what's said here about xorg.conf is equally true (or even truer (if that's a word)) for cmake files. But maybe somebody has an answer to my question above 😒

[justinclift]

Heh Heh Heh

That's good research. Very valid conclusion too.

We may need to ask on the CMake mailing list to get an answer though (unsure). 😉

[revolter]

@MKleusberg, Do you know what exactly autogenerates that file?

[justinclift]

So the question seems to be: what can we do to let cmake generate the file in such a way that these two files are included last?

Could we instead take the approach of changing the name of either the #define or enum? If that works, we could have a chat with the Q/Scintilla people to let them know of the problem and our potential fix.

Kind of thinking we might not be the only people that use it with CMake, so if we describe the problem to them they might be willing to work out some solution or use whatever we figure out. 😄

[MKleusberg]

@revolter The program that generates the moc_xxx.cpp files is called moc. I'm not sure though which program generates the file mocs_compilation.cpp file. It's probably generated through cmake though.

@justinclift I don't think it's that easy. The Scintilla library uses #define and they won't see any reason to change that because they aren't interested in QScintilla. QScintilla on the other hand, because they include Scintilla files, uses Scintilla's #defines as well. No way to get around that. Additionally however they have their own enums which they could get rid of. But they probably won't because 1) enum is better style than #define and gives better error messages etc. and 2) for them it's a way to not always include all the Scintilla code when all they need is a constant. So it improves their code quality in two ways. Also renaming their enum symbols doesn't make sense because being just a wrapper they should offer the same API in all places. Lastly we shouldn't change this ourselves in our copy of the library because it introduces too many differences between our copy and the upstream version which makes this really hard to update in the future and tricky to use in a way which doesn't depend on the changed symbol names.

So I think this is something we need to look into and fix and we need to do it by tweaking something with cmake, even though I don't like that conclusion...

[revolter]

But we already modified the local QScintilla version. For example, we manually removed support for other programming languages by removing lines from multiple files.

[MKleusberg]

You're right of course. But I think the scale would be different. Currently we have some very local changes (and some of them are already applied upstream) on the one hand and the removed files on the other hand. The first sort are small changes which only apply to some internals in QScintilla functions. The last sort only remove obviously not needed files, and introducing them back by accident wouldn't really hurt much. Changing the name of the enum symbols on the other hand would be different: that would generate quite a large patch because it's not just the definition that changes but all the uses too. Also the enums are part of the QScintilla API. So we would need to make sure we're not using the new names anywhere in our code because otherwise we would lose compatibility with the original QScintilla library.

Not sure if that's convincing 😉 But I think renaming symbols would be more error-prone (too much for my taste, given that all this is a workaround for an issue which should have a proper solution) than the changes we made so far in QScintilla. As a last resort though, why not 😄

[revolter]

Renaming the enum is definitely a bad idea. But couldn't we remove the enum or the defines instead?

[MKleusberg]

Removing the #defines would mean that the Scintilla files have to include QScintilla files. So the underlying library would include files of the wrapper. That's not ideal and might even end up in some circular include order.
Removing the enum is definitely the better approach. This way the wrapper would just use the constants from the underlying library. I haven't tried this yet but I was suspecting that all the files which include the QScintilla headers then also need to include the Scintilla headers. But this could probably be done with a single #include in the right header file. So it might actually work after all 😄 Still, I wouldn't be surprised if that extra include causes problems somewhere else because of double definitions or so.

[justinclift]

Still, I wouldn't be surprised if that extra include causes problems somewhere else because of double definitions or so.

Maybe solvable by wrapping it inside it's own #ifndef, so there's no double up?

Something like:

#ifndef OUR_CMAKE_INCLUDE
    #define OUR_CMAKE_INCLUDE
    #include <whatever_header.h>
#endif

[mgrojo]

@MKleusberg

All the files and directories included there are generated, too. Interestingly the first two lines are in a different directory than the other ones. If I move them to the bottom of the file, it compiles just fine. So the question seems to be: what can we do to let cmake generate the file in such a way that these two files are included last? I guess to answer this we would need to understand why there are even two different subdirectories. The documentation says:

The custom directories with checksum based names help to avoid name collisions for moc files with the same basename.

But I don't see why there should be a name collision here.

I guess the different generated directory names are because the source headers are in different directories (then their basenames might collide, although it isn't our case):

   ./Qsci/qsciscintillabase.h
   ./Qsci/qsciabstractapis.h
   ./Qsci/qsciapis.h
   ./Qsci/qscilexer.h
   ./Qsci/qscilexercustom.h
   ./Qsci/qscilexersql.h
   ./Qsci/qscilexerjson.h
   ./Qsci/qscilexerhtml.h
   ./Qsci/qscilexerxml.h
   ./Qsci/qscilexerjavascript.h
   ./Qsci/qscilexercpp.h
   ./Qsci/qscilexerpython.h
   ./Qsci/qscimacro.h
   SciClasses.h
   ScintillaQt.h
   SciAccessibility.h

Sci*.h are in Qt4Qt5/ and qsci*.h are in Qt4Qt5/Qsci/. The order in which they are included in mocs_compilation.cpp might be: purely alphabetical, based on the basenames; or might be consequence of a file system scanning searching for header files, and the Qt4Qt5 directory inevitably goes before Qt4Qt5/Qsci. In any case, I don't know what the solution should be and don't have a cmake 3.11 for playing, but an option might be to disable AUTOMOC altogether and use QT5_WRAP_CPP manually.

[mgrojo]

Also worth testing if setting AUTOGEN_PARALLEL to 1 returns to the older behaviour, given the already mentioned reference in the release notes:

When using “AUTOMOC” or “AUTOUIC”, CMake now starts multiple
parallel “moc” or “uic” processes to reduce the build time. A new
“CMAKE_AUTOGEN_PARALLEL” variable and “AUTOGEN_PARALLEL” target
property may be set to specify the number of parallel “moc” or “uic”
processes to start. The default is derived from the number of CPUs
on the host.

[mgrojo]

I just updated to Ubuntu 18.06 and installed CMake 3.13.1. I've reproduced the issue and have tested with AUTOGEN_PARALLEL set to 1 (no success) and with QT5_WRAP_CPP (it is compiling now). I'm going to test with cmake from Ubuntu (3.10.2) and if it also works, I'll commit this change and we will be able to finally close this issue.

[mgrojo]

@adriaandegroot @moll @deepsidhu1313 @ihexon
The problem described in your issues about compiling with CMake 3.11 should be solved now in the master branch. If you pulled the latest version and tested it in your environments, we could close your issues with confidence of being solved.

[justinclift]

Oh fantastic! I'm playing around with some compilation stuff in a Windows VM atm anyway (getting ready to test SQLCipher 4.0.0). I'll update CMake in the VM to the latest version and see if master now builds on windows too. If it does, I'll cherry pick the change across to the v3.11.x branch so that builds as well. 😄

[karim]

Awesome work Manuel. Just tested it with the latest version of CMake (v3.13.1) and it compiled without any problems. 👍 😃

[justinclift]

Whooo Hooo! 😄

[justinclift]

SQLite 3.26.0 was released yesterday. Going to test that as well. 😄

[karim]

Don't forget OpenSSL 1.0.2q too. 😃

[justinclift]

Thanks, hadn't noticed that. Was more planning to check OpenSSL 1.1.x + newer SQLCipher, mainly to get an understanding of whether it builds "as is". Before we even think about what options we need to change for the newer SQLCipher anyway. 😄

[justinclift]

Just cherry-picked the commit across to the v3.11.x branch. It's pretty clearly a win all around. 😺

[justinclift]

Compilation with SQLCipher v4.0.0 works without any changes done, at least when using OpenSSL 1.0.2p. Yeah, that OpenSSL is a bit outdated. I'll try it with OpenSSL 1.1.0j next.

[deepsidhu1313]

I can confirm issue has been resolved, latest builds are deployed to testing ppa for 18.10 and 19.04. Thank you guys!!

[justinclift]

Excellent. Closing this now, as the CMake problem is (finally) fixed. 😄

[MKleusberg]

Awesome 👍 Thanks, Manuel 😄