Hello Martin and welcome aboard,

I think you would get more help if you posted your questions on the devel mailing list. I won't take this review, as it is quite out of my league, but I'll try to give some pointers. I know that the mere number of guidelines and policies in place can be daunting. Since you've run fedora-review yourself, you've already seen most of what needs to be taken care of.

Having read through your entire message, I believe you will find the answers to a lot of your questions in https://fedoraproject.org/wiki/Packaging:Guidelines


(In reply to Martin Brandenburg from comment #0)> 
> This is my first package.  I represent the upstream developers.  I have
> fixed many problems since our last release 2.9.6 while making this
> package.  We intend to fix any further problems uncovered by this review
> and make a new release 2.9.7 if this package is accepted.  As such, my
> RPM currently builds our SVN trunk.

See https://fedoraproject.org/wiki/Packaging:Versioning and https://fedoraproject.org/wiki/Package_Versioning_Examples and mind the prerelease versioning schemes in particular.


> The OrangeFS server and client (which is all I have packaged) are
> intended to be LGPL v2.1 or later.  There are a few files which are
> under various open licenses:
> 
> *No copyright* LGPL (v2)
> ------------------------
> orangefs-2.9.7/src/apps/admin/pvfs2-config.in
> 
> BSD (2 clause)
> --------------
> orangefs-2.9.7/maint/config/ssl.m4
> 
> BSD (3 clause)
> --------------
> orangefs-2.9.7/src/client/usrint/fts.c
> orangefs-2.9.7/src/client/usrint/fts.h
> 
> ISC
> ---
> orangefs-2.9.7/src/common/lmdb/mdb.c
> 
> LGPL (v2.1)
> -----------
> orangefs-2.9.7/src/common/dotconf/dotconf.c
> 
> NTP
> ---
> orangefs-2.9.7/maint/config/install-sh

Isn't this one under MIT?

> 
> zlib/libpng
> -----------
> orangefs-2.9.7/src/common/misc/md5.c
> orangefs-2.9.7/src/common/misc/md5.h

Your License tag may contain as many licenses as needed, see:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios

Here is a somewhat comprehensive list of compatible and incompatible licenses: https://fedoraproject.org/wiki/Licensing:Main

 
> Then there is code under other licenses in our source tree but which are
> not built by this package.  The kernel module is GPL v2.  Parts of our
> Hadoop integration are Apache v2.0.  Parts of the webpack (a set of
> Apache modules) is GPL v3.  None of this is built.

I'm guessing these (sub)packages should have a requirement on Apache and such, but you don't need to worry about their licenses.

> OrangeFS includes a copy of LMDB.  It does not link against any external
> package.  Does this need to be changed in upstream?

Is there a reason that you can't use the version of LMDB already in Fedora (e.g. forked code, older upstream snapshot etc.)? Depending on your needs, you could have something like
BuildRequires:   lmdb-devel
or
Requires:        lmdb
in your spec file.

> 
> OrangeFS requires some configuration to start and creates some files at
> runtime.  A simple configuration file that configures a one-machine file
> system still requires the local hostname.  We generally use a tool to
> generate configuration files.  Then the storage database must be
> initialized.  I have a commented postinstall script.  How do I handle
> this?

Sorry, no idea. Perhaps you could take a look at something like owncloud and see how the maintainer has handled first time setup there?

> 
> The storage database is currently /usr/storage (i.e. $PREFIX/storage).
> Obviously this is not right.  I suppose it should go in /var somewhere.
> Where should it go?  I also don't know how to mark this directory as
> owned by the package.  Should it be deleted on package uninstall since
> it contains user data?

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
In general, I think that whatever might contain user data is left intact when the package gets uninstalled. There is a similar provisions for user-made configurations.

> 
> The storage database contains a number called the FSID (file system
> identifier).  It is generally different for each installation
> (calculated randomly), so distributing one base storage directory would
> be impractical.
> 
> I have written a systemd file for the server.  I'm not really sure how
> to make it not run if the file system has not yet been initialized.

Maybe these could help?
https://fedoraproject.org/wiki/Packaging:Systemd
https://fedoraproject.org/wiki/Packaging:Scriptlets#Systemd

> 
> The utility programs distributed and others which can be linked against
> our libraries will run against the server.  It is also possible to use a
> userspace client program along with the kernel module (distributed by
> kernel.org and already present in Fedora).  This would require running
> the client program and mounting the filesystem through the kernel.  I
> suppose systemd scripts are needed, but I'm not sure what to distribute.
> 
> The server logs to /var/log/orangefs-server.log and the client logs to
> /tmp/pvfs2-client.log.  Obviously /tmp/pvfs2-client.log should be moved
> to /var/log.  The packaging system does not know about either file yet.

https://fedoraproject.org/wiki/Packaging:Guidelines#Log_Files


> [!]: Changelog in prescribed format.

I think rpmlint complains about your changelog because there is no (epoch-)version-release in the entry. See
 https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs but keep in mind what I wrote before about prerelease/snapshot versioning.
Btw, I think that the changelog should go at the end of the spec file.

> [?]: Package contains systemd file(s) if in need.
> 
> I'd like someone to review my systemd file.

If you don't get definitive answers in the wiki, ask on the mailing lists.

> [!]: Description and summary sections in the package spec file contains
>      translations for supported Non-English languages, if available.
> 
> I can write some German badly, but I can't write anything else.

You don't have to, that is in case there are translations included with the code upstream.

> [!]: %check is present and all tests pass.

A %check section contains test suites or units. If you have these in your source code, you should enable them there (if it makes sense).

> I intend to deal with the manpage issues, but I'm at the point now where
> I'm looking for advice on most of it.

https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages

> 
> The macro-in-comment lines will go away when those lines are deleted
> from the spec.
> 
> Rpmlint
> -------
> Checking: orangefs-2.9.7-1.fc26.x86_64.rpm
>           orangefs-debuginfo-2.9.7-1.fc26.x86_64.rpm
>           orangefs-devel-2.9.7-1.fc26.x86_64.rpm
>           orangefs-server-2.9.7-1.fc26.x86_64.rpm
>           orangefs-2.9.7-1.fc26.src.rpm
> orangefs.x86_64: W: no-version-in-last-changelog
> orangefs.x86_64: W: unstripped-binary-or-object /usr/lib64/libpvfs2.so.2.9.6
> orangefs.x86_64: E: no-ldconfig-symlink /usr/lib64/libpvfs2.so.2.9.6
> orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpvfs2.so.2.9.6
> exit.5
> orangefs.x86_64: W: manual-page-warning
> /usr/share/man/man1/pvfs2-drop-caches.1.gz 13: warning: numeric expression
> expected (got `f')
> orangefs.x86_64: W: manual-page-warning
> /usr/share/man/man1/pvfs2-fs-dump.1.gz 15: warning: numeric expression
> expected (got `m')
> orangefs-debuginfo.x86_64: W: no-version-in-last-changelog
> orangefs-devel.x86_64: W: no-dependency-on orangefs/orangefs-libs/liborangefs
> orangefs-devel.x86_64: W: no-version-in-last-changelog
> orangefs-devel.x86_64: W: no-documentation
> orangefs-devel.x86_64: W: no-manual-page-for-binary pvfs2-config
> orangefs-server.x86_64: W: no-version-in-last-changelog
> orangefs-server.x86_64: W: no-manual-page-for-binary pvfs2-start-all
> orangefs-server.x86_64: W: no-manual-page-for-binary pvfs2-stop-all
> orangefs.src: W: no-version-in-last-changelog
> orangefs.src:12: W: unversioned-explicit-provides libofs.so()(64bit)
> orangefs.src:12: W: unversioned-explicit-provides liborangefs.so()(64bit)
> orangefs.src:12: W: unversioned-explicit-provides libpvfs2.so()(64bit)
> orangefs.src:19: W: macro-in-comment %{version}
> orangefs.src:41: W: macro-in-comment %{_bindir}
> orangefs.src:42: W: macro-in-comment %{_bindir}
> orangefs.src:43: W: macro-in-comment %{_bindir}
> orangefs.src:44: W: macro-in-comment %{_bindir}
> orangefs.src:45: W: macro-in-comment %{_bindir}
> orangefs.src:64: W: macro-in-comment %{_bindir}
> orangefs.src:80: W: macro-in-comment %{_bindir}
> orangefs.src:83: W: macro-in-comment %{_libdir}
> orangefs.src:84: W: macro-in-comment %{_libdir}
> orangefs.src:85: W: macro-in-comment %{_libdir}
> orangefs.src:86: W: macro-in-comment %{_libdir}
> orangefs.src:87: W: macro-in-comment %{_libdir}
> orangefs.src:88: W: macro-in-comment %{_libdir}
> orangefs.src:164: W: macro-in-comment %{_libdir}
> orangefs.src:165: W: macro-in-comment %{_libdir}
> orangefs.src:166: W: macro-in-comment %{_libdir}
> orangefs.src:167: W: macro-in-comment %{_libdir}
> orangefs.src:168: W: macro-in-comment %{_libdir}
> orangefs.src:169: W: macro-in-comment %{_libdir}
> orangefs.src:197: W: macro-in-comment %config
> orangefs.src: W: invalid-url Source0: orangefs-2.9.7.tar.gz
> 5 packages and 0 specfiles checked; 1 errors, 39 warnings.

You can find some useful info on rpmlint warnings and errors here:
https://fedoraproject.org/wiki/Common_Rpmlint_issues

I hope I have helped a little bit. Happy reading and I'm sure you'll find more qualified people to answer your questions on devel or IRC.

Just wanting to clarify, do you already have a sponsor?  If so, I'll happily review this, but I'm not a sponsor.

If you don't have a sponsor, read here: https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

I can sponsor if someone else can review this...

Thanks Alexander for your help.

I do not have a sponsor yet.

I'll leave the version as is until I either get more specific advice.

Upstream copied the LMDB sources because it was easier to do that than
deal with the many different distributions which may not package LMDB.

Really that's not ideal though.  I will add the ability to link against
an external LMDB upstream, so we can use that.

Most of the macro-in-comment warnings stem from aborted support for
part of OrangeFS called the usrint, which is a libc interposer which can
be used to give applications support for accessing an OrangeFS volume
without kernel support.

I have disabled it because it does not build on some exotic
architectures due to missing syscalls.  I'm not sure we have the ability
to fix this without the hardware.

Is it appropriate to enable it on some architectures and not the others?
Several of our utility programs now rely on it.

I'm willing to sponsor if Jonathan completes the review process.

I just noticed this, and should try to find time to comment in detail
as I put some effort into packaging it.  What I advertised is under
https://copr.fedorainfracloud.org/coprs/loveshack/orangefs/ but I have
an unfinished update against 2.9.5 to tackle at least library sonames
-- I forget what else off-hand, but will take a look.

There are various other problems for packaging that may or not be
helped by more recent code which I haven't followed.  The main one is,
I think, the need for combinatorial builds against tcp and openib
networking and the authentication methods.  I think it should support
EPEL to cater for the bulk of HPC systems (I added dkms support), and
shouldn't assume it's run as a system service since a big feature is
being able to spin up ephemeral filesystems in userspace.

HTH. Prod me if I don't get on it fairly soon.

Sorry, just hit the wrong link there.  I am still interested in reviewing this, but I've just returned home from a summer away, so I'm still getting back into the swing of things.  Give me a week or so to get this together.

(In reply to Martin Brandenburg from comment #0)
> This is my first package.  I represent the upstream developers.  I have
> fixed many problems since our last release 2.9.6 while making this
> package.  We intend to fix any further problems uncovered by this review
> and make a new release 2.9.7 if this package is accepted.  As such, my
> RPM currently builds our SVN trunk.

As Alexander pointed out, at least until 2.9.7 has been built you really need to use proper pre-release versioning (https://fedoraproject.org/wiki/Package_Versioning_Examples#Complex_versioning_examples, scroll down to  pkg pre-release svn checkout)

This allows us to verify that the code submitted matches the upstream code.
 
> I have several questions which have arison from my reading of the wiki
> and fedora-review's output.
> 
> The OrangeFS server and client (which is all I have packaged) are
> intended to be LGPL v2.1 or later.  There are a few files which are
> under various open licenses:
> 
> *No copyright* LGPL (v2)
> ------------------------
> orangefs-2.9.7/src/apps/admin/pvfs2-config.in
> 
> BSD (2 clause)
> --------------
> orangefs-2.9.7/maint/config/ssl.m4
> 
> BSD (3 clause)
> --------------
> orangefs-2.9.7/src/client/usrint/fts.c
> orangefs-2.9.7/src/client/usrint/fts.h
> 
> ISC
> ---
> orangefs-2.9.7/src/common/lmdb/mdb.c
> 
> LGPL (v2.1)
> -----------
> orangefs-2.9.7/src/common/dotconf/dotconf.c
> 
> NTP
> ---
> orangefs-2.9.7/maint/config/install-sh
> 
> zlib/libpng
> -----------
> orangefs-2.9.7/src/common/misc/md5.c
> orangefs-2.9.7/src/common/misc/md5.h
> 
> Then there is code under other licenses in our source tree but which are
> not built by this package.  The kernel module is GPL v2.  Parts of our
> Hadoop integration are Apache v2.0.  Parts of the webpack (a set of
> Apache modules) is GPL v3.  None of this is built.

If it's not built, you don't need to list the licenses.  But I would wonder why you're not building these modules (obviously excepting the kernel module), and splitting them into subpackages so your users can easily get the parts of OrangeFS that they want.

I noticed the following options not being set by configure:
PVFS2 configured to build karma gui               :  no
PVFS2 configured to build visualization tools     :  no
PVFS2 configured to perform coverage analysis     :  no
PVFS2 configured to use FUSE                      :  no
PVFS2 configured for the 2.6/3 kernel module      :  no
PVFS2 configured for the 2.4.x kernel module      :  no
PVFS2 configured for using the ra-cache           :  no
PVFS2 configured for resetting file position      :  no
PVFS2 will use workaround for redhat 2.4 kernels  :  no
PVFS2 will use workaround for buggy NPTL          :  no
PVFS2 server capability cache enabled             :  no
PVFS2 server credential cache enabled             :  no
PVFS2 server certificate cache enabled            :  no
PVFS2 configured with key-based security          :  no
PVFS2 configured with certificate-based security  :  no
PVFS2 configured with profiling enabled           :  no
PVFS2 user interface libraries will be built      :  no
PVFS2 symbolic libraries will be built            :  no
PVFS2 user environment variables enabled          :  no
PVFS2 user interface library cache enabled        :  no
PVFS2 client JNI enabled                          :  no
 
Obviously some of those are irrelevant to Fedora, but what about the karma gui, visualization tools, ra-cache, server caches, key-based security, certificate-based security, etc.  Are these all obsolete or irrelevant to Fedora, or would some of those be useful to your users?

> I am not sure how to handle this.  I assume that the BSD/MIT style
> licenses will not pose a problem, but I don't know where to document it.

Normally like this: GPLv3 and BSD and ...

> OrangeFS includes a copy of LMDB.  It does not link against any external
> package.  Does this need to be changed in upstream?

According to the Guidelines, if upstream *can't* be built against the system version of LMDB, then you can bundle it, while following the guidelines on how to do that (https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries), but, given that you are upstream, it would be *much* better if you could make the changes required to build against the system library.  You really don't want to be in a position where your bundled library has a vulnerability that's been patched in the system library, but not in your bundled library.  I've been there.  It sucks.

> OrangeFS requires some configuration to start and creates some files at
> runtime.  A simple configuration file that configures a one-machine file
> system still requires the local hostname.  We generally use a tool to
> generate configuration files.  Then the storage database must be
> initialized.  I have a commented postinstall script.  How do I handle
> this?

My recommendation would be to put pvfs2-genconfig in /usr/bin, as you already have, but possibly throw a simple config in /etc/orangefs/orangefs.conf.  If the config could contain minimal (even if it doesn't work out of the box) configuration and a comment explaining the best way to properly generate it using pvfs2-genconfig, that would make it easy enough for even a non-documentation-reading sysadmin like myself to figure out what to do.

Whatever you do, it looks like there are enough configuration files (certificates, etc as well as orangefs.conf) that it would be worth creating and owning /etc/orangefs/ and using that as your default configuration location.

> The storage database is currently /usr/storage (i.e. $PREFIX/storage).
> Obviously this is not right.  I suppose it should go in /var somewhere.
> Where should it go?  I also don't know how to mark this directory as
> owned by the package.  Should it be deleted on package uninstall since
> it contains user data?

/var/lib/orangefs or /var/lib/orangefs/storage if there are other state files kept in /var/lib/orangefs.  I'm assuming the admin can change this location in the config file.

> The storage database contains a number called the FSID (file system
> identifier).  It is generally different for each installation
> (calculated randomly), so distributing one base storage directory would
> be impractical.

I'm assuming storage database generation is manual.  If so, I would use /var/lib/orangefs as the default location (as generated by pvfs2-genconfig), but when the sysadmin runs pvfs2-genconfig, they will obviously be given the chance to change this.

> I have written a systemd file for the server.  I'm not really sure how
> to make it not run if the file system has not yet been initialized.

If the server exits with a sane enough message ("Your file system has not yet been initialized.  Please read the configuration file/documentation for more details"), then I wouldn't worry about it.

> The utility programs distributed and others which can be linked against
> our libraries will run against the server.  It is also possible to use a
> userspace client program along with the kernel module (distributed by
> kernel.org and already present in Fedora).  This would require running
> the client program and mounting the filesystem through the kernel.  I
> suppose systemd scripts are needed, but I'm not sure what to distribute.

If the client needs to be run with arguments to mount the filesystem, I'd probably not bother with a systemd service, but if it reads the configuration from /etc, a service might make sense.

> The server logs to /var/log/orangefs-server.log and the client logs to
> /tmp/pvfs2-client.log.  Obviously /tmp/pvfs2-client.log should be moved
> to /var/log.  The packaging system does not know about either file yet.

Can both log to the journal?  Or to syslog (which will get forwarded to the journal)?  I believe that would be the most sane option.

> We have a component called the usrint which is a libc interposer.
> Several of our utility programs require it now.  It does not build on
> all platforms.  It is not required to run either the server or the
> client.  I have disabled it completely.

If some of your utilities require it and it's important, I would probably check that it builds on all primary architectures.  If it does, I'd enable it, and then use conditionals to disable building it on the arches it crashes on.  Then open a bug for each arch it crashes on, (similar to what's described at https://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Build_Failures).  It's possible someone who knows those arches may fix the bugs.  If it doesn't build on all primary architectures (which I believe are currently ARM and x86_64), then I'd try to get it working at least for those, but it's not required by the guidelines.

The one other thing I noticed is that you're packaging .a files in your -devel package.  I'd remove them as part of your build process.

There are other issues, but I'd like to see the above dealt with before we start moving through the detailed review.

Dave, yes we're interested in EPEL as well.  The problem is the
third-party kernel module.  Fortunately we've solved this problem for
newer kernels.  I'm sorry to say that I haven't looked through your
packages much, but I hope that we won't duplicate too much
work going forward.

There are a lot of options, and many of them are mutually exclusive and
compile-time only which makes it hard to decide which are appropriate
defaults.

Jonathan, thanks for reviewing.

(In reply to Jonathan Dieter from comment #8)
> (In reply to Martin Brandenburg from comment #0)
> > This is my first package.  I represent the upstream developers.  I have
> > fixed many problems since our last release 2.9.6 while making this
> > package.  We intend to fix any further problems uncovered by this review
> > and make a new release 2.9.7 if this package is accepted.  As such, my
> > RPM currently builds our SVN trunk.
> 
> As Alexander pointed out, at least until 2.9.7 has been built you really
> need to use proper pre-release versioning
> (https://fedoraproject.org/wiki/
> Package_Versioning_Examples#Complex_versioning_examples, scroll down to  pkg
> pre-release svn checkout)
> 
> This allows us to verify that the code submitted matches the upstream code.

That's easy enough.

> If it's not built, you don't need to list the licenses.  But I would wonder
> why you're not building these modules (obviously excepting the kernel
> module), and splitting them into subpackages so your users can easily get
> the parts of OrangeFS that they want.
> 
> I noticed the following options not being set by configure:
> PVFS2 configured to build karma gui               :  no
> PVFS2 configured to build visualization tools     :  no
> PVFS2 configured to perform coverage analysis     :  no
> PVFS2 configured to use FUSE                      :  no
> PVFS2 configured for the 2.6/3 kernel module      :  no
> PVFS2 configured for the 2.4.x kernel module      :  no
> PVFS2 configured for using the ra-cache           :  no
> PVFS2 configured for resetting file position      :  no
> PVFS2 will use workaround for redhat 2.4 kernels  :  no
> PVFS2 will use workaround for buggy NPTL          :  no
> PVFS2 server capability cache enabled             :  no
> PVFS2 server credential cache enabled             :  no
> PVFS2 server certificate cache enabled            :  no
> PVFS2 configured with key-based security          :  no
> PVFS2 configured with certificate-based security  :  no
> PVFS2 configured with profiling enabled           :  no
> PVFS2 user interface libraries will be built      :  no
> PVFS2 symbolic libraries will be built            :  no
> PVFS2 user environment variables enabled          :  no
> PVFS2 user interface library cache enabled        :  no
> PVFS2 client JNI enabled                          :  no
>  
> Obviously some of those are irrelevant to Fedora, but what about the karma
> gui, visualization tools, ra-cache, server caches, key-based security,
> certificate-based security, etc.  Are these all obsolete or irrelevant to
> Fedora, or would some of those be useful to your users?

Some of these have not been dealt with in a long time, but you're
certainly right that some of it may be useful.

It turns out that the "visualization tools" don't build.

Obviously the kernel modules are unnecessary.  You can see the age of
this stuff.  We actually don't ship the 2.4 code.  I wonder why these
messages survived.

I wonder what these workarounds are.  They're before my time.

The ra-cache is new and not very well tested.  I don't know what
"resetting file position" is, but I have an idea.  It causes read/write
to falsely avoid returning partial reads/writes.  It was a hack for
someone who would not fix their application, and I hope nobody else
wants it.  It should just be deleted.

The caches affect the security modes.  OrangeFS supports three security
modes, which are dealt with at compile time.  It would be nice to build
all three as mutually exclusive packages.  (It would be nicer to make it
a runtime option, but that's difficult.)

The user interface library is the libc interposer I mentioned before.

I don't know what a symbolic library is.

We put a lot of effort into JNI, and it would be nice to build it, but I
personally don't know much about it.

It breaks down in to obsolete things which should just be deleted, and
useful but non-core features.  I wasn't going to tackle non-core stuff
yet, but it's definitely on the to-do list before I'd want the package
released in Fedora.

Honestly for some of this, just getting it regularly built (and tested!)
will be an improvement to the status quo.  It at least forces us to make
the decision whether to remove old crufty bits or keep them working.

So I've enabled Karma, FUSE, and (stopped disabling) the usrint.  That
leaves JNI and the webpack as far as I can see as optional things that
can be enabled.

Then there's stuff like the caches where we have to consider stability
with and without.  I will discuss this with other OrangeFS developers.

I've enabled Infiniband as well.

> > I am not sure how to handle this.  I assume that the BSD/MIT style
> > licenses will not pose a problem, but I don't know where to document it.
> 
> Normally like this: GPLv3 and BSD and ...
> 
> > OrangeFS includes a copy of LMDB.  It does not link against any external
> > package.  Does this need to be changed in upstream?
> 
> According to the Guidelines, if upstream *can't* be built against the system
> version of LMDB, then you can bundle it, while following the guidelines on
> how to do that
> (https://fedoraproject.org/wiki/Packaging:
> Guidelines#Bundling_and_Duplication_of_system_libraries), but, given that
> you are upstream, it would be *much* better if you could make the changes
> required to build against the system library.  You really don't want to be
> in a position where your bundled library has a vulnerability that's been
> patched in the system library, but not in your bundled library.  I've been
>?deve there.  It sucks.

I've fixed upstream to allow linking against the system LMDB.

> > OrangeFS requires some configuration to start and creates some files at
> > runtime.  A simple configuration file that configures a one-machine file
> > system still requires the local hostname.  We generally use a tool to
> > generate configuration files.  Then the storage database must be
> > initialized.  I have a commented postinstall script.  How do I handle
> > this?
> 
> My recommendation would be to put pvfs2-genconfig in /usr/bin, as you
> already have, but possibly throw a simple config in
> /etc/orangefs/orangefs.conf.  If the config could contain minimal (even if
> it doesn't work out of the box) configuration and a comment explaining the
> best way to properly generate it using pvfs2-genconfig, that would make it
> easy enough for even a non-documentation-reading sysadmin like myself to
> figure out what to do.
> 
> Whatever you do, it looks like there are enough configuration files
> (certificates, etc as well as orangefs.conf) that it would be worth creating
> and owning /etc/orangefs/ and using that as your default configuration
> location.

I've done this now.

Most of the client applications look for /etc/pvfs2tab.  Should I patch
it to look for /etc/orangefs/pvfs2tab?  The server configuration is
given on the command line, but this is hardcoded (or settable by
environment variable).

> > The storage database is currently /usr/storage (i.e. $PREFIX/storage).
> > Obviously this is not right.  I suppose it should go in /var somewhere.
> > Where should it go?  I also don't know how to mark this directory as
> > owned by the package.  Should it be deleted on package uninstall since
> > it contains user data?
> 
> /var/lib/orangefs or /var/lib/orangefs/storage if there are other state
> files kept in /var/lib/orangefs.  I'm assuming the admin can change this
> location in the config file.
> 
> > The storage database contains a number called the FSID (file system
> > identifier).  It is generally different for each installation
> > (calculated randomly), so distributing one base storage directory would
> > be impractical.
> 
> I'm assuming storage database generation is manual.  If so, I would use
> /var/lib/orangefs as the default location (as generated by pvfs2-genconfig),
> but when the sysadmin runs pvfs2-genconfig, they will obviously be given the
> chance to change this.

All this makes sense.  We have encouraged people to chose paths ad hoc
in the past, and I would really like to move away from this.

I have patched genconfig to default to Fedora-appropriate paths and
options.  I've also produced the "example" configuration file mentioned
above and ensured that genconfig produces the same output (modulo the
random ID).  The administrator can set whatever path is preferred.

> > I have written a systemd file for the server.  I'm not really sure how
> > to make it not run if the file system has not yet been initialized.
> 
> If the server exits with a sane enough message ("Your file system has not
> yet been initialized.  Please read the configuration file/documentation for
> more details"), then I wouldn't worry about it.

It does.

> > The utility programs distributed and others which can be linked against
> > our libraries will run against the server.  It is also possible to use a
> > userspace client program along with the kernel module (distributed by
> > kernel.org and already present in Fedora).  This would require running
> > the client program and mounting the filesystem through the kernel.  I
> > suppose systemd scripts are needed, but I'm not sure what to distribute.
> 
> If the client needs to be run with arguments to mount the filesystem, I'd
> probably not bother with a systemd service, but if it reads the
> configuration from /etc, a service might make sense.

It doesn't need any arguments.  The mount request comes in from the
kernel with all the information it needs.  I've added a systemd unit for
it.

I'm not sure how to arrange things so that systemd doesn't try to mount
the filesystem before the daemon is started.

> > The server logs to /var/log/orangefs-server.log and the client logs to
> > /tmp/pvfs2-client.log.  Obviously /tmp/pvfs2-client.log should be moved
> > to /var/log.  The packaging system does not know about either file yet.
> 
> Can both log to the journal?  Or to syslog (which will get forwarded to the
> journal)?  I believe that would be the most sane option.

I've patched genconfig to product configuration which logs to syslog by
default.  It'll probably be better to clean up and upstream this.

> > We have a component called the usrint which is a libc interposer.
> > Several of our utility programs require it now.  It does not build on
> > all platforms.  It is not required to run either the server or the
> > client.  I have disabled it completely.
> 
> If some of your utilities require it and it's important, I would probably
> check that it builds on all primary architectures.  If it does, I'd enable
> it, and then use conditionals to disable building it on the arches it
> crashes on.  Then open a bug for each arch it crashes on, (similar to what's
> described at
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Architecture_Build_Failures).  It's possible someone who knows
> those arches may fix the bugs.  If it doesn't build on all primary
> architectures (which I believe are currently ARM and x86_64), then I'd try
> to get it working at least for those, but it's not required by the
> guidelines.
> 
> The one other thing I noticed is that you're packaging .a files in your
> -devel package.  I'd remove them as part of your build process.

I figure somebody may want to link statically, but I have no preference
either way, and this is easy to change.  I've left it in for now, but
will remove if there's a Fedora policy or anyone has a strong opinion.

> There are other issues, but I'd like to see the above dealt with before we
> start moving through the detailed review.

I will update next week with an updated spec and SRPM fixing at least
some of this.

I have some arch-specific stuff now.  On armv7hl, we disable Infiniband
since there is no package libibverbs-devel.  I've disabled aarch64.  The
other option is to disable the usrint only on aarch64, but I don't like
cutting out a big part of the package only on one architecture.

Here is a new build:

Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=21890828
Spec: http://dev.orangefs.org/2017/marbran/0915/orangefs.spec
SRPM: http://dev.orangefs.org/2017/marbran/0915/orangefs-2.9.6-0.1.20170904svn.fc26.src.rpm

Thanks for addressing the issues I've listed.  Responses inline and at the bottom:

> (In reply to Jonathan Dieter from comment #8)
> > (In reply to Martin Brandenburg from comment #0)
> > If it's not built, you don't need to list the licenses.  But I would wonder
> > why you're not building these modules (obviously excepting the kernel
> > module), and splitting them into subpackages so your users can easily get
> > the parts of OrangeFS that they want.
> > 
> Honestly for some of this, just getting it regularly built (and tested!)
> will be an improvement to the status quo.  It at least forces us to make
> the decision whether to remove old crufty bits or keep them working.
> 
> So I've enabled Karma, FUSE, and (stopped disabling) the usrint.  That
> leaves JNI and the webpack as far as I can see as optional things that
> can be enabled.

Looks good to me.

> Then there's stuff like the caches where we have to consider stability
> with and without.  I will discuss this with other OrangeFS developers.
> 
> I've enabled Infiniband as well.
> 
> > > I am not sure how to handle this.  I assume that the BSD/MIT style
> > > licenses will not pose a problem, but I don't know where to document it.
> > 
> > Normally like this: GPLv3 and BSD and ...

Ok, a quick note on the licensing.  The NTP license is actually a variant of MIT, and, at least according to https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/JM7YJILE4GIFRD3J636EAT2PBOEND7WP/, should be listed as such.

And, according https://fedoraproject.org/wiki/Licensing:Main#Good_Licenses, you can list LGPLv2.1 as LGPLv2.

> > My recommendation would be to put pvfs2-genconfig in /usr/bin, as you
> > already have, but possibly throw a simple config in
> > /etc/orangefs/orangefs.conf.  If the config could contain minimal (even if
> > it doesn't work out of the box) configuration and a comment explaining the
> > best way to properly generate it using pvfs2-genconfig, that would make it
> > easy enough for even a non-documentation-reading sysadmin like myself to
> > figure out what to do.
> > 
> > Whatever you do, it looks like there are enough configuration files
> > (certificates, etc as well as orangefs.conf) that it would be worth creating
> > and owning /etc/orangefs/ and using that as your default configuration
> > location.
> 
> I've done this now.
> 
> Most of the client applications look for /etc/pvfs2tab.  Should I patch
> it to look for /etc/orangefs/pvfs2tab?  The server configuration is
> given on the command line, but this is hardcoded (or settable by
> environment variable).

Thanks for this.  If /etc/pvfs2tab is a well-known location, I sure wouldn't change it unless upstream is planning to make the change universal.

> > > The utility programs distributed and others which can be linked against
> > > our libraries will run against the server.  It is also possible to use a
> > > userspace client program along with the kernel module (distributed by
> > > kernel.org and already present in Fedora).  This would require running
> > > the client program and mounting the filesystem through the kernel.  I
> > > suppose systemd scripts are needed, but I'm not sure what to distribute.
> > 
> > If the client needs to be run with arguments to mount the filesystem, I'd
> > probably not bother with a systemd service, but if it reads the
> > configuration from /etc, a service might make sense.
> 
> It doesn't need any arguments.  The mount request comes in from the
> kernel with all the information it needs.  I've added a systemd unit for
> it.
> 
> I'm not sure how to arrange things so that systemd doesn't try to mount
> the filesystem before the daemon is started.

You could try putting After= in the mount unit file, which would order it after the server if and only if the server is also installed on the same machine.  If there is no server on the local machine, then the client will start in parallel with everything else.

> > > We have a component called the usrint which is a libc interposer.
> > > Several of our utility programs require it now.  It does not build on
> > > all platforms.  It is not required to run either the server or the
> > > client.  I have disabled it completely.
> > 
> > If some of your utilities require it and it's important, I would probably
> > check that it builds on all primary architectures.  If it does, I'd enable
> > it, and then use conditionals to disable building it on the arches it
> > crashes on.  Then open a bug for each arch it crashes on, (similar to what's
> > described at
> > https://fedoraproject.org/wiki/Packaging:
> > Guidelines#Architecture_Build_Failures).  It's possible someone who knows
> > those arches may fix the bugs.  If it doesn't build on all primary
> > architectures (which I believe are currently ARM and x86_64), then I'd try
> > to get it working at least for those, but it's not required by the
> > guidelines.
> > 
> > The one other thing I noticed is that you're packaging .a files in your
> > -devel package.  I'd remove them as part of your build process.
> 
> I figure somebody may want to link statically, but I have no preference
> either way, and this is easy to change.  I've left it in for now, but
> will remove if there's a Fedora policy or anyone has a strong opinion.

Fedora has very specific guidelines if you're packaging static libraries (see: https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries, specifically the part about splitting static libraries into a separate subpackage),  so I'd recommend just dropping the static libraries, unless you have a deep and overwhelming desire to deal with the extra red tape. ;)

One major thing you need to do is remove the manual Provides in the orangefs client package.  The reason the libraries aren't automatically being Provided: is that they're not executable, so you need to make sure they are installed with 0755 permissions.  Once you do that, rpm will automatically add the Provides: for you.

Hopefully this will sort out some of the rpmlint errors I'm seeing.  I am a bit concerned by the shared-lib-calls-exit warning I'm getting.  Do you intend for your library to actually exit the calling program as opposed to returning an error?

Somewhat late, these are comments partly as a user and partly as a
packager (which may duplicate the reviewer's somewhat).  I've put an
updated version of what I ran on an HPC system previously at
<http://loveshack.fedorapeople.org/reviews/orangefs>.  It's probably
not packaging standard, but it has things like support for dkms and
multiple transports -- I'd expect openib support for HPC -- and
addresses some of these comments.

HTH.

* Use correct svn Version and Source forms

* Use ./prepare in %prep on straight exported source

* Use unbundled lmdb (see patch)

* Use interface-related soname (see patch); the interface was
  sufficiently stable over some previous versions I checked with
  abipkgdiff to keep a major version.

* It needs an IB version (unless you can configure for both IB and TCP
  now)

* Probably should have combinatorial builds over network and security
  types too

* Remove the Provides: (but combinatorial builds should maybe provide
  "orangefs")

* Make -libs-<transport> packages for use with mpi-io etc.

* Fix strange aarch64-specific build failure, but conditionalize it
  for now -- ask about it on devel list?

  src/client/usrint/pvfs-path.c: In function 'PVFS_expand_path':
src/client/usrint/pvfs-path.c:266:25: error: 'SYS_readlink' undeclared (first use in this function); did you mean 'SYS_readlinkat'?
             n = syscall(SYS_readlink,
                         ^~~~~~~~~~~~
                         SYS_readlinkat

* Configure --disable-static

* -devel should have Requires, but not the Provides

* Install service file correctly, but provide init file for el6

* Don't try to configure the server/client; just provide -example files

* Add %licence with correct contents

* Expand description?

* Cosmetic: Order of stanzas is confusing (e.g. %changelog is
  conventionally at end)

* Build Java, and possibly web, stuff.
  Any reason not to --enable-fuse ?

* Include pacemaker stuff

* pvfs2-{start,stop}-all should be %config as they need editing?

* Ship examples and built doc

* Don't mark %mandir as %doc

* Fix the build upstream to be smp-safe: currently tends to fail in
  statecomp/scanner directory

(In reply to Jonathan Dieter from comment #10)
> > (In reply to Jonathan Dieter from comment #8)
> > > (In reply to Martin Brandenburg from comment #0)
> > > > I am not sure how to handle this.  I assume that the BSD/MIT style
> > > > licenses will not pose a problem, but I don't know where to document it.
> > > 
> > > Normally like this: GPLv3 and BSD and ...
> 
> Ok, a quick note on the licensing.  The NTP license is actually a variant of
> MIT, and, at least according to
> https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/
> thread/JM7YJILE4GIFRD3J636EAT2PBOEND7WP/, should be listed as such.
> 
> And, according https://fedoraproject.org/wiki/Licensing:Main#Good_Licenses,
> you can list LGPLv2.1 as LGPLv2.

And LGPLv2 and LGPLv2+?  They're still listed separately.

> > Most of the client applications look for /etc/pvfs2tab.  Should I patch
> > it to look for /etc/orangefs/pvfs2tab?  The server configuration is
> > given on the command line, but this is hardcoded (or settable by
> > environment variable).
> 
> Thanks for this.  If /etc/pvfs2tab is a well-known location, I sure wouldn't
> change it unless upstream is planning to make the change universal.

Then that will remain as it is now.

> > > > The utility programs distributed and others which can be linked against
> > > > our libraries will run against the server.  It is also possible to use a
> > > > userspace client program along with the kernel module (distributed by
> > > > kernel.org and already present in Fedora).  This would require running
> > > > the client program and mounting the filesystem through the kernel.  I
> > > > suppose systemd scripts are needed, but I'm not sure what to distribute.
> > > 
> > > If the client needs to be run with arguments to mount the filesystem, I'd
> > > probably not bother with a systemd service, but if it reads the
> > > configuration from /etc, a service might make sense.
> > 
> > It doesn't need any arguments.  The mount request comes in from the
> > kernel with all the information it needs.  I've added a systemd unit for
> > it.
> > 
> > I'm not sure how to arrange things so that systemd doesn't try to mount
> > the filesystem before the daemon is started.
> 
> You could try putting After= in the mount unit file, which would order it
> after the server if and only if the server is also installed on the same
> machine.  If there is no server on the local machine, then the client will
> start in parallel with everything else.

No you've misunderstood me, though you bring up another good point.  If
a pvfs2 filesystem appears in fstab, it cannot be mounted before the
pvfs2-client (i.e. orangefs-client.service) is running.  The mount will
hang until either a timeout expires or the client starts running.  Given
systemd's parallel nature this might not actually cause a problem, but
certainly isn't very clean.

And you bring up something I hadn't thought about: if the server and
client are to run on the same machine, it will be necessary to start the
server first.

> > > The one other thing I noticed is that you're packaging .a files in your
> > > -devel package.  I'd remove them as part of your build process.
> > 
> > I figure somebody may want to link statically, but I have no preference
> > either way, and this is easy to change.  I've left it in for now, but
> > will remove if there's a Fedora policy or anyone has a strong opinion.
> 
> Fedora has very specific guidelines if you're packaging static libraries
> (see:
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Packaging_Static_Libraries, specifically the part about splitting
> static libraries into a separate subpackage),  so I'd recommend just
> dropping the static libraries, unless you have a deep and overwhelming
> desire to deal with the extra red tape. ;)

Then I will take the static libraries out.

> One major thing you need to do is remove the manual Provides in the orangefs
> client package.  The reason the libraries aren't automatically being
> Provided: is that they're not executable, so you need to make sure they are
> installed with 0755 permissions.  Once you do that, rpm will automatically
> add the Provides: for you.

Okay fixed with Dave Love's orangefs-soname.patch.

> Hopefully this will sort out some of the rpmlint errors I'm seeing.  I am a
> bit concerned by the shared-lib-calls-exit warning I'm getting.  Do you
> intend for your library to actually exit the calling program as opposed to
> returning an error?

Unfortunately, probably yes.  I've done some quick grepping, and many of
them look like places where an assert wouldn't be unreasonable.

(In reply to Dave Love from comment #11)
> Somewhat late, these are comments partly as a user and partly as a
> packager (which may duplicate the reviewer's somewhat).  I've put an
> updated version of what I ran on an HPC system previously at
> <http://loveshack.fedorapeople.org/reviews/orangefs>.  It's probably
> not packaging standard, but it has things like support for dkms and
> multiple transports -- I'd expect openib support for HPC -- and
> addresses some of these comments.
> 
> HTH.
> 
> * Use correct svn Version and Source forms
> 
> * Use ./prepare in %prep on straight exported source

Should we?  The release tarballs will not require it.

> * Use unbundled lmdb (see patch)

I fixed this a while ago, though we are still shipping a copy of LMDB
for those who want it.

> * Use interface-related soname (see patch); the interface was
>   sufficiently stable over some previous versions I checked with
>   abipkgdiff to keep a major version.

Thanks for this.  I assume you are good with me merging your
orangefs-soname.patch upstream?

> * It needs an IB version (unless you can configure for both IB and TCP
>   now)

It complains loudly that performance will suffer, but it's wrong or at
least misleading.  Performance will only suffer if you actually attempt
to use both at the same time.  Just having the code built and available
for run-time configuration doesn't harm anything.

> * Probably should have combinatorial builds over network and security
>   types too

Do you use any of the security modes?  We built all that stuff, but
haven't really heard from anyone who uses it.  I agree it would be nice
to have, and I don't want to rule it out this early.

There's several network modes in src/io/bmi, but all we've focused on
recently is bmi_tcp and bmi_ib.  I'm not sure if the others build.

I don't want to make something that is missing features people want to
use.

> * Remove the Provides: (but combinatorial builds should maybe provide
>   "orangefs")
> 
> * Make -libs-<transport> packages for use with mpi-io etc.

I have to admit I'm not entirely sure what you're talking about with
either of these.

> * Fix strange aarch64-specific build failure, but conditionalize it
>   for now -- ask about it on devel list?
> 
>   src/client/usrint/pvfs-path.c: In function 'PVFS_expand_path':
> src/client/usrint/pvfs-path.c:266:25: error: 'SYS_readlink' undeclared
> (first use in this function); did you mean 'SYS_readlinkat'?
>              n = syscall(SYS_readlink,
>                          ^~~~~~~~~~~~
>                          SYS_readlinkat

They dropped obsolete syscalls in the kernel for the new architecture.
That code is somewhat fragile (can't you tell from the fact it calls
syscall instead of readlink?), so I'm not going to attempt to fix this,
but I will forward the bug upstream.

> * Configure --disable-static

Done.

> * -devel should have Requires, but not the Provides
> 
> * Install service file correctly, but provide init file for el6
> 
> * Don't try to configure the server/client; just provide -example files

Honestly that probably makes more sense than what I'm doing.  I'm going
to go ahead and do it.

> * Add %licence with correct contents
> 
> * Expand description?

Not a bad idea.  May I take (and probably tweak) your description?

> * Cosmetic: Order of stanzas is confusing (e.g. %changelog is
>   conventionally at end)

I think it is now?  Please suggest anything else I should reorder.

> * Build Java, and possibly web, stuff.
>   Any reason not to --enable-fuse ?

There is much debate here about this.

> * Include pacemaker stuff

Again I have to admit I don't know what this is.

> * pvfs2-{start,stop}-all should be %config as they need editing?

They're also in /usr/sbin.  Putting them somewhere as an example or
extending them to take command line arguments may make more sense.

> * Ship examples and built doc
> 
> * Don't mark %mandir as %doc

Done.

> * Fix the build upstream to be smp-safe: currently tends to fail in
>   statecomp/scanner directory

After who knows how long, I finally fixed that one.

There's some argument here that we should not package Karma, because it
may not work.  I will test it.  I've also been advised to add
--disable-threaded, which is non-default but supposedly the thread does
not help performance.  I'm less excited about enabling a bunch of
non-default (and thus poorly tested) options, though.

I have created a repository here so that others may see my work with its
history:

https://github.com/omnibond/orangefs-fedora

Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=22006250
Spec: http://dev.orangefs.org/2017/marbran/0921/orangefs.spec
SRPM: http://dev.orangefs.org/2017/marbran/0921/orangefs-2.9.6-0.2.20170904svn.fc26.src.rpm

(In reply to Martin Brandenburg from comment #12)
> (In reply to Jonathan Dieter from comment #10)
> > Ok, a quick note on the licensing.  The NTP license is actually a variant of
> > MIT, and, at least according to
> > https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/
> > thread/JM7YJILE4GIFRD3J636EAT2PBOEND7WP/, should be listed as such.
> > 
> > And, according https://fedoraproject.org/wiki/Licensing:Main#Good_Licenses,
> > you can list LGPLv2.1 as LGPLv2.
> 
> And LGPLv2 and LGPLv2+?  They're still listed separately.

Yes
 
> No you've misunderstood me, though you bring up another good point.  If
> a pvfs2 filesystem appears in fstab, it cannot be mounted before the
> pvfs2-client (i.e. orangefs-client.service) is running.  The mount will
> hang until either a timeout expires or the client starts running.  Given
> systemd's parallel nature this might not actually cause a problem, but
> certainly isn't very clean.

Ok, I get it.  I have no idea what the answer is.  You could ask on #systemd on IRC, and see what they tell you.  Beyond that, I don't know that I have a solution (though, as a sysadmin, I find it a bit odd that you have to start a local daemon before mounting a remote filesystem).
 
> > Hopefully this will sort out some of the rpmlint errors I'm seeing.  I am a
> > bit concerned by the shared-lib-calls-exit warning I'm getting.  Do you
> > intend for your library to actually exit the calling program as opposed to
> > returning an error?
> 
> Unfortunately, probably yes.  I've done some quick grepping, and many of
> them look like places where an assert wouldn't be unreasonable.

Ok, that's fine.  It's not a blocker.

> > * Use unbundled lmdb (see patch)
> 
> I fixed this a while ago, though we are still shipping a copy of LMDB
> for those who want it.

That's great, but please deal with it as required at https://fedoraproject.org/wiki/Bundled_Libraries#Treatment_of_Bundled_Libraries.  Namely, please delete the lmdb directory in %prep so there's no possible way you could accidentally build against it.

> Do you use any of the security modes?  We built all that stuff, but
> haven't really heard from anyone who uses it.  I agree it would be nice
> to have, and I don't want to rule it out this early.

FWIW, if I was going to deploy OrangeFS where I work, at least one of the security modes would have to work for me.

> > * Remove the Provides: (but combinatorial builds should maybe provide
> >   "orangefs")

You've already done this.

> > * Fix strange aarch64-specific build failure, but conditionalize it
> >   for now -- ask about it on devel list?
> > 
> >   src/client/usrint/pvfs-path.c: In function 'PVFS_expand_path':
> > src/client/usrint/pvfs-path.c:266:25: error: 'SYS_readlink' undeclared
> > (first use in this function); did you mean 'SYS_readlinkat'?
> >              n = syscall(SYS_readlink,
> >                          ^~~~~~~~~~~~
> >                          SYS_readlinkat
> 
> They dropped obsolete syscalls in the kernel for the new architecture.
> That code is somewhat fragile (can't you tell from the fact it calls
> syscall instead of readlink?), so I'm not going to attempt to fix this,
> but I will forward the bug upstream.

Please do, though it is not a blocker.  There are test machines available to Fedora maintainers for this kind of thing, but according to the list at https://fedoraproject.org/wiki/Test_Machine_Resources_For_Package_Maintainers, none are aarch64.

> > * Don't try to configure the server/client; just provide -example files
> 
> Honestly that probably makes more sense than what I'm doing.  I'm going
> to go ahead and do it.

Ok, I think the way you did this probably wasn't the best choice (and I'm not convinced this is the best direction to go for your users).

Your two options are to put the examples in /usr/share/doc or to put the nonfunctioning config files (marked as config in your spec) in the usual locations (/etc and /etc/orangefs).  I personally prefer the latter, but the former is acceptable as well.  Your current approach of putting *-example into etc pollutes /etc with extra unused config files.

The reason I prefer putting nonfunctioning config files straight into the right locations is that, as a user, that's the first place I look when I want to configure them.  Copying the example config files from /usr/share/doc/ is an extra step.  I would recommend commenting out the line in /etc/pvfs2tab with a comment above it briefly explaining the correct way to configure it or at least pointing to a man page (see /etc/fstab for an example of the kind of comment I'm looking for).
 
> > * pvfs2-{start,stop}-all should be %config as they need editing?
> 
> They're also in /usr/sbin.  Putting them somewhere as an example or
> extending them to take command line arguments may make more sense.

If they require editing, you can't leave them in /usr/sbin as is.  I'd mark them as documentation.
 
> There's some argument here that we should not package Karma, because it
> may not work.  I will test it.  I've also been advised to add
> --disable-threaded, which is non-default but supposedly the thread does
> not help performance.  I'm less excited about enabling a bunch of
> non-default (and thus poorly tested) options, though.

FWIW, I briefly tested it on a single-node filesystem, and it seemed to work just fine.

Some other comments:
If possible, I think pvfs2-genconfig should output to /etc/orangefs/orangefs.conf by default instead of requiring the user to specify the location.    This is not a blocker, but I think it will make life easier for your users, especially if you choose to put orangefs.conf in /usr/share/doc as orangefs-example.conf.

What exactly is /usr/lib64/liborangefsposix.so.2.9.6?
Its contents on my system:
GROUP ( -lofs -lorangefs -lpvfs2 -lrt -lm -llmdb  -ldl -lpthread -lpthread  -lssl -lcrypto -L/usr/lib64 -libverbs )

The orangefs-client systemd service isn't logging correctly.  If you're going to use a "simple" systemd service, you really need the following flags: ExecStart=/usr/sbin/pvfs2-client -f --logtype=syslog

Also, when pvfs2-client-core exits with an error, it doesn't get passed up the stack, so systemd never realizes that the service errored out.

Starting the orangefs-client service on a system without the kernel module enabled (I tested on a F26 machine) causes the client to die.  If orangefs-fuse is installed, how hard would it be to automatically fall back to the fuse library?  For that matter, is the orangefs-client service required to use the FUSE module?

Finally, for the server, the man page is for pvfs2.conf instead of orangefs.conf.  This should be changed.

> > Hopefully this will sort out some of the rpmlint errors I'm seeing.  I am a
> > bit concerned by the shared-lib-calls-exit warning I'm getting.  Do you
> > intend for your library to actually exit the calling program as opposed to
> > returning an error?
> 
> Unfortunately, probably yes.  I've done some quick grepping, and many of
> them look like places where an assert wouldn't be unreasonable.

For what it's worth, there are plenty of packages that won't worry
about that.

> > * Use ./prepare in %prep on straight exported source
> 
> Should we?  The release tarballs will not require it.

If you package from a release, no, it's not required.  I don't know if
it's actually required otherwise, but it's normal ("best"?) practice.

> > * Use interface-related soname (see patch); the interface was
> >   sufficiently stable over some previous versions I checked with
> >   abipkgdiff to keep a major version.
> 
> Thanks for this.  I assume you are good with me merging your
> orangefs-soname.patch upstream?

Sure, if it's wanted.

> > * It needs an IB version (unless you can configure for both IB and TCP
> >   now)
> 
> It complains loudly that performance will suffer, but it's wrong or at
> least misleading.  Performance will only suffer if you actually attempt
> to use both at the same time.  Just having the code built and available
> for run-time configuration doesn't harm anything.

Oh, fine.  I thought it was a real problem last time I asked.  Can you
build with both and try to silence the complaint?

> > * Probably should have combinatorial builds over network and security
> >   types too
> 
> Do you use any of the security modes?

Probably not; I assumed others did.

> There's several network modes in src/io/bmi, but all we've focused on
> recently is bmi_tcp and bmi_ib.  I'm not sure if the others build.

Tcp and IB are the only ones I'd worry about.

> I don't want to make something that is missing features people want to
> use.
>
> > * Remove the Provides: (but combinatorial builds should maybe provide
> >   "orangefs")
> > 
> > * Make -libs-<transport> packages for use with mpi-io etc.
> 
> I have to admit I'm not entirely sure what you're talking about with
> either of these.

I think there was an explicit Provides of the libraries, which there
shouldn't be.  With multiple packages for different security modes
(assuming you don't need them for different transports), you probably
want to be able to require something like "orangefs" to get one of
them.

Assuming the -<transport> bit isn't relevant now, I can't remember
what was there, but to build MPI-IO support, you need a -devel package
and a package just providing relevant libraries without pulling in
executables.

> 
> > * Fix strange aarch64-specific build failure, but conditionalize it
> >   for now -- ask about it on devel list?
> > 
> >   src/client/usrint/pvfs-path.c: In function 'PVFS_expand_path':
> > src/client/usrint/pvfs-path.c:266:25: error: 'SYS_readlink' undeclared
> > (first use in this function); did you mean 'SYS_readlinkat'?
> >              n = syscall(SYS_readlink,
> >                          ^~~~~~~~~~~~
> >                          SYS_readlinkat
> 
> They dropped obsolete syscalls in the kernel for the new architecture.
> That code is somewhat fragile (can't you tell from the fact it calls
> syscall instead of readlink?), so I'm not going to attempt to fix this,
> but I will forward the bug upstream.

You can at least conditionalize it so that you can build most of
orangefs on those platforms.  I think I tested that.

> > * Expand description?
> 
> Not a bad idea.  May I take (and probably tweak) your description?

Sure, if its helpful.

> > * Include pacemaker stuff
> 
> Again I have to admit I don't know what this is.

It's for high-availability support (not that I've used it).  It's
trivial to put in, anyhow.

> > * pvfs2-{start,stop}-all should be %config as they need editing?
> 
> They're also in /usr/sbin.  Putting them somewhere as an example or
> extending them to take command line arguments may make more sense.

Yes, or somehow make them extensible.

> There's some argument here that we should not package Karma, because it
> may not work.

I had a note that it had been replaced by grafana support, which I
assume I got from the list, but it sounds as if that's wrong.

> I will test it.  I've also been advised to add
> --disable-threaded, which is non-default but supposedly the thread does
> not help performance.  I'm less excited about enabling a bunch of
> non-default (and thus poorly tested) options, though.

Right, if there's a good reason they're not defaulted other than build
requirements.

(In reply to Jonathan Dieter from comment #13)

> What exactly is /usr/lib64/liborangefsposix.so.2.9.6?
> Its contents on my system:
> GROUP ( -lofs -lorangefs -lpvfs2 -lrt -lm -llmdb  -ldl -lpthread -lpthread 
> -lssl -lcrypto -L/usr/lib64 -libverbs )

It's a linker script, special-cased by by soname patch.

(In reply to Jonathan Dieter from comment #13)
> (In reply to Martin Brandenburg from comment #12)
> > (In reply to Jonathan Dieter from comment #10)
>  
> > No you've misunderstood me, though you bring up another good point.  If
> > a pvfs2 filesystem appears in fstab, it cannot be mounted before the
> > pvfs2-client (i.e. orangefs-client.service) is running.  The mount will
> > hang until either a timeout expires or the client starts running.  Given
> > systemd's parallel nature this might not actually cause a problem, but
> > certainly isn't very clean.
> 
> Ok, I get it.  I have no idea what the answer is.  You could ask on #systemd
> on IRC, and see what they tell you.  Beyond that, I don't know that I have a
> solution (though, as a sysadmin, I find it a bit odd that you have to start
> a local daemon before mounting a remote filesystem).

Yeah it's a little obtuse.

> > > * Use unbundled lmdb (see patch)
> > 
> > I fixed this a while ago, though we are still shipping a copy of LMDB
> > for those who want it.
> 
> That's great, but please deal with it as required at
> https://fedoraproject.org/wiki/
> Bundled_Libraries#Treatment_of_Bundled_Libraries.  Namely, please delete the
> lmdb directory in %prep so there's no possible way you could accidentally
> build against it.

Sounds good.  Done.

> > Do you use any of the security modes?  We built all that stuff, but
> > haven't really heard from anyone who uses it.  I agree it would be nice
> > to have, and I don't want to rule it out this early.
> 
> FWIW, if I was going to deploy OrangeFS where I work, at least one of the
> security modes would have to work for me.

Quite.

The advice I got from rdieter in #fedora-devel is to compile is three
times within the specfile and then use subpackages.

If there's no better solution, I may just declare it out of scope for
this package and that if we want it, we'll have to make security a
run-time option rather than a compile-time option.

> > > * Fix strange aarch64-specific build failure, but conditionalize it
> > >   for now -- ask about it on devel list?
> > > 
> > >   src/client/usrint/pvfs-path.c: In function 'PVFS_expand_path':
> > > src/client/usrint/pvfs-path.c:266:25: error: 'SYS_readlink' undeclared
> > > (first use in this function); did you mean 'SYS_readlinkat'?
> > >              n = syscall(SYS_readlink,
> > >                          ^~~~~~~~~~~~
> > >                          SYS_readlinkat
> > 
> > They dropped obsolete syscalls in the kernel for the new architecture.
> > That code is somewhat fragile (can't you tell from the fact it calls
> > syscall instead of readlink?), so I'm not going to attempt to fix this,
> > but I will forward the bug upstream.
> 
> Please do, though it is not a blocker.  There are test machines available to
> Fedora maintainers for this kind of thing, but according to the list at
> https://fedoraproject.org/wiki/
> Test_Machine_Resources_For_Package_Maintainers, none are aarch64.

Yes, unfortunately the exotic architectures are harder to find.  But we
know what the problem is at least.

> > > * Don't try to configure the server/client; just provide -example files
> > 
> > Honestly that probably makes more sense than what I'm doing.  I'm going
> > to go ahead and do it.
> 
> Ok, I think the way you did this probably wasn't the best choice (and I'm
> not convinced this is the best direction to go for your users).
> 
> Your two options are to put the examples in /usr/share/doc or to put the
> nonfunctioning config files (marked as config in your spec) in the usual
> locations (/etc and /etc/orangefs).  I personally prefer the latter, but the
> former is acceptable as well.  Your current approach of putting *-example
> into etc pollutes /etc with extra unused config files.
> 
> The reason I prefer putting nonfunctioning config files straight into the
> right locations is that, as a user, that's the first place I look when I
> want to configure them.  Copying the example config files from
> /usr/share/doc/ is an extra step.  I would recommend commenting out the line
> in /etc/pvfs2tab with a comment above it briefly explaining the correct way
> to configure it or at least pointing to a man page (see /etc/fstab for an
> example of the kind of comment I'm looking for).

Sounds good.  For the server config, shall I put in a DeleteMe option
that the server will throw a syntax error on?  Well even without, the
server will throw an error on the missing storage space, and the user
will have to read the documentation or at least look in the config file
to learn how to create it.

> > > * pvfs2-{start,stop}-all should be %config as they need editing?
> > 
> > They're also in /usr/sbin.  Putting them somewhere as an example or
> > extending them to take command line arguments may make more sense.
> 
> If they require editing, you can't leave them in /usr/sbin as is.  I'd mark
> them as documentation.

Right.  I could move them to /usr/share/doc/orangefs, but I don't see
where they require editing.

> > There's some argument here that we should not package Karma, because it
> > may not work.  I will test it.  I've also been advised to add
> > --disable-threaded, which is non-default but supposedly the thread does
> > not help performance.  I'm less excited about enabling a bunch of
> > non-default (and thus poorly tested) options, though.
> 
> FWIW, I briefly tested it on a single-node filesystem, and it seemed to work
> just fine.

I thought so too, but see my response to Dave below.

> Some other comments:
> If possible, I think pvfs2-genconfig should output to
> /etc/orangefs/orangefs.conf by default instead of requiring the user to
> specify the location.    This is not a blocker, but I think it will make
> life easier for your users, especially if you choose to put orangefs.conf in
> /usr/share/doc as orangefs-example.conf.

I'll put in a default.  It'll prompt, so the user can specify whatever,
but the user can press return to get the default like some of the other
prompts.  This seems like something to submit upstream as well.

> The orangefs-client systemd service isn't logging correctly.  If you're
> going to use a "simple" systemd service, you really need the following
> flags: ExecStart=/usr/sbin/pvfs2-client -f --logtype=syslog
> 
> Also, when pvfs2-client-core exits with an error, it doesn't get passed up
> the stack, so systemd never realizes that the service errored out.

I run the pvfs2-client-core from systemd now.

> Starting the orangefs-client service on a system without the kernel module
> enabled (I tested on a F26 machine) causes the client to die.  If
> orangefs-fuse is installed, how hard would it be to automatically fall back
> to the fuse library?  For that matter, is the orangefs-client service
> required to use the FUSE module?

It absolutely needs to print an error message if the kernel module is
unavailable.  That's been on my todo list for a while, but thanks for
bringing it up.

I don't think it'll be possible to fallback to FUSE.

> Finally, for the server, the man page is for pvfs2.conf instead of
> orangefs.conf.  This should be changed.

Done.

(In reply to Dave Love from comment #14)
> > > Hopefully this will sort out some of the rpmlint errors I'm seeing.  I am a
> > > bit concerned by the shared-lib-calls-exit warning I'm getting.  Do you
> > > intend for your library to actually exit the calling program as opposed to
> > > returning an error?
> > 
> > Unfortunately, probably yes.  I've done some quick grepping, and many of
> > them look like places where an assert wouldn't be unreasonable.
> 
> For what it's worth, there are plenty of packages that won't worry
> about that.
> 
> > > * Use ./prepare in %prep on straight exported source
> > 
> > Should we?  The release tarballs will not require it.
> 
> If you package from a release, no, it's not required.  I don't know if
> it's actually required otherwise, but it's normal ("best"?) practice.

I've got to do it to remove LMDB, so it's in.

> > > * Use interface-related soname (see patch); the interface was
> > >   sufficiently stable over some previous versions I checked with
> > >   abipkgdiff to keep a major version.
> > 
> > Thanks for this.  I assume you are good with me merging your
> > orangefs-soname.patch upstream?
> 
> Sure, if it's wanted.
> 
> > > * It needs an IB version (unless you can configure for both IB and TCP
> > >   now)
> > 
> > It complains loudly that performance will suffer, but it's wrong or at
> > least misleading.  Performance will only suffer if you actually attempt
> > to use both at the same time.  Just having the code built and available
> > for run-time configuration doesn't harm anything.
> 
> Oh, fine.  I thought it was a real problem last time I asked.  Can you
> build with both and try to silence the complaint?

We'll remove the complaint since it's wrong.

> > > * Remove the Provides: (but combinatorial builds should maybe provide
> > >   "orangefs")
> > > 
> > > * Make -libs-<transport> packages for use with mpi-io etc.
> > 
> > I have to admit I'm not entirely sure what you're talking about with
> > either of these.
> 
> I think there was an explicit Provides of the libraries, which there
> shouldn't be.  With multiple packages for different security modes
> (assuming you don't need them for different transports), you probably
> want to be able to require something like "orangefs" to get one of
> them.

I think I fixed that.  I'm going to look into the security modes though.

> Assuming the -<transport> bit isn't relevant now, I can't remember
> what was there, but to build MPI-IO support, you need a -devel package
> and a package just providing relevant libraries without pulling in
> executables.

Okay MPI-IO.  I do know about that.  But OpenMPI or MPICH or something
else?  I don't have a preference, but I'll ask around here.

> > 
> > > * Fix strange aarch64-specific build failure, but conditionalize it
> > >   for now -- ask about it on devel list?
> > > 
> > >   src/client/usrint/pvfs-path.c: In function 'PVFS_expand_path':
> > > src/client/usrint/pvfs-path.c:266:25: error: 'SYS_readlink' undeclared
> > > (first use in this function); did you mean 'SYS_readlinkat'?
> > >              n = syscall(SYS_readlink,
> > >                          ^~~~~~~~~~~~
> > >                          SYS_readlinkat
> > 
> > They dropped obsolete syscalls in the kernel for the new architecture.
> > That code is somewhat fragile (can't you tell from the fact it calls
> > syscall instead of readlink?), so I'm not going to attempt to fix this,
> > but I will forward the bug upstream.
> 
> You can at least conditionalize it so that you can build most of
> orangefs on those platforms.  I think I tested that.

Jonathan, what do you think?  The options are to drop support for
aarch64 entirely since part of it will be missing or build what will
work.

> > > * Expand description?
> > 
> > Not a bad idea.  May I take (and probably tweak) your description?
> 
> Sure, if its helpful.
> 
> > > * Include pacemaker stuff
> > 
> > Again I have to admit I don't know what this is.
> 
> It's for high-availability support (not that I've used it).  It's
> trivial to put in, anyhow.

It turns out we call it heartbeat, but it looks like the same thing.
I'll look into this.

> > > * pvfs2-{start,stop}-all should be %config as they need editing?
> > 
> > They're also in /usr/sbin.  Putting them somewhere as an example or
> > extending them to take command line arguments may make more sense.
> 
> Yes, or somehow make them extensible.

See above.  I don't see where they need editing, but I could be missing
something.

> > There's some argument here that we should not package Karma, because it
> > may not work.
> 
> I had a note that it had been replaced by grafana support, which I
> assume I got from the list, but it sounds as if that's wrong.

I had the same note.  I don't think it's readily available.

> > I will test it.  I've also been advised to add
> > --disable-threaded, which is non-default but supposedly the thread does
> > not help performance.  I'm less excited about enabling a bunch of
> > non-default (and thus poorly tested) options, though.
> 
> Right, if there's a good reason they're not defaulted other than build
> requirements.

I think a lot of stuff isn't tweaked out of the box simply because we
haven't tested one way or the other.

Git: https://github.com/omnibond/orangefs-fedora
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=22006250
Spec: http://dev.orangefs.org/2017/marbran/1002/orangefs.spec
SRPM: http://dev.orangefs.org/2017/marbran/1002/orangefs-2.9.6-0.3.20171002.svn.fc26.src.rpm

I've trimmed a lot from my reply.  Anything trimmed looks great!

(In reply to Martin Brandenburg from comment #16)
> (In reply to Jonathan Dieter from comment #13)
> > FWIW, if I was going to deploy OrangeFS where I work, at least one of the
> > security modes would have to work for me.
> 
> Quite.
> 
> The advice I got from rdieter in #fedora-devel is to compile is three
> times within the specfile and then use subpackages.
> 
> If there's no better solution, I may just declare it out of scope for
> this package and that if we want it, we'll have to make security a
> run-time option rather than a compile-time option.
> 

Rex's (and, since I'm sure you wondered, no we're not related) final advice to focus on one first was good, so I think declaring it out of scope until you guys have made security a run-time option is the best option here.

> > The reason I prefer putting nonfunctioning config files straight into the
> > right locations is that, as a user, that's the first place I look when I
> > want to configure them.  Copying the example config files from
> > /usr/share/doc/ is an extra step.  I would recommend commenting out the line
> > in /etc/pvfs2tab with a comment above it briefly explaining the correct way
> > to configure it or at least pointing to a man page (see /etc/fstab for an
> > example of the kind of comment I'm looking for).
> 
> Sounds good.  For the server config, shall I put in a DeleteMe option
> that the server will throw a syntax error on?  Well even without, the
> server will throw an error on the missing storage space, and the user
> will have to read the documentation or at least look in the config file
> to learn how to create it.

If the server will throw an error anyway, I wouldn't bother with a DeleteMe option.

> > > > * pvfs2-{start,stop}-all should be %config as they need editing?
> > > 
> > > They're also in /usr/sbin.  Putting them somewhere as an example or
> > > extending them to take command line arguments may make more sense.
> > 
> > If they require editing, you can't leave them in /usr/sbin as is.  I'd mark
> > them as documentation.
> 
> Right.  I could move them to /usr/share/doc/orangefs, but I don't see
> where they require editing.

I looked through them and it seems that they *can* be edited.  If upstream *recommends* editing the scripts directly, then they should probably go in /usr/share/doc/orangefs, otherwise you can probably leave them where they're at.

> (In reply to Dave Love from comment #14) 
> > You can at least conditionalize it so that you can build most of
> > orangefs on those platforms.  I think I tested that.
> 
> Jonathan, what do you think?  The options are to drop support for
> aarch64 entirely since part of it will be missing or build what will
> work.

I'd probably prefer to build what will work, but conditionalize it so the missing stuff is only missing on aarch64.

> > > There's some argument here that we should not package Karma, because it
> > > may not work.
> > 
> > I had a note that it had been replaced by grafana support, which I
> > assume I got from the list, but it sounds as if that's wrong.
> 
> I had the same note.  I don't think it's readily available.

Including karma isn't a blocker for me.  If it's not supported, we probably shouldn't build it.  On the other hand, if the alternative isn't actually available, that kind of sucks.  You go ahead and make the call.
 
> Git: https://github.com/omnibond/orangefs-fedora
> Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=22006250
> Spec: http://dev.orangefs.org/2017/marbran/1002/orangefs.spec
> SRPM:
> http://dev.orangefs.org/2017/marbran/1002/orangefs-2.9.6-0.3.20171002.svn.
> fc26.src.rpm

The SRPM link gives me a 404.

(In reply to Jonathan Dieter from comment #17)
> I've trimmed a lot from my reply.  Anything trimmed looks great!
> 
> (In reply to Martin Brandenburg from comment #16)
> > (In reply to Jonathan Dieter from comment #13)
> > > FWIW, if I was going to deploy OrangeFS where I work, at least one of the
> > > security modes would have to work for me.
> > 
> > Quite.
> > 
> > The advice I got from rdieter in #fedora-devel is to compile is three
> > times within the specfile and then use subpackages.
> > 
> > If there's no better solution, I may just declare it out of scope for
> > this package and that if we want it, we'll have to make security a
> > run-time option rather than a compile-time option.
> > 
> 
> Rex's (and, since I'm sure you wondered, no we're not related) final advice
> to focus on one first was good, so I think declaring it out of scope until
> you guys have made security a run-time option is the best option here.
> 
> > > The reason I prefer putting nonfunctioning config files straight into the
> > > right locations is that, as a user, that's the first place I look when I
> > > want to configure them.  Copying the example config files from
> > > /usr/share/doc/ is an extra step.  I would recommend commenting out the line
> > > in /etc/pvfs2tab with a comment above it briefly explaining the correct way
> > > to configure it or at least pointing to a man page (see /etc/fstab for an
> > > example of the kind of comment I'm looking for).
> > 
> > Sounds good.  For the server config, shall I put in a DeleteMe option
> > that the server will throw a syntax error on?  Well even without, the
> > server will throw an error on the missing storage space, and the user
> > will have to read the documentation or at least look in the config file
> > to learn how to create it.
> 
> If the server will throw an error anyway, I wouldn't bother with a DeleteMe
> option.
> 
> > > > > * pvfs2-{start,stop}-all should be %config as they need editing?
> > > > 
> > > > They're also in /usr/sbin.  Putting them somewhere as an example or
> > > > extending them to take command line arguments may make more sense.
> > > 
> > > If they require editing, you can't leave them in /usr/sbin as is.  I'd mark
> > > them as documentation.
> > 
> > Right.  I could move them to /usr/share/doc/orangefs, but I don't see
> > where they require editing.
> 
> I looked through them and it seems that they *can* be edited.  If upstream
> *recommends* editing the scripts directly, then they should probably go in
> /usr/share/doc/orangefs, otherwise you can probably leave them where they're
> at.

It looks to me like they accept command line options, but can be edited
to code in specific defaults.  They also don't use systemd.  As a
sysadmin, I'd rather have systemd start the daemon at boot on any
machine I wish to run it on.

I don't see any reference to them at all in any of our documentation.

So I think the conclusion is that I'll move them to
/usr/share/doc/orangefs if I don't drop them entirely.

While we're on the subject there's some docs I should build and install
there.

> > (In reply to Dave Love from comment #14) 
> > > You can at least conditionalize it so that you can build most of
> > > orangefs on those platforms.  I think I tested that.
> > 
> > Jonathan, what do you think?  The options are to drop support for
> > aarch64 entirely since part of it will be missing or build what will
> > work.
> 
> I'd probably prefer to build what will work, but conditionalize it so the
> missing stuff is only missing on aarch64.

I'll do that then.  I'd certainly prefer to run on more systems than
less.

> > > > There's some argument here that we should not package Karma, because it
> > > > may not work.
> > > 
> > > I had a note that it had been replaced by grafana support, which I
> > > assume I got from the list, but it sounds as if that's wrong.
> > 
> > I had the same note.  I don't think it's readily available.
> 
> Including karma isn't a blocker for me.  If it's not supported, we probably
> shouldn't build it.  On the other hand, if the alternative isn't actually
> available, that kind of sucks.  You go ahead and make the call.

I think I'll drop that.

> > Git: https://github.com/omnibond/orangefs-fedora
> > Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=22006250
> > Spec: http://dev.orangefs.org/2017/marbran/1002/orangefs.spec
> > SRPM:
> > http://dev.orangefs.org/2017/marbran/1002/orangefs-2.9.6-0.3.20171002.svn.
> > fc26.src.rpm
> 
> The SRPM link gives me a 404.

Looks like I typed an extra dot.  Try

http://dev.orangefs.org/2017/marbran/1002/orangefs-2.9.6-0.3.20171002svn.fc26.src.rpm

I have removed Karma, re-enabled aarch64 without the usrint, and dropped
the start/stop scripts.

I have also tested an EPEL build, which worked with no changes.

Git: https://github.com/omnibond/orangefs-fedora
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=22385794
Spec: http://dev.orangefs.org/2017/marbran/1011/orangefs.spec
SRPM: http://dev.orangefs.org/2017/marbran/1011/orangefs-2.9.6-0.4.20171004svn.fc26.src.rpm

Thanks so much, this is looking better.  I'm enclosing the rpmlint output because there are a number of problems that need to be investigated.

You can ignore the shared-lib-calls-exit warnings, the script-without-shebang warning, the no-manual-page-for-binary warning (assuming you're not planning to write some new man pages), the useless-provides for the debuginfo subpackage, and the invalid-url warning.  

Please check the rest of them and either fix them or explain why they're there.

Rpmlint
-------
Checking: orangefs-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
          orangefs-debuginfo-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
          orangefs-devel-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
          orangefs-server-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
          orangefs-fuse-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
          orangefs-2.9.6-0.4.20171004svn.fc28.src.rpm
orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libofs.so.2.9.6 exit.5
orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/liborangefs.so.2.9.6 exit.5
orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpvfs2.so.2.9.6 exit.5
orangefs.x86_64: E: script-without-shebang /usr/lib64/liborangefsposix.so.2.9.6
orangefs.x86_64: W: manual-page-warning /usr/share/man/man1/pvfs2-drop-caches.1.gz 13: warning: numeric expression expected (got `f')
orangefs.x86_64: W: manual-page-warning /usr/share/man/man1/pvfs2-fs-dump.1.gz 15: warning: numeric expression expected (got `m')
orangefs.x86_64: W: no-manual-page-for-binary ofs_cp
orangefs.x86_64: W: no-manual-page-for-binary ofs_graphite_driver
orangefs.x86_64: W: no-manual-page-for-binary ofs_rm
orangefs.x86_64: W: no-manual-page-for-binary ofs_setdirhint
orangefs-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
orangefs-devel.x86_64: W: no-dependency-on orangefs/orangefs-libs/liborangefs
orangefs-devel.x86_64: W: only-non-binary-in-usr-lib
orangefs-devel.x86_64: W: no-documentation
orangefs-devel.x86_64: W: no-manual-page-for-binary pvfs2-config
orangefs-server.x86_64: W: conffile-without-noreplace-flag /etc/orangefs/orangefs.conf
orangefs-server.x86_64: W: conffile-without-noreplace-flag /etc/pvfs2tab
orangefs-fuse.x86_64: W: only-non-binary-in-usr-lib
orangefs-fuse.x86_64: W: no-documentation
orangefs-fuse.x86_64: W: no-manual-page-for-binary pvfs2fuse
orangefs.src:26: W: unversioned-explicit-provides libpvfs2.so()(64bit)
orangefs.src:28: W: unversioned-explicit-provides libofs.so()(64bit)
orangefs.src:28: W: unversioned-explicit-provides liborangefs.so()(64bit)
orangefs.src:28: W: unversioned-explicit-provides libpvfs2.so()(64bit)
orangefs.src:39: W: macro-in-comment %{version}
orangefs.src:289: W: macro-in-%changelog %doc
orangefs.src: E: specfile-error warning: bogus date in %changelog: Mon Oct 11 2017 Martin Brandenburg <martin> - 2.9.6-0.4.20171004svn
6 packages and 0 specfiles checked; 3 errors, 24 warnings.




Rpmlint (debuginfo)
-------------------
Checking: orangefs-debuginfo-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
          orangefs-server-debuginfo-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
          orangefs-fuse-debuginfo-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
orangefs-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
orangefs-server-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
3 packages and 0 specfiles checked; 2 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
orangefs-fuse.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen error [Errno -2] Name or service not known>
orangefs-fuse.x86_64: W: only-non-binary-in-usr-lib
orangefs-fuse.x86_64: W: no-documentation
orangefs-fuse.x86_64: W: no-manual-page-for-binary pvfs2fuse
orangefs-fuse-debuginfo.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen error [Errno -2] Name or service not known>
orangefs-debuginfo.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen error [Errno -2] Name or service not known>
orangefs-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
orangefs-devel.x86_64: W: no-dependency-on orangefs/orangefs-libs/liborangefs
orangefs-devel.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen error [Errno -2] Name or service not known>
orangefs-devel.x86_64: W: only-non-binary-in-usr-lib
orangefs-devel.x86_64: W: no-documentation
orangefs-devel.x86_64: W: no-manual-page-for-binary pvfs2-config
orangefs-server.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen error [Errno -2] Name or service not known>
orangefs-server.x86_64: W: conffile-without-noreplace-flag /etc/orangefs/orangefs.conf
orangefs-server.x86_64: W: conffile-without-noreplace-flag /etc/pvfs2tab
orangefs.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen error [Errno -2] Name or service not known>
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libofs.so.2.9.6 /lib64/librt.so.1
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libofs.so.2.9.6 /lib64/libm.so.6
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libofs.so.2.9.6 /lib64/liblmdb.so.0.0.0
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libofs.so.2.9.6 /lib64/libssl.so.1.1
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libofs.so.2.9.6 /lib64/libcrypto.so.1.1
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libofs.so.2.9.6 /lib64/libibverbs.so.1
orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libofs.so.2.9.6 exit.5
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/liborangefs.so.2.9.6 /lib64/librt.so.1
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/liborangefs.so.2.9.6 /lib64/libm.so.6
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/liborangefs.so.2.9.6 /lib64/liblmdb.so.0.0.0
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/liborangefs.so.2.9.6 /lib64/libssl.so.1.1
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/liborangefs.so.2.9.6 /lib64/libcrypto.so.1.1
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/liborangefs.so.2.9.6 /lib64/libibverbs.so.1
orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/liborangefs.so.2.9.6 exit.5
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpvfs2.so.2.9.6 /lib64/liblmdb.so.0.0.0
orangefs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpvfs2.so.2.9.6 /lib64/libssl.so.1.1
orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpvfs2.so.2.9.6 exit.5
orangefs.x86_64: E: script-without-shebang /usr/lib64/liborangefsposix.so.2.9.6
orangefs.x86_64: W: manual-page-warning /usr/share/man/man1/pvfs2-drop-caches.1.gz 13: warning: numeric expression expected (got `f')
orangefs.x86_64: W: manual-page-warning /usr/share/man/man1/pvfs2-fs-dump.1.gz 15: warning: numeric expression expected (got `m')
orangefs.x86_64: W: no-manual-page-for-binary ofs_cp
orangefs.x86_64: W: no-manual-page-for-binary ofs_graphite_driver
orangefs.x86_64: W: no-manual-page-for-binary ofs_rm
orangefs.x86_64: W: no-manual-page-for-binary ofs_setdirhint
orangefs-server-debuginfo.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen error [Errno -2] Name or service not known>
orangefs-server-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
7 packages and 0 specfiles checked; 3 errors, 39 warnings.

(In reply to Jonathan Dieter from comment #20)
> Thanks so much, this is looking better.  I'm enclosing the rpmlint output
> because there are a number of problems that need to be investigated.
> 
> You can ignore the shared-lib-calls-exit warnings, the
> script-without-shebang warning, the no-manual-page-for-binary warning
> (assuming you're not planning to write some new man pages), the
> useless-provides for the debuginfo subpackage, and the invalid-url warning.  
> 
> Please check the rest of them and either fix them or explain why they're
> there.
> 
> Rpmlint
> -------
> Checking: orangefs-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
>           orangefs-debuginfo-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
>           orangefs-devel-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
>           orangefs-server-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
>           orangefs-fuse-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
>           orangefs-2.9.6-0.4.20171004svn.fc28.src.rpm
> orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libofs.so.2.9.6
> exit.5
> orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/liborangefs.so.2.9.6
> exit.5
> orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpvfs2.so.2.9.6
> exit.5
> orangefs.x86_64: E: script-without-shebang
> /usr/lib64/liborangefsposix.so.2.9.6
> orangefs.x86_64: W: manual-page-warning
> /usr/share/man/man1/pvfs2-drop-caches.1.gz 13: warning: numeric expression
> expected (got `f')
> orangefs.x86_64: W: manual-page-warning
> /usr/share/man/man1/pvfs2-fs-dump.1.gz 15: warning: numeric expression
> expected (got `m')

These are fixed.

> orangefs.x86_64: W: no-manual-page-for-binary ofs_cp
> orangefs.x86_64: W: no-manual-page-for-binary ofs_graphite_driver
> orangefs.x86_64: W: no-manual-page-for-binary ofs_rm
> orangefs.x86_64: W: no-manual-page-for-binary ofs_setdirhint

I wrote a whole bunch of manpages before submitting for the first time,
so I will write more.

> orangefs-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
> orangefs-devel.x86_64: W: no-dependency-on orangefs/orangefs-libs/liborangefs

Fixed.

> orangefs-devel.x86_64: W: only-non-binary-in-usr-lib

I'm guessing this is the linker script...

> orangefs-devel.x86_64: W: no-documentation
> orangefs-devel.x86_64: W: no-manual-page-for-binary pvfs2-config
> orangefs-server.x86_64: W: conffile-without-noreplace-flag
> /etc/orangefs/orangefs.conf
> orangefs-server.x86_64: W: conffile-without-noreplace-flag /etc/pvfs2tab

Fixed.

> orangefs-fuse.x86_64: W: only-non-binary-in-usr-lib
> orangefs-fuse.x86_64: W: no-documentation
> orangefs-fuse.x86_64: W: no-manual-page-for-binary pvfs2fuse
> orangefs.src:26: W: unversioned-explicit-provides libpvfs2.so()(64bit)
> orangefs.src:28: W: unversioned-explicit-provides libofs.so()(64bit)
> orangefs.src:28: W: unversioned-explicit-provides liborangefs.so()(64bit)
> orangefs.src:28: W: unversioned-explicit-provides libpvfs2.so()(64bit)
> orangefs.src:39: W: macro-in-comment %{version}

This will go away when the real release is made, but until then I don't
want to lose the real URL.

> orangefs.src:289: W: macro-in-%changelog %doc

I should write 'percent-doc' I guess?

> orangefs.src: E: specfile-error warning: bogus date in %changelog: Mon Oct
> 11 2017 Martin Brandenburg <martin> -
> 2.9.6-0.4.20171004svn

I guess it is Wednesday.

> 6 packages and 0 specfiles checked; 3 errors, 24 warnings.
> 
> 
> 
> 
> Rpmlint (debuginfo)
> -------------------
> Checking: orangefs-debuginfo-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
>           orangefs-server-debuginfo-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
>           orangefs-fuse-debuginfo-2.9.6-0.4.20171004svn.fc28.x86_64.rpm
> orangefs-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
> orangefs-server-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
> 3 packages and 0 specfiles checked; 2 errors, 0 warnings.
> 
> 
> 
> 
> 
> Rpmlint (installed packages)
> ----------------------------
> sh: /usr/bin/python: No such file or directory

I don't know where this comes from?

> orangefs-fuse.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen
> error [Errno -2] Name or service not known>

I assume the network is blocked wherever this is run?

> orangefs-fuse.x86_64: W: only-non-binary-in-usr-lib
> orangefs-fuse.x86_64: W: no-documentation
> orangefs-fuse.x86_64: W: no-manual-page-for-binary pvfs2fuse
> orangefs-fuse-debuginfo.x86_64: W: invalid-url URL: http://www.orangefs.org/
> <urlopen error [Errno -2] Name or service not known>
> orangefs-debuginfo.x86_64: W: invalid-url URL: http://www.orangefs.org/
> <urlopen error [Errno -2] Name or service not known>
> orangefs-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
> orangefs-devel.x86_64: W: no-dependency-on orangefs/orangefs-libs/liborangefs
> orangefs-devel.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen
> error [Errno -2] Name or service not known>
> orangefs-devel.x86_64: W: only-non-binary-in-usr-lib
> orangefs-devel.x86_64: W: no-documentation
> orangefs-devel.x86_64: W: no-manual-page-for-binary pvfs2-config
> orangefs-server.x86_64: W: invalid-url URL: http://www.orangefs.org/
> <urlopen error [Errno -2] Name or service not known>
> orangefs-server.x86_64: W: conffile-without-noreplace-flag
> /etc/orangefs/orangefs.conf
> orangefs-server.x86_64: W: conffile-without-noreplace-flag /etc/pvfs2tab
> orangefs.x86_64: W: invalid-url URL: http://www.orangefs.org/ <urlopen error
> [Errno -2] Name or service not known>
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libofs.so.2.9.6 /lib64/librt.so.1
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libofs.so.2.9.6 /lib64/libm.so.6
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libofs.so.2.9.6 /lib64/liblmdb.so.0.0.0
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libofs.so.2.9.6 /lib64/libssl.so.1.1
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libofs.so.2.9.6 /lib64/libcrypto.so.1.1
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libofs.so.2.9.6 /lib64/libibverbs.so.1
> orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libofs.so.2.9.6
> exit.5
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/liborangefs.so.2.9.6 /lib64/librt.so.1
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/liborangefs.so.2.9.6 /lib64/libm.so.6
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/liborangefs.so.2.9.6 /lib64/liblmdb.so.0.0.0
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/liborangefs.so.2.9.6 /lib64/libssl.so.1.1
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/liborangefs.so.2.9.6 /lib64/libcrypto.so.1.1
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/liborangefs.so.2.9.6 /lib64/libibverbs.so.1

This is the only new non-trivial thing I see.  It is fixed now.

> orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/liborangefs.so.2.9.6
> exit.5
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpvfs2.so.2.9.6 /lib64/liblmdb.so.0.0.0
> orangefs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpvfs2.so.2.9.6 /lib64/libssl.so.1.1
> orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpvfs2.so.2.9.6
> exit.5
> orangefs.x86_64: E: script-without-shebang
> /usr/lib64/liborangefsposix.so.2.9.6
> orangefs.x86_64: W: manual-page-warning
> /usr/share/man/man1/pvfs2-drop-caches.1.gz 13: warning: numeric expression
> expected (got `f')
> orangefs.x86_64: W: manual-page-warning
> /usr/share/man/man1/pvfs2-fs-dump.1.gz 15: warning: numeric expression
> expected (got `m')
> orangefs.x86_64: W: no-manual-page-for-binary ofs_cp
> orangefs.x86_64: W: no-manual-page-for-binary ofs_graphite_driver
> orangefs.x86_64: W: no-manual-page-for-binary ofs_rm
> orangefs.x86_64: W: no-manual-page-for-binary ofs_setdirhint
> orangefs-server-debuginfo.x86_64: W: invalid-url URL:
> http://www.orangefs.org/ <urlopen error [Errno -2] Name or service not known>
> orangefs-server-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
> 7 packages and 0 specfiles checked; 3 errors, 39 warnings.

Git: https://github.com/omnibond/orangefs-fedora
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=22391406
Spec: http://dev.orangefs.org/2017/marbran/1011/2/orangefs.spec
SRPM: http://dev.orangefs.org/2017/marbran/1011/2/orangefs-2.9.6-0.5.20171011svn.fc26.src.rpm

I now have

$ rpmlint *.src.rpm
orangefs.src:35: W: macro-in-comment %{version}
orangefs.src: W: invalid-url Source0: orangefs-2.9.6.tar.gz
1 packages and 0 specfiles checked; 0 errors, 2 warnings.
$ rpmlint orangefs orangefs-debuginfo orangefs-devel orangefs-fuse orangefs-server
orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libofs.so.2.9.6 exit.5
orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/liborangefs.so.2.9.6 exit.5
orangefs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpvfs2.so.2.9.6 exit.5
orangefs.x86_64: E: script-without-shebang /usr/lib64/liborangefsposix.so.2.9.6
orangefs.x86_64: W: no-manual-page-for-binary ofs_cp
orangefs.x86_64: W: no-manual-page-for-binary ofs_graphite_driver
orangefs.x86_64: W: no-manual-page-for-binary ofs_rm
orangefs.x86_64: W: no-manual-page-for-binary ofs_setdirhint
orangefs-devel.x86_64: W: only-non-binary-in-usr-lib
orangefs-devel.x86_64: W: no-documentation
orangefs-devel.x86_64: W: no-manual-page-for-binary pvfs2-config
orangefs-fuse.x86_64: W: no-documentation
orangefs-fuse.x86_64: W: no-manual-page-for-binary pvfs2fuse
5 packages and 0 specfiles checked; 1 errors, 12 warnings.

So I've put all the documentation in the main package, but some of it is
focused at developers.  This is things like an overview of the design
of various subsystems.  It may not be required reading but anyone
intending to deploy in production should probably read most of it.
Should any of this go in the -devel subpackage instead?

That leaves only-non-binary-in-usr-lib, which I think is the linker
script.  What should I do about it?

Thanks so much for clearing out all of the important rpmlint issues (even if some of them were pretty trivial).  We're finally close enough to the end that I'm posting my full review, though there are still a few things that need to be sorted out.  Please note the problems both with MUST and SHOULD:

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[!]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.

     COPYING is available in tarball but not installed as %license

[!]: License file installed when any subpackage combination is installed.

     Since COPYING represents the overall license of the collected work, please 
     make sure COPYING is installed as %license if the server or fuse client is 
     installed (I'm not sure if either are supposed to Require: orangefs)

[!]: Package must own all directories that it creates.

     Directories without known owners:
     /usr/share/doc/orangefs/design, /usr/share/doc/orangefs,
     /usr/share/doc/orangefs/coding, /usr/share/doc/orangefs/random
     You do need to own these documentation directories 

[!]: Rpmlint is run on all rpms the build produces.

     Rpmlint was run, and all major warnings have been fixed, but I'm still
     uncomfortable with the script-without-shebang error we're seeing with the
     linker script.  I believe this is because the linker script is executable
     when it shouldn't be.  I am not an expert on linker scripts, so this
     opinion is based on the fact that the other linker scripts on my system
     are not executable.

[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license. 
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[x]: Package requires other packages for directories it uses.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[-]: Package contains desktop file if it is a GUI application.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.

===== SHOULD items =====

Generic:
[!]: Fully versioned dependency in subpackages if applicable.

     No Requires: %{name}%{?_isa} = %{version}-%{release} in
     orangefs-server, orangefs-fuse
     Is this intended?

[!]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.

     Please write a short (few word) justification for the patches

[!]: Packages should try to preserve timestamps of original installed
     files.

     Documentation install is done without preserving timestamps

[!]: Description is descriptive.

     I'd love to see something a bit more detailed than a single line, if
     that's possible

[!]: Spec file doesn't contain unnecessary sections.

     There is a commented out %post script for the server that should be
     removed

[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[x]: SourceX tarball generation or download is documented.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: %check is present and all tests pass.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Rpmlint is run on debuginfo package(s).
[x]: Rpmlint is run on all installed packages.
[x]: Package should not use obsolete m4 macros
[x]: Spec file according to URL is the same as in SRPM.

(In reply to Jonathan Dieter from comment #22)
> Thanks so much for clearing out all of the important rpmlint issues (even if
> some of them were pretty trivial).  We're finally close enough to the end
> that I'm posting my full review, though there are still a few things that
> need to be sorted out.  Please note the problems both with MUST and SHOULD:
> 
> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> 
> 
> ===== MUST items =====
> 
> C/C++:
> [x]: Package does not contain kernel modules.
> [x]: Package contains no static executables.
> [x]: Header files in -devel subpackage, if present.
> [x]: ldconfig called in %post and %postun if required.
> [x]: Package does not contain any libtool archives (.la)
> [x]: Rpath absent or only used for internal libs.
> [x]: Development (unversioned) .so files in -devel subpackage, if present.
> 
> Generic:
> [!]: If (and only if) the source package includes the text of the
>      license(s) in its own file, then that file, containing the text of the
>      license(s) for the package is included in %license.
> 
>      COPYING is available in tarball but not installed as %license

Installed.

> [!]: License file installed when any subpackage combination is installed.
> 
>      Since COPYING represents the overall license of the collected work,
> please 
>      make sure COPYING is installed as %license if the server or fuse client
> is 
>      installed (I'm not sure if either are supposed to Require: orangefs)

They are and do now.

> [!]: Package must own all directories that it creates.
> 
>      Directories without known owners:
>      /usr/share/doc/orangefs/design, /usr/share/doc/orangefs,
>      /usr/share/doc/orangefs/coding, /usr/share/doc/orangefs/random
>      You do need to own these documentation directories 

Done.

> [!]: Rpmlint is run on all rpms the build produces.
> 
>      Rpmlint was run, and all major warnings have been fixed, but I'm still
>      uncomfortable with the script-without-shebang error we're seeing with
> the
>      linker script.  I believe this is because the linker script is
> executable
>      when it shouldn't be.  I am not an expert on linker scripts, so this
>      opinion is based on the fact that the other linker scripts on my system
>      are not executable.

That'll make only-non-binary-in-usr-lib show up, but that's probably
preferable.

> [x]: Package is licensed with an open-source compatible license and meets
>      other legal requirements as defined in the legal section of Packaging
>      Guidelines.
> [x]: License field in the package spec file matches the actual license. 
> [x]: If the package is under multiple licenses, the licensing breakdown
>      must be documented in the spec.
> [x]: Package requires other packages for directories it uses.
> [x]: %build honors applicable compiler flags or justifies otherwise.
> [x]: Package contains no bundled libraries without FPC exception.
> [x]: Changelog in prescribed format.
> [x]: Sources contain only permissible code or content.
> [x]: Development files must be in a -devel package
> [x]: Package uses nothing in %doc for runtime.
> [x]: Package consistently uses macros (instead of hard-coded directory
>      names).
> [x]: Package is named according to the Package Naming Guidelines.
> [x]: Package does not generate any conflict.
> [x]: Package obeys FHS, except libexecdir and /usr/target.
> [x]: Requires correct, justified where necessary.
> [x]: Spec file is legible and written in American English.
> [x]: Package contains systemd file(s) if in need.
> [x]: Useful -debuginfo package or justification otherwise.
> [x]: Package is not known to require an ExcludeArch tag.
> [x]: Package complies to the Packaging Guidelines
> [x]: Package successfully compiles and builds into binary rpms on at least
>      one supported primary architecture.
> [x]: Package installs properly.
> [x]: Package does not own files or directories owned by other packages.
> [x]: All build dependencies are listed in BuildRequires, except for any
>      that are listed in the exceptions section of Packaging Guidelines.
> [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
> [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
> [x]: %config files are marked noreplace or the reason is justified.
> [x]: Macros in Summary, %description expandable at SRPM build time.
> [x]: Dist tag is present.
> [x]: Package does not contain duplicates in %files.
> [x]: Permissions on files are set properly.
> [x]: Package use %makeinstall only when make install DESTDIR=... doesn't
>      work.
> [x]: Package is named using only allowed ASCII characters.
> [x]: No %config files under /usr.
> [x]: Package does not use a name that already exists.
> [x]: Package is not relocatable.
> [x]: Sources used to build the package match the upstream source, as
>      provided in the spec URL.
> [x]: Spec file name must match the spec package %{name}, in the format
>      %{name}.spec.
> [x]: File names are valid UTF-8.
> [x]: Packages must not store files under /srv, /opt or /usr/local
> [-]: Package contains desktop file if it is a GUI application.
> [-]: If the package is a rename of another package, proper Obsoletes and
>      Provides are present.
> 
> ===== SHOULD items =====
> 
> Generic:
> [!]: Fully versioned dependency in subpackages if applicable.
> 
>      No Requires: %{name}%{?_isa} = %{version}-%{release} in
>      orangefs-server, orangefs-fuse
>      Is this intended?

Fixed.

> [!]: Patches link to upstream bugs/comments/lists or are otherwise
>      justified.
> 
>      Please write a short (few word) justification for the patches

Okay.  One of them has been submitted but not yet committed upstream.

> [!]: Packages should try to preserve timestamps of original installed
>      files.
> 
>      Documentation install is done without preserving timestamps

I've changed cp to install -p -m 644.

> [!]: Description is descriptive.
> 
>      I'd love to see something a bit more detailed than a single line, if
>      that's possible

I put some more in.  I can write more if you don't think it's enough.

> [!]: Spec file doesn't contain unnecessary sections.
> 
>      There is a commented out %post script for the server that should be
>      removed

Right.

> [x]: Uses parallel make %{?_smp_mflags} macro.
> [x]: Final provides and requires are sane (see attachments).
> [x]: Package functions as described.
> [x]: Latest version is packaged.
> [x]: Package does not include license text files separate from upstream.
> [x]: Scriptlets must be sane, if used.
> [x]: SourceX tarball generation or download is documented.
> [x]: Package should compile and build into binary rpms on all supported
>      architectures.
> [x]: Reviewer should test that the package builds in mock.
> [x]: Buildroot is not present
> [x]: Package has no %clean section with rm -rf %{buildroot} (or
>      $RPM_BUILD_ROOT)
> [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
> [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
> [x]: SourceX is a working URL.
> [x]: Spec use %global instead of %define unless justified.
> [-]: If the source package does not include license text(s) as a separate
>      file from upstream, the packager SHOULD query upstream to include it.
> [-]: Description and summary sections in the package spec file contains
>      translations for supported Non-English languages, if available.
> [-]: %check is present and all tests pass.
> 
> ===== EXTRA items =====
> 
> Generic:
> [x]: Large data in /usr/share should live in a noarch subpackage if package
>      is arched.
> [x]: Rpmlint is run on debuginfo package(s).
> [x]: Rpmlint is run on all installed packages.
> [x]: Package should not use obsolete m4 macros
> [x]: Spec file according to URL is the same as in SRPM.

Then I intend to make the 2.9.7 release if this is accepted.  I think
the only changes I want to make upstream is the genconfig-path patch and 
these remaining man pages.

Git: https://github.com/omnibond/orangefs-fedora
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=22506890
Spec: http://dev.orangefs.org/2017/marbran/1017/orangefs.spec
SRPM: http://dev.orangefs.org/2017/marbran/1017/orangefs-2.9.6-0.6.20171011svn.fc26.src.rpm

Everything looks pretty good to me, so this review is APPROVED!  There is one small formatting change I'd like you to make, though, before you actually import it into Fedora:

Currently there's a newline at the beginning of the description and no blank line between the shared description and the subpackage-specific descriptions.  It would look better if there was no newline at the beginning and a blank line between the shared description and the subpackage-specific descriptions.

Current output:
Description :

OrangeFS (formerly PVFS2) is a high-performance parallel network file
system designed for use on high performance computing systems.  It
provides very high performance access to disk storage for parallel
applications.  It is accessible through a variety of interfaces,
including the native OrangeFS library, the kernel, FUSE, and MPI-IO.
This package contains the headers and libraries necessary for client
development.

Preferred output:
Description :
OrangeFS (formerly PVFS2) is a high-performance parallel network file
system designed for use on high performance computing systems.  It
provides very high performance access to disk storage for parallel
applications.  It is accessible through a variety of interfaces,
including the native OrangeFS library, the kernel, FUSE, and MPI-IO.

This package contains the headers and libraries necessary for client
development.

I have sponsored Martin Brandenburg into the Packager group.

Martin, welcome to the Fedora packagers team! You may now do the fedrepo-req requests, per the process documentation[1].

[1]: https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/orangefs

I've created the repository and successfully built the package in
rawhide.  I've got the pre-release I've been working on here in rawhide;
I've started the release process and will update soon.

Accordingly I'm closing this.  Thanks for everybody's help.