glib can no longer be included in extern "C" blocks

Bug #1923642 reported by Heather Ellsworth
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
GnuCash
Fix Released
Critical
gnucash (Debian)
Fix Released
Unknown
gnucash (Ubuntu)
Medium
Heather Ellsworth

Bug Description

Glib newer than 2.66 will not support extern "C" blocks.
For more info: https://gitlab.gnome.org/GNOME/glib/-/issues/2331

Revision history for this message
In , Thomas Klausner (tk-giga) wrote :

There is a breaking change in glib 2.68.0:

https://bugzilla.redhat.com/show_bug.cgi?id=1926239
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1935#note_1034039

In short, the glib headers cannot be included in ``extern "C"'' any longer.

This breaks gnucash 4.4 and 4.5. A lot of error output, looking basically like this:

In file included from /usr/pkg/include/glib-2.0/glib/gatomic.h:31,
                 from /usr/pkg/include/glib-2.0/glib/gthread.h:32,
                 from /usr/pkg/include/glib-2.0/glib/gasyncqueue.h:32,
                 from /usr/pkg/include/glib-2.0/glib.h:32,
                 from /scratch/finance/gnucash/work/gnucash-4.5/libgnucash/engine/SchedXaction.h:42,
                 from /scratch/finance/gnucash/work/gnucash-4.5/gnucash/gnome/assistant-loan.cpp:33:
/usr/include/g++/type_traits:3024:3: error: template with C linkage
 3024 | template<typename _Tp>
      | ^~~~~~~~
/scratch/finance/gnucash/work/gnucash-4.5/gnucash/gnome/assistant-loan.cpp:26:1: note: 'extern "C"' linkage started here
   26 | extern "C"
      | ^~~~~~~~~~

The glib includes are sometimes in other includes - here in SchedXAction.h, so it's not easy to see what needs to be moved.

Revision history for this message
In , Jralls (jralls) wrote :

Rats. assistant_loan.cpp was modified as part of https://github.com/Gnucash/gnucash/pull/912 for this exact problem, but Bill put the glib include after the extern "C" block so it gets included first from SchedXAction.h. We had to do a bunch more to get CI tests to pass on Arch Linux as it had picked up the changes in the meantime.

It's obvious what to do, but I'm a bit curious about why it works on Arch but still fails for you. Are you perhaps building with clang?

Revision history for this message
In , Thomas Klausner (tk-giga) wrote :

No clang, that's with gcc 9.3.0.
I tried turning off aqbanking, ofx, dbi but that didn't help.
I'm not sure what the difference is, sorry.

I played around a bit and found a diff that made it compile on my system:

--- gnucash/gnome/assistant-loan.cpp.orig 2021-03-26 23:08:11.000000000 +0000
+++ gnucash/gnome/assistant-loan.cpp
@@ -23,6 +23,10 @@
  * Boston, MA 02110-1301, USA <email address hidden> *
 \********************************************************************/

+#include <glib.h>
+#include <glib/gi18n.h>
+#include <gtk/gtk.h>
+
 extern "C"
 {
 #include <config.h>
@@ -50,9 +54,6 @@ extern "C"
 #endif
 }

-#include <glib.h>
-#include <glib/gi18n.h>
-#include <gtk/gtk.h>
 #include <gnc-locale-utils.hpp>
 #include <boost/locale.hpp>
 #include <string>

I guess glib's multiple-inclusion-protection in the header saves us here :)

Revision history for this message
In , Jralls (jralls) wrote :

> I guess glib's multiple-inclusion-protection in the header saves us here :)

Exactly. It's also what kills us when #include <glib.h> is after the extern "C" block.

Interesting as well that fixing this one instance gets GnuCash to build: There are several more in the original commit.

Revision history for this message
In , Thomas Klausner (tk-giga) wrote :

Sorry, that's the one I need on top of gnucash 4.5.

Revision history for this message
Heather Ellsworth (hellsworth) wrote :
Revision history for this message
Heather Ellsworth (hellsworth) wrote :
Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "fix-build.patch" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

tags: added: patch
Revision history for this message
Heather Ellsworth (hellsworth) wrote :
Changed in gnucash:
status: Unknown → New
Mathew Hodson (mhodson)
affects: gnucash → ubuntu
Changed in ubuntu:
importance: Unknown → Undecided
no longer affects: ubuntu
Mathew Hodson (mhodson)
tags: added: ftbfs
Changed in gnucash (Ubuntu):
importance: Undecided → Medium
Changed in gnucash (Debian):
status: Unknown → New
Revision history for this message
In , Jralls (jralls) wrote :

Fixed for 4.6. I've moved all of the includes from PR 912.

Changed in gnucash:
importance: Unknown → Critical
status: Unknown → Fix Released
Changed in gnucash (Debian):
status: New → Confirmed
Revision history for this message
Robie Basak (racb) wrote (last edit ):

Looks like upstream has fixed this now with https://github.com/Gnucash/gnucash/commit/bbb4113a5a996dcd7bb3494e0be900b275b49a4f, so it would probably be best if we cherry-picked that commit. Could you prepare an upload with a quilt patch and dep3 headers against that upstream commit please, and I'll sponsor it? Or I can do it, but I don't want to steal your credit :)

Changed in gnucash (Ubuntu):
assignee: nobody → Heather Ellsworth (hellsworth)
Revision history for this message
Heather Ellsworth (hellsworth) wrote :

This was just a case of not getting notified of my PR's activity and forgetting.. sorry bout that.

I've updated the PR to get the fix pulled in debian.

Revision history for this message
Robie Basak (racb) wrote :

Thank you for working on getting this pulled into Debian!

What's your plan for Ubuntu? Are you holding on to see if Debian uploads soon so that Ubuntu can sync it? Or do you want your fix uploaded into Ubuntu ahead of Debian, pending it being fixed in Debian?

I would normally expect the latter for an FTBFS fix in Ubuntu, but if that's not your plan then we can leave it open. I'm just not sure what you intend because this bug still has ~ubuntu-sponsors subscribed but there's nothing ready for upload in Ubuntu right now, I don't think?

Revision history for this message
Heather Ellsworth (hellsworth) wrote :

I've just put the patch on top of 4.4 that is in impish and uploaded the build artifacts to: https://launchpad.net/~hellsworth/+archive/ubuntu/gnucash

Revision history for this message
Gunnar Hjalmarsson (gunnarhj) wrote (last edit ):

@Heather: Debian chose to fix it by updating to a new upstream release (version 4.6). Syncing into impish would require a FFe approval, I suppose:

https://github.com/Gnucash/gnucash/releases/tag/4.6

Better do that in next cycle IMO, and use your patch in Ubuntu for now.

However, the PPA build failed...

Revision history for this message
Gunnar Hjalmarsson (gunnarhj) wrote :

I digged deeper into it. The patch in your PPA actually just creates another patch, and doesn't change any upstream files. If you had done it right, you'd have found that it doesn't apply by itself.

I uploaded a fix. Besides the upstream commit which Robie mentioned, I included six other commits to make it work.

Changed in gnucash (Ubuntu):
status: New → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package gnucash - 1:4.4-1ubuntu1

---------------
gnucash (1:4.4-1ubuntu1) impish; urgency=medium

  * Fix FTBFS due to glib headers in 'extern "C"' blocks (LP: #1923642)
    - No longer permitted with glib2.0 2.67.3+
    - The fix consists of these upstream commits:
      + Fix-build-with-glib2-2.67.x.patch
      + Move-glib-and-gtk-includes-out-of-extern-C-for-tests.patch
      + Potentially-fix-CI-test-on-Arch-related-to-glib-and-c++.patch
      + More-fixes-for-Arch-ci-failure.patch
      + Still-more-fixes-for-Arch-ci-failure.patch
      + Finish-the-glib-2.67-fixes-for-CI-tests.patch
      + Bug-798156-glib-2.68.0-breaks-gnucash.patch
    - Thanks to Heather Ellsworth for researching the issue!

 -- Gunnar Hjalmarsson <email address hidden> Wed, 08 Sep 2021 06:48:41 +0200

Changed in gnucash (Ubuntu):
status: Fix Committed → Fix Released
Revision history for this message
Heather Ellsworth (hellsworth) wrote :

@Gunnar, Thanks! I didn't realize that the 4.6 that debian pulled fixes the issue. There was nothing in the 4.6 commits that suggested this issue had been fixed, so did you just build and test it to see? Or how did you know that 4.6 fixes the issue?

https://salsa.debian.org/debian/gnucash/-/compare/f9461670c5f516e31631a80975eebf6b6e56a1c6...c45dcd6db80fc45ef60cc5d30eb5612f6b64ce9d

I don't doubt you at all, just trying to figure out what I missed :)

Also, this creating of a patch worked 2 months ago to get past the build issue.. so yesterday I assumed it would still work. But it looks like there are more files to patch now and you've done that with your patches. But, why not put all of the patches into one big patch?

Revision history for this message
Brian Murray (brian-murray) wrote :

Keeping the patches separate is good practice in that they might not all be accepted upstream at the same time. Using this strategy its easier to drop individual patches, when merging with a new upstream version, then modifying one big patch.

Revision history for this message
Gunnar Hjalmarsson (gunnarhj) wrote :

On 2021-09-08 16:54, Heather Ellsworth wrote:
> @Gunnar, Thanks! I didn't realize that the 4.6 that debian pulled
> fixes the issue. There was nothing in the 4.6 commits that suggested
> this issue had been fixed, so did you just build and test it to see?
> Or how did you know that 4.6 fixes the issue?

First and foremost I noticed that gnucash 1:4.6-1 built on Debian with version 2.68 of glib2.0. Also, the "Bug 798156 - glib 2.68.0 breaks gnucash" commit predates the upstream 4.6 release. So I didn't build/test myself.

> https://salsa.debian.org/debian/gnucash/-/compare/f9461670c5f516e31631a80975eebf6b6e56a1c6...c45dcd6db80fc45ef60cc5d30eb5612f6b64ce9d

The master branch of the gnucash repo seems to only include the debian/ directory, not the upstream files.

> I don't doubt you at all, just trying to figure out what I missed :)

Sure, n.p.

> Also, this creating of a patch worked 2 months ago to get past the
> build issue..

Well, the "Bug 798156 - glib 2.68.0 breaks gnucash" patch never was sufficient on top of version 4.4 of gnucash. It didn't even apply without first applying a bunch of other commits. Probably it makes a difference on top of version 4.5, though.

What you put in your Debian MR seems to be the same as in your PPA. Neither of them changes anything in substance. So it appears you never built with that patch before submitting your MR. Compare your MR with the equivalent patch I included in the fix to understand what I'm talking about.

> But, why not put all of the patches into one big patch?

In addition to Brian's comment on that, I can say that the fix is the result of some trial and error. Concatenating the patches would have been additional work. :/

TBH I fear that there would have been a much simpler solution:

https://gitlab.gnome.org/GNOME/glib/-/issues/2331#note_1067322

But I saw that comment only when the fix was working, and now I'm disinclined to try it with the risk to find out that the work I did was wasted...

Revision history for this message
Heather Ellsworth (hellsworth) wrote :

> Keeping the patches separate is good practice in that they might not all be accepted upstream at the same time.

Ah that makes sense!

> The master branch of the gnucash repo seems to only include the debian/ directory, not the upstream files.

This threw me off and that's why I made a patch that made a patch, thinking that's what would have had to happen. I see that while that needs to happen sometimes for some projects (libreoffice), it's not a hard rule.

> Well, the "Bug 798156 - glib 2.68.0 breaks gnucash" patch never was sufficient on top of version 4.4 of gnucash.

I could have sworn I tested it but looking back at my notes from when I first opened the PR, you must be right. I had definitely convinced myself that it fixed the issue but I was wrong.

> What you put in your Debian MR seems to be the same as in your PPA.
Yep, I was rushing and had incorrectly remembered that the patch had fixed the problem and was hoping it still would fix it.

> TBH I fear that there would have been a much simpler solution:
>
> https://gitlab.gnome.org/GNOME/glib/-/issues/2331#note_1067322
>
> But I saw that comment only when the fix was working, and now I'm disinclined to try it with the risk to find out that the work I did was wasted...

Darnit I should have caught this too.

@Gunnar, @Brian thanks so much for answering my questions. I'll try to learn from this experience and avoid these same mistakes next time :)

Changed in gnucash (Debian):
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.