Re: wip/glusterfs to pkgsrc filesystems/glusterfs

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

On Mon, Feb 27, 2023 at 11:17:31PM +0100, Jonathan Schleifer wrote:
> Am 27.02.23 um 22:18 schrieb Alexander Schreiber:
> 
> > Hi,
> > 
> > so I've updated pkgsrc wip/glusterfs to 10.3 and didn't get any complaints
> > after announcing that here and on pkgsrc-users (I hope at least someone
> > else tried it).
> > 
> > How to I get this from wip to actual pkgsrc?
> 
> The best is to send a patch against what's currently in pkgsrc proper. E.g.:
> 
>  $ diff -Nru --exclude=CVS /usr/pkgsrc/filesystems/glusterfs wip/glusterfs

Ok, will do once I've addressed all comments.

> Doing this, there's a few things that I see:
> 
> > -DISTNAME=      glusterfs-8.2
> > -PKGREVISION=   8
> > +DISTNAME=      glusterfs-10.3
> > +PKGREVISION=   9
> 
> If you update the version, PKGREVISION should be reset.

Already deleted as recommended by Thomas - apparently I've read the
pkgsrc guide wrong for that one.

> > +BROKEN_ON_PLATFORM+=    *-*-earm*
> > +BROKEN_ON_PLATFORM+=    *-*-hppa
> > +BROKEN_ON_PLATFORM+=    *-*-i386
> > +BROKEN_ON_PLATFORM+=    *-*-m68000
> > +BROKEN_ON_PLATFORM+=    *-*-m68k
> > +BROKEN_ON_PLATFORM+=    *-*-mipseb
> > +BROKEN_ON_PLATFORM+=    *-*-mipsel
> > +BROKEN_ON_PLATFORM+=    *-*-powerpc
> > +BROKEN_ON_PLATFORM+=    *-*-riscv32
> > +BROKEN_ON_PLATFORM+=    *-*-sparc
> > +BROKEN_ON_PLATFORM+=    *-*-vax
> 
> 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 ;-)

> If there's users of the old package for which this update drops support, we
> might need to make this a versioned package.
> 
> > -SUBST_FILES.etc+=      libglusterfs/src/logging.c
> > -SUBST_FILES.etc+=      doc/glusterfsd.8
> > +SUBST_FILES.etc=       libglusterfs/src/logging.c
> > +SUBST_FILES.etc=       doc/glusterfsd.8
> 
> Why are you removing the + here? Especially, with your version, substitution
> is only performed in doc/glusterfsd.8 and not in logging.c.

Not quite sure how that happened, fixing.

> > +TOOL_DEPENDS+= automake>=1.13:../../devel/automake
> 
> This shouldn't be needed, as you already add it to USE_TOOLS.

fixing

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

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

> I believe for the removed patches you have verified they have indeed been
> upstreamed and made it into the release?

Yes, I explicitly checked and they have all been upstreamed since at
least glusterfs 9.3, which is why I removed them.

> > I'm happy to take over pkgsrc filesystems/glusterfs, given that:
> >   - it looks abandoned, since it is stuck at 8.2 (from 2020-09-16)
> >   - I'm actually running glusterfs storage so I'm interested in keeping
> >     this up to date (and yes, I've seen that 11.0 is out now)
> >   - I'm working on getting my two portability patches merged into
> >     upstream glusterfs (1/2 done)
> > 
> > I've been happy pkgsrc user for many years, now I'd like to contribute
> > as well.
> > 
> > So, how do I got about getting this done?
> 
> I would propose to address the issues above and then I'm happy to import it.
> Feel free to set yourself as MAINTAINER. Also feel free to send further
> changes my way.

I'll go through the recommendations from both you and Thomas and will
get back to you.

Thanks - to both of you - for your helpful comments, appreciated.

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