Re: wip/glusterfs to pkgsrc filesystems/glusterfs

[Alexander Schreiber <als%thangorodrim.ch@localhost>]

On Wed, Mar 01, 2023 at 09:46:47AM +0100, Jonathan Schleifer wrote:
> 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.

Already changed.

> 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 $
> > > > +$NetBSD$
> > > 
> > > I know the current package already does this, but it's really bad practice.
> > > Only patch configure.ac and then regenerate. Do not patch the generated
> > > file.
> > 
> > Just patching configure.ac 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?

I'll give it another try and will report back.

> > > 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.

Patch description updated. I also need to split the configure patches.

(none of the changes yet committed and pushed, all still too fluid)

Kind regards,
            Alex.
-- 
"Opportunity is missed by most people because it is dressed in overalls and
 looks like work."                                      -- Thomas A. Edison