Hi Omar,
Omar Polo wrote on Fri, Dec 03, 2021 at 08:00:03PM +0100:
> Woops, I identified a correct issue in unistd.c, but the fix was wrong.
> Updated tarball attached and upstream notified :)
>
> (update: upstream has accepted one of the two patches)
>
> Also enabling the built-in tests this time, I haven't noticed them the
> first time because without --enable-tests `make test' was successful and
> listed 0 tests available. (The mkstemp test fails because why check
> what malloc returns?)
Failing to check return values is bad, too, but not the root cause of
the failure. That test is attempting to allocate (2^24 - 1) * 15 =
251658225 bytes of memory in a single malloc(3) call, about 240 MB,
because on OpenBSD, TMP_MAX is 0x7fffffff. Running that particular
test makes no sense until upstream fixes it.
The test src/tests/buildinfo.bats is also broken. It assumes that,
if you have a file "./utility" that start with the first line
"#!/bin/sh" and a file "./argument" that starts with the first
line "#!./utility" and then run "./argument", that the kernel
would run "#!/bin/sh ./utility ./argument". But our kernel
runs "#!/bin/sh ./argument" instead.
The buildinfo.bats issue is more or less unfixable because on the
one hand, the automake infrastructure in Makefile.am and the
script config/aux/tap-driver.sh require that the TESTS make
variable must contain the name of the test *data* file, but then
turn around and tell the kernel to *execute* that data file.
To make that portable, upstream needs to completely restructure
that maze of indirections and abstractions.
On top of that, the tests require gmake(1) and bash(1) while
nothing in the actual software requires those.
So half the tests (2 out of 4) are broken and completely non-portable.
The "select" test is completely trivial and certainly does not test
select(2) in any meaningful way. So what remains as potentially
working and non-trivial is one single test, the "spawn" test. I don't
think that is worth depending on gmake and bash and all the trouble
i described above. Consequently, i suggest to just forget about the
tests until upstream gets their shit together and makes sure they
are portable, work, and test some non-trivial features.
I mean, come on, this is supposed to be a portability library -
but all the same, the test suite is thoroughly Linux-only...
Next problem to consider for upstream: when run with automake 1.16
and autoconf 2.71, the build complains loudly:
configure.ac:53: warning: The macro `AC_PROG_CC_C99' is obsolete.
configure.ac:53: You should run autoupdate.
/usr/local/share/autoconf-2.71/autoconf/c.m4:1662:
AC_PROG_CC_C99 is expanded from...
configure.ac:53: the top level
configure.ac:67: warning: The macro `AC_LANG_C' is obsolete.
configure.ac:67: You should run autoupdate.
/usr/local/share/autoconf-2.71/autoconf/c.m4:72:
AC_LANG_C is expanded from...
config/m4/ax_pthread.m4:283: AX_PTHREAD is expanded from...
configure.ac:67: the top level
configure.ac:67: warning: The macro `AC_TRY_LINK' is obsolete.
configure.ac:67: You should run autoupdate.
/usr/local/share/autoconf-2.71/autoconf/general.m4:2921:
AC_TRY_LINK is expanded from...
config/m4/ax_pthread.m4:283: AX_PTHREAD is expanded from...
configure.ac:67: the top level
So that is apparently newer than the autotools versions they
are aiming for. But when trying to use the autotools versions
advertised in README.md, automake-1.10 and autoconf-2.67, the
build crashes instantly, so those are apparently too old.
There are more problems with the software. For example,
in src/lib/install_signalfd.c, i spotted a signal handler
that calls snprintf(3). I don't doubt there is more.
Then again, correctness of upstream code isn't really
a requirement for a port...
I'm suggesting the version of the Makefile appended below,
with the following changes:
* Remove BUILD_DEPENDS = devel/check
The build does not need it, only the tests might, some day.
* Remove USE_GMAKE = Yes
Again, the build does not need it, only the tests do.
* Add SEPARATE_BUILD = Yes
Upstream strongly recommends that, and faq/ports/testing.html
recommends it, too.
* Add CONFIGURE_ENV += ac_cv_prog_GIT=
That achieves the same as patch-config_m4_custom_checks_m4 ,
but in a much simpler way. Do forget to remove that patch
if you agree with this change.
* Add a comment explaining why the test suite makes no sense
just yet, with an incomplete list of changes that would be
needed to enable it, all commented out, such that we don't
forget what we already found out.
* Add a comment about the autotools versions.
I think someone should talk to upstream and find out
which versions this software is actually designed for.
Feel free to use part or all of this in any way you want.
I don't want to commit it, but you might. :-)
Yours,
Ingo
# $OpenBSD$
COMMENT = thin wrapper over libc functions and macros
GH_ACCOUNT = sionescu
GH_PROJECT = libfixposix
GH_TAGNAME = v0.4.3
SHARED_LIBS += fixposix 0.0 # 5.0
CATEGORIES = devel
MAINTAINER = Omar Polo <op@omarpolo.com>
# Boost SL 1.0
PERMIT_PACKAGE = Yes
WANTLIB += pthread
SEPARATE_BUILD = Yes
CONFIGURE_STYLE = autoreconf
CONFIGURE_ENV += ac_cv_prog_GIT=
# The test suite is almost trivial, quite non-portable,
# and utterly broken, so leave it disabled for now.
# There are only four tests: two are broken and one is trivial.
# The test suite requires gmake and bash.
# The presence of the check(1) utility is tested by ./configure
# unless the following magical incantation is used:
#CONFIGURE_ARGS += --enable-tests CHECK_CFLAGS=' ' CHECK_LIBS=' '
#TEST_DEPENDS = devel/check shells/bash
#USE_GMAKE = Yes
# These versions are not quite right, they are obviously too new
# and throw multiple warnings. But the versions recommended by
# upstream in README.md are too old and don't work at all...
AUTOCONF_VERSION = 2.71
AUTOMAKE_VERSION = 1.16
.include <bsd.port.mk>
No comments:
Post a Comment