tech-pkg archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: wip/glusterfs to pkgsrc filesystems/glusterfs

Am 01.03.23 um 00:49 schrieb Alexander Schreiber:

Maybe ONLY_FOR_PLATFORM would be better here? This smells like the code
isn't written portably, so probably will fail on anything but amd64 and
aarch64 anyway?

I've successfully tested (built & run) this on all 64bit platforms I have
available for testing: aarch64, amd64, sparc64. It cannot run (but can be
built) on any 32bit platform, since the code tries to atomically update
a 64bit value eventually (userspace-rcu), which is not something that is
going to work on 32bit (unless you got a weird one). New 32bit platforms
are unlikely to spring up, but pkgsrc help already mentions e.g. riscv64.

I would prefer a cleaner ONLY_FOR_64BIT (or similar), but that doesn't
appear to exist.

There were two portability issues that broke the build on !Linux, but the
patches address that. And in fact it was alignment-related breakage
(SIGBUS) on sparc64 (fixed sometime between 9.3 and 10.3 upstream) that
got me to work on this ;-)

In that case, ${LP32PLATFORMS} as Thomas recommended is the right way to go. I didn't expect it to work on anything but the big 64 bit platforms, nice to hear it works on SPARC64 :).

-$NetBSD: patch-configure,v 1.5 2020/09/27 01:13:11 manu Exp $

I know the current package already does this, but it's really bad practice.
Only patch and then regenerate. Do not patch the generated

Just patching and regenerating was my initial approach
because it is obviously the right way to do. Except that ran into a
whole pile of issues, which made me realize _why_ people do the ugly
hack of patching ./configure.

Let's fix this properly. What issues did you run into?

patch-libglusterfs-src-glusterfs-dict.h is a bit sketchy. How about checking
for __LP64__ instead?

Sorry, that is wrong and makes the same mistake as the original code
of using machine word size as a (very bad) proxy for sizeof(time_t).
In fact, NetBSD is the prime example why this is wrong: on a 32bit
machine arch (i386), NetBSD still has sizeof(time_t) == 8. _Actually_
having configure check for the value of sizeof(time_t) is the only
correct approach here.

That's not what the description of the patch says:

Do not take for granted that __WORDSIZE is defined to distinguish between 32 and 64 bit platforms. Instead use ULONG_MAX from <limits.h>
which is mandated by ISO C99.

__LP64__ would probably be the best fallback for that.

Still not a good solution, as __WORDSIZE is used as a proxy for sizeof(time_t),
but some superficial tests indicate that sizeof(time_t) seems to track with
machine word size on reasonably current platforms.

AC_CHECK_SIZEOF(time_t) would be the correct solution for that, and not infer it, as you say yourself.

Now, looking at the actual patch, that seems to be what it does, so you'll need to fix the patch description.


Home | Main Index | Thread Index | Old Index