-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support FreeBSD 14.0 + NetBSD 10.0 + add testing #74
Conversation
Hey @saper @generalmimon @reezer, I went ahead and adjusted defines in a way that it makes it work on modern FreeBSD + modern NetBSD. This fixes problems with compilation + problems with defines resulting in floating point reading functions misbehaving. The only caveat remaining is iconv: it looks like starting circa FreeBSD 10.0 and sometimes in recent NetBSD, new iconv-like engine was adopted, something called CITRUS. The problem with it, it seems to be supporting much smaller set of encodings, and, it's quite forgiving, which makes our tests like "convert B0 30 in GB2312 to UTF-8" not report an error, while it clearly should. My only idea is to specifically detect CITRUS and disable the relevant test. Any objections? |
Excellent find. I think that's a pretty good idea to disable the test. Great work, thank you very much! |
Bruno Haible @bhaible identified and reported multiple issues related to the Citrus iconv being too permissive: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=272382 We should definitely skip that test for now. |
"too permissive" is an adequate description for the third among these bugs. For the other two, "broken" is a better summary. |
I agree and looks good to me as well. Thanks for picking this up! |
// Necessary for proper detection of *BSD | ||
#if !defined(_MSC_VER) | ||
#include <sys/param.h> | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GreyCat I suspect this isn't actually necessary since you're only using the __FreeBSD__
and __NetBSD__
macros, not the BSD
macro which indeed requires #include <sys/param.h>
: https://sourceforge.net/p/predef/wiki/OperatingSystems/#bsd-environment
I think this unnecessary include should be removed. For now, I guess this condition is fine, because the test exclusions should be minimal and these are the only BSD variants that we know are problematic:
#if !defined(__FreeBSD__) && !defined(__NetBSD__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall in my testing that it actually required that include, but maybe I've indeed got something crossed, so if you have an idea what we can eliminate/optimize here — absolutely feel free to go ahead and change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think (at least I haven't seen any single reference on the internet that would suggest otherwise) that sys/param.h
only defines BSD
(see FreeBSD's sys/param.h
, NetBSD's sys/param.h
), whereas __FreeBSD__
and __NetBSD__
are predefined macros by the compiler, so you don't need to include anything to use them.
Adds proven support for FreeBSD and NetBSD. Uses GH Actions for 2 new workflows (which internally download an image for a VM and runs it in qemu emulator on Ubuntu host).