Summary: | devel/libgtop: fails to build on head after commit 2bfd8992c7c7 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | Ports & Packages | Reporter: | Guido Falsi <madpilot> | ||||||
Component: | Individual Port(s) | Assignee: | Cy Schubert <cy> | ||||||
Status: | Closed FIXED | ||||||||
Severity: | Affects Only Me | CC: | cy, gnome, junchoon, madpilot, nyan, vishwin | ||||||
Priority: | --- | Flags: | bugzilla:
maintainer-feedback?
(gnome) |
||||||
Version: | Latest | ||||||||
Hardware: | Any | ||||||||
OS: | Any | ||||||||
Bug Depends on: | 253839 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Guido Falsi
2021-02-26 11:01: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 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/*, ...).
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 :)
(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. (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. 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. 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. (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. 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 As the reporter, since this is now fixed, should I close this or should this bug stay open for some other reason? Fixed. |