Bug 253870 - devel/libgtop: fails to build on head after commit 2bfd8992c7c7
Summary: devel/libgtop: fails to build on head after commit 2bfd8992c7c7
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Cy Schubert
URL:
Keywords:
Depends on: 253839
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-26 11:01 UTC by Guido Falsi
Modified: 2021-03-05 13:46 UTC (History)
6 users (show)

See Also:
bugzilla: maintainer-feedback? (gnome)


Attachments
AdHoc patch (2.69 KB, patch)
2021-02-26 15:46 UTC, Tomoaki AOKI
no flags Details | Diff
Patch using sys/types.h (2.91 KB, patch)
2021-02-26 16:59 UTC, Guido Falsi
madpilot: maintainer-approval? (gnome)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Guido Falsi freebsd_committer 2021-02-26 11:01:25 UTC
After updating to a recent head I noticed libgtop failing, with error at the end of this message.

The mentioned commit add the bool return type to the inmem function.

I noticed this bool type is defined in sys/types.h, but only if _KERNEL is defined.

So the first time it is included _KERNEL needs to be defined.

I'm trying to get a patch to that effect, but until now I've always been missing some previous inclusion.

I'll followup with it as soon as I have a working patch.


In file included from procmap.c:54:
In file included from /usr/include/ufs/ufs/inode.h:50:
/usr/include/sys/buf.h:569:1: error: unknown type name 'bool'
bool    inmem(struct vnode *, daddr_t);
^
procmap.c:101:25: warning: unused variable 'um' [-Wunused-variable]
        struct ufsmount um;
                        ^
procmap.c:405:16: warning: cast from 'gchar *' (aka 'char *') to 'glibtop_map_entry *' (aka 'struct _glibtop_map_entry *') increases required alignment from 1 to 8 [-Wcast-align]
        return (glibtop_map_entry*) g_array_free(maps, FALSE);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings and 1 error generated.
gmake[4]: *** [Makefile:573: procmap.lo] Error 1
gmake[4]: *** Waiting for unfinished jobs....
Comment 1 Guido Falsi freebsd_committer 2021-02-26 11:03:25 UTC
Forgot to mention, I submitted this information as a bug against base, which resulted as invalid, I'm referencing it here:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=253839
Comment 2 Tomoaki AOKI 2021-02-26 15:46:32 UTC
Created attachment 222847 [details]
AdHoc patch

AdHoc patch to allow build on main.
Applicable on /usr/ports.
Tested only on main at commit 172f2fc11cc560bbd3d3b29260a8ae21df6a541b.
So possibly break other branches (stable/*, releng/*, ...).
Comment 3 Guido Falsi freebsd_committer 2021-02-26 16:59:36 UTC
Created attachment 222851 [details]
Patch using sys/types.h

I'm using a slightly different patch.

Actually that bool type should be the one expected by kernel own sources, which is defined in types.h

Hoisting it's inclusion and forcing _KERNEL defined warrants the correct includes.

I'm not able to judge which one is the best approach.

Obviously I'm biased towards my own patch :)
Comment 4 Tomoaki AOKI 2021-02-27 01:00:30 UTC
(In reply to Guido Falsi from comment #3)

Personally, whichever fix is OK.
Indeed, I first thought of your fix.

But reading bug 253839, I switched to use stdbool.h.

Just FYI, I switched my thought because...
  *sys/types.h introduces much more things, possibly conflict future update.

  *I feel FreeBSD project is going to decrease name space pollution.

  *stdbool.h is NOT a kernel header, while headers in sys directory are
   kernel headers. See HIER (7).

Unfortunately, kernel INTERNAL headers and (if any or not) kernel EXPOSED
headers are mixed up.

Considering what Konstantin said in bug 253839 stricter, in userspace apps,
including <sys/*> would better avoided whenever possible.
Comment 5 Guido Falsi freebsd_committer 2021-02-27 13:05:46 UTC
(In reply to Tomoaki AOKI from comment #4)

Basic problem is that the source itself is including and using system includes and already polluting things with _KERNEL. I grabbed sys/types.h because it is what buf.h seems to expect.

Anyway in this case what we are missing is only the bool definition, and I suspect that the function returning bool and causing the error is not even used in this code.

This means we could get away with a typedef bool to int and everything would work.

I really have no idea what is the "correct" solution. Ideal solution would be fixing this code to not use system includes at all.
Comment 6 Charlie Li 2021-03-03 04:46:37 UTC
I tested the non-kernel header approach; runtime seems fine with sysutils/gnome-system-monitor. I'm inclined to go with this approach due to bug 253839.

Maybe slightly unrelated, but since there have not been any releases since 2.40.0, other operating system distros have tracked commits after/on top of the release tag. Some of those commits include FreeBSD-specific/guarded code.
Comment 7 nyan 2021-03-03 23:36:25 UTC
I just figured out this ticket after submitting a review.
https://reviews.freebsd.org/D29050

Mine approach is slightly different, that to defer the 
inclusion of ufs/ufs/inode.h, therefore, the kernel specific 
symbols in sys/buf.h will not be included at all.
Comment 8 nyan 2021-03-03 23:44:27 UTC
(In reply to nyan from comment #7)
A bit more of the justification to defer the inclusion after
undef _KERNEL is that, #define _KERNEL is userspace is really
hacky and simply feels wrong, since code in _KERNEL are really
meant to be used for and by kernel only, which means there are
no guarantee on api/kpi stability. 

I personally feels a more correct approach will be to avoid 
_KERNEL as much as possible to avoid further changes in kernel 
specific code affects the port.
Comment 9 commit-hook freebsd_committer 2021-03-04 16:15:20 UTC
A commit references this bug:

Author: cy
Date: Thu Mar  4 16:14:45 UTC 2021
New revision: 567321
URL: https://svnweb.freebsd.org/changeset/ports/567321

Log:
  devel/libgtop: Fix build on 14-CURRENT post 2bfd8992c7c7

  sys/inode.h now implicitly includes sys/buf.h which which itself has
  dependencies requiring repositioning of inode.h.

  This patch also references the inode itself instead of relying on the VTOI
  macro thereby removing the need to define the _KERNEL macro when including
  sys/inode.h.

  PR:		253928, 253870
  Submitted by:	nyan _ myuji.xyz
  Reported by:	cy, madpilot
  Reviewed by:	cy
  MFH:		2021Q1
  Differential Revision:	https://reviews.freebsd.org/D29050

Changes:
  head/devel/libgtop/files/patch-sysdeps_freebsd_procmap.c
Comment 10 Guido Falsi freebsd_committer 2021-03-05 09:42:19 UTC
As the reporter, since this is now fixed, should I close this or should this bug stay open for some other reason?
Comment 11 Cy Schubert freebsd_committer 2021-03-05 13:46:09 UTC
Fixed.