Bug 1396755 - moc-qt4 issues with QT_VERSION_CHECK : Parse error at "defined"
Summary: moc-qt4 issues with QT_VERSION_CHECK : Parse error at "defined"
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: qt
Version: 26
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1401709 (view as bug list)
Depends On:
Blocks: 1396676 1398080 1399532 1400240 1401516 1401674
TreeView+ depends on / blocked
 
Reported: 2016-11-19 19:07 UTC by Orion Poplawski
Modified: 2017-03-13 13:14 UTC (History)
15 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-03-13 13:14:33 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Orion Poplawski 2016-11-19 19:07:50 UTC
Description of problem:

[ 15%] Generating covers/moc_coversearchstatisticsdialog.cxx
cd /builddir/build/BUILD/clementine-1.3.1/x86_64-redhat-linux-gnu/src/covers && /usr/lib64/qt4/bin/moc-qt4 @/builddir/build/BUILD/clementine-1.3.1/x86_64-redhat-linux-gnu/src/covers/moc_coversearchstatisticsdialog.cxx_parameters
/usr/include/glib-2.0/glib/gversionmacros.h:52: Parse error at "defined"

Version-Release number of selected component (if applicable):
clementine-1.3.1-4.fc26

Comment 1 Jonathan Wakely 2016-11-21 13:12:47 UTC
Probably due to https://bugreports.qt.io/browse/QTBUG-22829 but not helped by the fact that moc prints the wrong filename in the error (I'm seeing something similar when trying to update boost in rawhide and rebuild the packages that use it).

Comment 2 Rex Dieter 2016-11-21 13:21:57 UTC
We currently carry,
http://pkgs.fedoraproject.org/cgit/rpms/qt.git/tree/qt-everywhere-opensource-src-4.8.6-QTBUG-22829.patch

as a workaround for currently known issues.  If you have suggestions for additions, we can do so.

Comment 3 Kevin Kofler 2016-11-22 13:47:14 UTC
Interestingly, the file name appears to be nonsense, but the line number is always 52. (Why FIFTY-two? Isn't the answer to everything FOURTY-two? ;-) )

Other packages got:
/usr/include/QtWebKit/qwebpage.h:52
and:
/usr/include/boost/predef/os/windows.h:52
as the claimed fault location.

Comment 4 Kevin Kofler 2016-11-23 18:17:28 UTC
*** Bug 1396676 has been marked as a duplicate of this bug. ***

Comment 5 Rex Dieter 2016-11-29 12:16:01 UTC
*** Bug 1399532 has been marked as a duplicate of this bug. ***

Comment 6 Rex Dieter 2016-11-29 12:44:47 UTC
adjusting summary

Comment 7 Orion Poplawski 2016-12-02 03:49:08 UTC
engrid is FTBFS due to this as well.

Looks like adding:

BOOST_PREDEF_VERSION_NUMBER_H

will help a lot.

Comment 8 Orion Poplawski 2016-12-02 04:16:21 UTC
Also ended up adding __G_VERSION_MACROS_H__ for the clementine build issue.  Building qt-4.8.7-22.fc26 now.

Comment 9 Kevin Kofler 2016-12-02 05:19:55 UTC
I don't see how that is anywhere near a valid fix. I think it is just a workaround that puts the bug back into latent state, and does not sustainably address it.

The only recent change in gversionmacros.h is this:
https://git.gnome.org/browse/glib/commit/glib/gversionmacros.h?id=67ce53058102905ac3c8f6f57b044616301d479b
which just adds more of the same definitions that had parsed just fine so far.

That said, the number "52" appears several times in there, which might be where the mysterious bogus "line 52" really comes from. Or it might not.

But this looks a lot like some kind of memory corruption bug in moc-qt4, possibly even a miscompilation of moc-qt4 by g++, not like anything particular in the source files being parsed (unlike the Boost issues we worked around with such #defines in the past, where we clearly identified unsupported constructs in the files we were disabling that way).

Comment 10 Kevin Kofler 2016-12-02 05:22:23 UTC
E.g., we had an error at /usr/include/QtWebKit/qwebpage.h:52, I don't think excluding qwebpage.h from moc is going to do what we want. And I think the actual file is actually completely irrelevant, given how the error always claims to be at line 52. It can probably happen in any file.

Comment 11 Orion Poplawski 2016-12-02 05:28:31 UTC
Until someone backports the fixes from qt5-moc into qt4-moc, I don't think we have much available to us other than these workarounds.  You are correct that the file name is not quite correct - it appears to be perhaps one up on the stack.  Some trial and error testing led me to the two headers that I've set to be excluded and appear to allow engrid and clementine to build.

Comment 12 Kevin Kofler 2016-12-02 05:43:45 UTC
It makes those 2 packages build, excluding a different header each time. It will not do anything to fix the packages breaking in other places.

Comment 13 Jonathan Wakely 2016-12-02 11:56:29 UTC
Allowing the Boost.Predef header to be skipped will fix the build for a lot more than two packages. Most of the Boost rebuild failures I'm seeing are caused by this, and all point to a predef header. Obviously it's only a workaround not a solution, but that's still a valid fix, because the Qt team aren't going to backport the real fix to moc, and this is affecting dozens of packages in Fedora.

If you have a fix for moc please propose it, otherwise please don't be so critical of small improvements that enable other people to make progress.

Comment 14 Kevin Kofler 2016-12-02 12:14:48 UTC
There is nothing to backport from Qt 5, moc-qt5 uses a completely different parser.

Comment 15 Kevin Kofler 2016-12-02 12:16:40 UTC
And my problem with the workaround is not that it is a workaround, but that it works around symptoms, not the real source of the error, which has not been identified yet.

Comment 16 Julian Sikorski 2016-12-05 06:16:05 UTC
qmc2 seems to fail due to this as well:
http://koji.fedoraproject.org/koji/buildinfo?buildID=821821
0.69 built fine with qt-devel-1:4.8.7-19.fc26. -21 and -22 fail with
/usr/lib64/qt4/bin/moc -DQMC2_SDLMAME -DQMC2_VERSION=0.70 -DQMC2_SVN_REV=0 -DBUILD_OS_NAME=Linux -DBUILD_OS_RELEASE=4.8.11-300.fc25.x86_64 -DBUILD_MACHINE=x86_64 -DPREFIX=/usr/local -DDATADIR=/usr/local/share -DSYSCONFDIR=/etc -DQMC2_JOYSTICK=1 -DQMC2_PHONON=1 -DQMC2_MULTIMEDIA=0 -DQMC2_FADER_SPEED=500 -DQMC2_BROWSER_EXTRAS_ENABLED -DQMC2_BROWSER_PLUGINS_ENABLED -DQMC2_BROWSER_JAVA_ENABLED -DQMC2_BROWSER_JAVASCRIPT_ENABLED -DQMC2_YOUTUBE_ENABLED -DQMC2_LIBARCHIVE_ENABLED -D_REENTRANT -D_7ZIP_PPMD_SUPPORT -D_7ZIP_ST -DQT_NO_DEBUG -DQT_WEBKIT_LIB -DQT_PHONON_LIB -DQT_SVG_LIB -DQT_SQL_LIB -DQT_XMLPATTERNS_LIB -DQT_XML_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_CORE_LIB -DQT_SHARED -I/usr/lib64/qt4/mkspecs/linux-g++ -I. -I/usr/include/QtCore -I/usr/include/QtNetwork -I/usr/include/QtGui -I/usr/include/QtXml -I/usr/include/QtXmlPatterns -I/usr/include/QtSql -I/usr/include/QtSvg -I/usr/include/phonon -I/usr/include/QtWebKit -I/usr/include -I/usr/include/QtTest -I/usr/include/SDL2 -Ilzma -I/usr/include/minizip -I/usr/include/phonon_compat -I. -I. docbrowser.h -o moc_docbrowser.cpp
/usr/include/QtWebKit/qwebpage.h:52: Parse error at "defined"
make[1]: *** [Makefile.qmake:668: moc_docbrowser.cpp] Error 1

Comment 17 Kevin Kofler 2016-12-05 14:55:51 UTC
So we have at least /usr/include/QtWebKit/qwebpage.h and /builddir/build/BUILD/webkit-qtwebkit-23/Source/WTF/wtf/TypeTraits.h as further examples of headers that trigger this bug. There are probably more. Do you see now why I am saying that the workaround is WRONG, does not really fix anything, and does not scale?

We first need to figure out what header is ACTUALLY causing the issue and then skip THAT header, not the ones that run into trouble due to it. As long as we have not identified the actual source of the error, we are only trying (and failing) to paper over the problem.

Comment 18 Kevin Kofler 2016-12-05 14:58:35 UTC
That the headers that the workaround is trying to skip are NOT the source of the problem should also be obvious from the fact that the same headers run through moc just fine on other Fedora versions. (That is also the big difference from the Boost workaround, where the header that was skipped was actually the correct one. It is the difference between a valid workaround and broken symptom treatment.)

Comment 19 Rex Dieter 2016-12-05 15:07:52 UTC
Fwiw, I agree with orion, this is not an either/or proposition.  Finding short-term fixes does not preclude finding any ideal long-term solutions.

Comment 20 Kevin Kofler 2016-12-05 20:32:44 UTC
/builddir/build/BUILD/kopete-16.08.3/protocols/jabber/libiris/src/irisnet/noncore/cutestuff/bytestream.h

Yet another header.

Comment 21 Kevin Kofler 2016-12-05 21:05:39 UTC
This is the header it's failing on:
https://cgit.kde.org/kopete.git/tree/protocols/jabber/libiris/src/irisnet/noncore/cutestuff/bytestream.h
I see a QT_VERSION_CHECK(5,0,0) used there too. So my theory that it might have something to do with version check macros might still hold.

Comment 22 Orion Poplawski 2016-12-05 21:52:42 UTC
Well, it chokes on boost and glib version check macros as well, so seems likely.

Comment 23 Rex Dieter 2016-12-08 18:44:02 UTC
After adding some debugging and educated guesses, we found that *something* in rawhide was defining macros 'major' 'minor' which are both commonly used in other code (often for version checks).  We suspect glibc's sysmacros.h to be the culprit, so we're testing 2 changes:

1.  patching QT_VERSION_CHECK to use safer/namespaced variables
2.  patch moc to define _SYS_SYSMACROS_H

http://koji.fedoraproject.org/koji/buildinfo?buildID=823761

Comment 24 Jonathan Wakely 2016-12-08 18:51:01 UTC
Ugh. This is because g++ auto-defines _GNU_SOURCE which makes glibc define all sorts of polluting names.

Comment 25 Kevin Kofler 2016-12-08 21:01:58 UTC
The problem is of course that moc does not implement the preprocessor correctly, the macro parameters should be shadowing the glibc macros, and also, the glibc macros are function-like macros and should thus only be expanded in a function-like context, which is not given here. But defining _SYS_SYSMACROS_H is the most effective workaround.

Comment 26 Rex Dieter 2016-12-09 05:03:13 UTC
OK, I *think* I can confirm qt(4) is fixed, closing this.

%changelog
* Thu Dec 08 2016 Rex Dieter <rdieter> - 1:4.8.7-24
- namespace QT_VERSION_CHECK to workaround major/minor being pre-defined (#1396755)
- update QTBUG-22829.patch to define _SYS_SYSMACROS_H (#1396755)

Comment 27 Kevin Kofler 2016-12-09 05:16:22 UTC
As per IRC: Looking at the current code:
http://sourceware.org/git/?p=glibc.git;a=blob;f=misc/sys/sysmacros.h;hb=HEAD
we need _SYS_SYSMACROS_H_OUTER instead of _SYS_SYSMACROS_H. _SYS_SYSMACROS_H does not actually help. Reopening.

The QT_VERSION_CHECK workaround can then also be dropped, the moc one should be sufficient on its own.

Comment 28 Kevin Kofler 2016-12-09 05:23:12 UTC
kdelibs builds fine against the current Qt 4, but that's due to the QT_VERSION_CHECK workaround, which does not help the other affected version check macros (e.g. Boost, GLib) (which is why I think you should remove that QT_VERSION_CHECK one, so that we actually test the real workaround).

Comment 29 Florian Weimer 2016-12-09 09:41:53 UTC
(In reply to Rex Dieter from comment #26)
> OK, I *think* I can confirm qt(4) is fixed, closing this.
> 
> %changelog
> * Thu Dec 08 2016 Rex Dieter <rdieter> - 1:4.8.7-24
> - namespace QT_VERSION_CHECK to workaround major/minor being pre-defined
> (#1396755)
> - update QTBUG-22829.patch to define _SYS_SYSMACROS_H (#1396755)

That's not valid.  The name _SYS_SYSMACROS_H might change at any time.

What's odd here is that on the glibc side, we plan to remove the automatic include of <sys/sysmacros.h> in a future version, and this bug reads like major/minor is defined in *more* contexts, which is exactly the opposite of the direction we want to move in.

Comment 30 Kevin Kofler 2016-12-09 12:18:45 UTC
> That's not valid.  The name _SYS_SYSMACROS_H might change at any time.

Then we will change our moc workaround. (In fact, we should be defining _SYS_SYSMACROS_H_OUTER instead to begin with, see comment #27. Of course, that is even more likely to change at some point, but for now, it will do what we want.)

> What's odd here is that on the glibc side, we plan to remove the automatic 
> include of <sys/sysmacros.h> in a future version, and this bug reads like 
> major/minor is defined in *more* contexts, which is exactly the opposite of the 
> direction we want to move in.

I think it's actually the new, more complex CONTENTS of the definitions (with the deprecation magic) that moc chokes on.

Comment 31 Rex Dieter 2016-12-09 13:24:08 UTC
 
qt.spec
%changelog
* Fri Dec 09 2016 Rex Dieter <rdieter> - 1:4.8.7-25
- update QTBUG-22829.patch to use _SYS_SYSMACROS_H_OUTER instead (#1396755)

(and related)
qt5-qtbase.spec
%changelog
* Fri Dec 09 2016 Rex Dieter <rdieter> - 5.7.1-8
- update moc patch to define _SYS_SYSMACROS_H_OUTER instead (#1396755)

Comment 32 Florian Weimer 2016-12-09 13:35:58 UTC
(In reply to Kevin Kofler from comment #30)
> > That's not valid.  The name _SYS_SYSMACROS_H might change at any time.
> 
> Then we will change our moc workaround. (In fact, we should be defining
> _SYS_SYSMACROS_H_OUTER instead to begin with, see comment #27. Of course,
> that is even more likely to change at some point, but for now, it will do
> what we want.)

Oh well, I now understand there is very little alternative.

> > What's odd here is that on the glibc side, we plan to remove the automatic 
> > include of <sys/sysmacros.h> in a future version, and this bug reads like 
> > major/minor is defined in *more* contexts, which is exactly the opposite of the 
> > direction we want to move in.
> 
> I think it's actually the new, more complex CONTENTS of the definitions
> (with the deprecation magic) that moc chokes on.

Right, if moc pretends to be __GNUC__, then it must be able to handle these complex macros.

Comment 33 Raphael Groner 2016-12-11 17:46:00 UTC
*** Bug 1401709 has been marked as a duplicate of this bug. ***

Comment 34 Volker Fröhlich 2016-12-25 10:59:51 UTC
QGIS is building fine again now.

Comment 35 Fedora End Of Life 2017-02-28 10:38:22 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 26 development cycle.
Changing version to '26'.

Comment 36 Rex Dieter 2017-03-13 13:14:33 UTC
I think all the related known issues are fixed, I'll take the liberty of closing this now.


Note You need to log in before you can comment on or make changes to this bug.