Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1359412 - Review Request: gawkextlib - library providing support functions for gawk extension libraries
Summary: Review Request: gawkextlib - library providing support functions for gawk ext...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: David Kaspar [Dee'Kej]
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-07-23 21:04 UTC by Andrew Schorr
Modified: 2017-11-26 20:58 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-11-26 20:58:48 UTC
dkaspar: fedora-review+


Attachments (Terms of Use)
gawextlib.spec (deleted)
2017-05-22 14:12 UTC, David Kaspar [Dee'Kej]
no flags Details
gawkextlib.spec (deleted)
2017-05-22 15:21 UTC, Andrew J. Schorr
no flags Details

Description Andrew Schorr 2016-07-23 21:04:15 UTC
Spec URL: http://sourceforge.net/projects/gawkextlib/files/rpms/gawkextlib.spec
SRPM URL: http://sourceforge.net/projects/gawkextlib/files/rpms/gawkextlib-1.0.2-1.fc24.src.rpm
Description: gawkextlib is a library providing helpful support functions for gawk extension libraries.
Fedora Account System Username: ajschorr

This is my first package, and I am seeking a sponsor. I am the upstream maintainer of the gawkextlib project. This project provides a number of extension libraries for gawk, including XML, postgresql, lmdb, and select I/O multiplexing.

The koji build is here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=14994986

Comment 1 Andrew Schorr 2016-07-30 16:05:33 UTC
FYI, I updated the spec file to make this minor change:

@@ -5,7 +5,7 @@ Release:        1%{?dist}
 License:       GPLv3+
 Group:         Development/Libraries
 URL:           http://sourceforge.net/projects/gawkextlib
-Source0:       http://sourceforge.net/projects/gawkextlib/files/%{name}-%{version}.tar.gz
+Source0:       %{url}/files/%{name}-%{version}.tar.gz
 BuildRequires: /usr/include/gawkapi.h
 Requires:      gawk

Comment 2 David Kaspar [Dee'Kej] 2016-09-02 13:11:50 UTC
So, I will lay down some questions/notes regarding the package, as they will come during the review:

0) Is this project already packaged for some other distributions? Which ones? Could you please provide the links those packages?

1) Is there any specific reason, why the project is named gawkextlib? I see your second review request for gawk-xml. If I should follow the Fedora packaging naming guidelines, I would prefer that this (current) package would be named gawk-extlib, so there will be uniformity between other gawk packages as well...

2) Please, remove the TABs in the specfile, so the indentation is same for everyone. Use whitespaces instead.

3) Try to keep the 'Summary' more simple, like for example:
>  Extension libraries for gawk
... Current summarry seems to me to vague, because using word library 2 times in a simple sentence is kind of confusing.

4) The Group tag is no longer necessary, and it shouldn't be in new specfiles. For more info, please, see:
https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

5) Unless you have multiple sources, it's less confusing to use 'Source' tag instead of 'Source0'.

6) (In reply to Andrew Schorr from comment #1)
> FYI, I updated the spec file to make this minor change:
> 
> @@ -5,7 +5,7 @@ Release:        1%{?dist}
>  License:       GPLv3+
>  Group:         Development/Libraries
>  URL:           http://sourceforge.net/projects/gawkextlib
> -Source0:      
> http://sourceforge.net/projects/gawkextlib/files/%{name}-%{version}.tar.gz
> +Source0:       %{url}/files/%{name}-%{version}.tar.gz
>  BuildRequires: /usr/include/gawkapi.h
>  Requires:      gawk

Actually, I think this change make the Source URL shorter, but for new maintainers in the future, it might be a little bit obfuscating. My personal preference is to keep the full URL for the source tarball, with only %{name} and %{version} in it. However, this is just my opinion, the final decision about this will be on you... :)

7) I see you have used the /usr/include/gawkapi.h as the BuildRequires. Again, this is too much obfuscated, because other people would have to find out from where this header file comes from (is it from gawk or gawkextlib?). Since this is normally part of the gawk package (according to 'dnf whatprovides <path/file>), you should just use simple form of:
> BuildRequires:  gawk

UPDATE: Looking at the gawk package itself, I will most likely split it into base package and devel supbackage. The /usr/include/gawkapi.h will be located inside the gawk-devel subpackage.

8) %description - it is generally more save to use the name of 'gawkextlib' in the description. According to Fedora Package Guidelines (FPG), there might be instances where the %{name} might not expand properly, which is not correct.

And I wouldn't be affraid to use more specific description, like the one you have on the sourceforge webpage:

"The gawkextlib project provides several extension libraries for gawk (GNU AWK), as well as libgawkextlib containing some APIs that are useful for building gawk extension libraries."

In that case, it's the same for people comparing if the source code @ sourceforg is the same as the source code packaged in Fedora. :)

9) There's no need to use '-n %{name}-devel' in the %package section. rpmbuild should be clever enough these days to put the dash (-) there automatically. So you can just use:
> %package devel

Update: This applies in case if you want to have the %{name} followed by dash and the suffix. It can be useful in case you need something like this:
> %package -n lib-%{name}-devel
In this way, you can override the default behaviour of rpmbuild.

10) The 'Requires' tag for /usr/include/gawkapi.h in devel supbackage is not necessary, because the gawkextlib (base package) already requires gawk, and the devel subpackage is depending on its base package.

11) Similar to point 9), you can just use "%description devel".

12) Please, if not necessary for some significant reason, try *not* use %setup -q anymore. rpmbuild is able to %autosetup, which will automatically apply all the listed patches (PatchN), if they are correctly formatted. It makes maintaing of packages in the future much simple. More info you can find here:
https://fedoraproject.org/wiki/Packaging:Guidelines#Applying_patches
and here:
http://www.rpm.org/wiki/PackagerDocs/Autosetup

13) The 'rm -rf %{buildroot}' is no longer necessary for building packages for Fedora. It is beening cleaned up automatically.

14) %makeinstall macro is "forbidden". Use some other macros instead:
https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

15) You're not calling the ldconfig for (un)installation of devel subpackages, which is not correct:
https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries

So, basically, you're missing something like this:
> %post devel -p /sbin/ldconfig
> %postun devel -p /sbin/ldconfig

16) In %files section - %defattr is not needed anymore:
https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

17) Please, consider if your package should use hardened build:
https://fedoraproject.org/wiki/Packaging:Guidelines#PIE

You know it better than I do.

18) Another thing I have noticed is that your devel supbackage does not use fully-qualified Requires tag. More info:
https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

19) Looking at your last changelog, I would suggest make it more descriptive in the future. In case it was a rebase to newer version (my guess), than it should be stated there explicitly. It helps in case someone is trying to find changes that caused some problems.

These are only notes regarding the specfile... I will look do the licensing check and other necessary steps soon. :)

One more thing, I know I require much from you right now, but I was lead to this with my experience with maintainership. I will most likely maintain these packages, if they ever get into RHEL, so I don't want to have more work than necessary in the future.

In case you're interested, here's example of one of the specfile I had cleaned up this year (tcsh):
http://pkgs.fedoraproject.org/cgit/rpms/tcsh.git/tree/tcsh.spec

It took me a lot of time to fix the mess which was in the specfile previously... :-/

Thank you for your understanding.

Comment 3 David Kaspar [Dee'Kej] 2016-09-05 10:02:37 UTC
Another set of notes about the package:
20) Since this package provides a library and other packages might be linking to it, is there a reason why this software is not packaged under some LGPL license? Legal stuff is generally problematic, this is just a question.

21) I see the m4/ folder inside the source package. Are you using automate (autoconf) for anything? Because if you do, you probably need additional BuildRequires for 'autoconf'...

23) Looking at the content of README file - it describes the use of libgawkextlib. This is suitable for the %description devel section. And the notes on how to build package from git sources are considered irrelevant. IOW:
* lets drop README file
* move the first section of it into %description devel
* ignore the info about building from git-sources

24) Do you have any documentation for the library, for example in some markdown format? If so, it would be good to add it into the sourceball, because after it we could transform the markdown into man page... :)

Feel free to reach to me, if you need any help. ;)

Comment 4 Andrew J. Schorr 2016-12-04 18:28:00 UTC
Hi, sorry for the long delay -- I have been extremely busy with regular work. I uploaded a new version of the spec file and my responses to your 24 comments are interspersed below:

> 0) Is this project already packaged for some other distributions? Which ones?
> Could you please provide the links those packages?

No. This will be the first distribution to include it.

> 1) Is there any specific reason, why the project is named gawkextlib? I see
> your second review request for gawk-xml. If I should follow the Fedora
> packaging naming guidelines, I would prefer that this (current) package would
> be named gawk-extlib, so there will be uniformity between other gawk packages
> as well...

I am open to other names, but please consider that comparing gawkextlib
to gawk-xml is like comparing apples to oranges. The gawkextlib package
provides a support library, not an actual gawk extenstion. The gawkextlib
shared library provides APIs that are used by various gawk extension
libraries such as gawk-xml, gawk-pgsql, etc. So it's really a different
beast. The gawkextlib rpm provides no end-user functionality. It provides
only a header file and shared library for use by actual gawk extension
libraries.

> 2) Please, remove the TABs in the specfile, so the indentation is same for
> everyone. Use whitespaces instead.

Done.

> 3) Try to keep the 'Summary' more simple, like for example:
> >  Extension libraries for gawk
> ... Current summarry seems to me to vague, because using word library 2 times
> in a simple sentence is kind of confusing.

Hmmm. "Extension libraries for gawk" is not accurate. This is a library that
provides APIs for use by gawk extension libraries. I'm not sure how else to say
it. Would it be more clear if "gawk extension libraries" were replaced by "gawk
extension modules"? I could also replace "infrastructure" if that's somehow
problematic. Here's a possible rewrite: "Library providing support functions
used by several gawk extension modules." Is that better than "Library providing
common infrastructure for gawk extension libraries"?

> 4) The Group tag is no longer necessary, and it shouldn't be in new specfiles.
> For more info, please, see:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

Thanks. I removed it.

> 5) Unless you have multiple sources, it's less confusing to use 'Source' tag
> instead of 'Source0'.

I changed "Source0" to "Source".

> 6) (In reply to Andrew Schorr from comment #1)
> > FYI, I updated the spec file to make this minor change:
> > 
> > @@ -5,7 +5,7 @@ Release:        1%{?dist}
> >  License:       GPLv3+
> >  Group:         Development/Libraries
> >  URL:           http://sourceforge.net/projects/gawkextlib
> > -Source0:      
> > http://sourceforge.net/projects/gawkextlib/files/%{name}-%{version}.tar.gz
> > +Source0:       %{url}/files/%{name}-%{version}.tar.gz
> >  BuildRequires: /usr/include/gawkapi.h
> >  Requires:      gawk
> 
> Actually, I think this change make the Source URL shorter, but for new
> maintainers in the future, it might be a little bit obfuscating. My personal
> preference is to keep the full URL for the source tarball, with only %{name}
> and %{version} in it. However, this is just my opinion, the final decision
> about this will be on you... :)

I made this change because I searched for other new packages currently in the
review process and noted that another maintainer had requested that this change
be made. I can't remember the name of that package at the moment. It seems that
this may be a matter of personal preference. I prefer to avoid repetition, so I
like using %{url} in the Source definition.

> 7) I see you have used the /usr/include/gawkapi.h as the BuildRequires. Again,
> this is too much obfuscated, because other people would have to find out from
> where this header file comes from (is it from gawk or gawkextlib?). Since this
> is normally part of the gawk package (according to 'dnf whatprovides
> <path/file>), you should just use simple form of:
> > BuildRequires:  gawk
> 
> UPDATE: Looking at the gawk package itself, I will most likely split it into
> base package and devel supbackage. The /usr/include/gawkapi.h will be located
> inside the gawk-devel subpackage.

I am changing this to say "BuildRequires: gawk-devel".

> 8) %description - it is generally more save to use the name of 'gawkextlib' in
> the description. According to Fedora Package Guidelines (FPG), there might be
> instances where the %{name} might not expand properly, which is not correct.

I don't see where that's mentioned. Particularly since you seem to dislike the
name "gawkextlib", it seems much safer to use %{name} where possible in case it
changes in the future.  Why might it not expand properly?

> And I wouldn't be afraid to use more specific description, like the one you
> have on the sourceforge webpage:
> 
> "The gawkextlib project provides several extension libraries for gawk (GNU
> AWK), as well as libgawkextlib containing some APIs that are useful for
> building gawk extension libraries."
> 
> In that case, it's the same for people comparing if the source code @
> sourceforg is the same as the source code packaged in Fedora. :)

Sorry -- I know this is confusing, but we have made the mistake of using
gawkextlib in 2 different ways. This particular package's purpose is
to provide libgawkextlib, an API library used by various gawk extension
libraries. In addition, the "gawkextlib" project is a collection of gawk
extension libraries. It's a naming mess. But the bottom line is that the
description from the web site refers to the collection of gawk modules
and is not appropriate for this rpm.

> 9) There's no need to use '-n %{name}-devel' in the %package section. rpmbuild
> should be clever enough these days to put the dash (-) there automatically. So
> you can just use:
> > %package devel
> 
> Update: This applies in case if you want to have the %{name} followed by dash
> and the suffix. It can be useful in case you need something like this:
> > %package -n lib-%{name}-devel
> In this way, you can override the default behaviour of rpmbuild.

Thanks. I made that change.

> 10) The 'Requires' tag for /usr/include/gawkapi.h in devel supbackage is not
> necessary, because the gawkextlib (base package) already requires gawk, and the
> devel subpackage is depending on its base package.

Hmmm. I'm confused about this one. The gawkextlib base package requires gawk,
but BUILDING the gawkextlib package requires gawk-devel. On the other hand,
INSTALLING gawkextlib-devel requires gawk-devel (which is not required for
installing gawkextlib). So I don't see why the Requires: gawk-devel is
superflous for the devel package. What am I misunderstanding? Perhaps you
wrote this before realizing that the header file belongs in a separate
gawk-devel subpackage...

> 11) Similar to point 9), you can just use "%description devel".

Done. Thanks.

> 12) Please, if not necessary for some significant reason, try *not* use %setup
> -q anymore. rpmbuild is able to %autosetup, which will automatically apply all
> the listed patches (PatchN), if they are correctly formatted. It makes
> maintaing of packages in the future much simple. More info you can find here:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Applying_patches
> and here:
> http://www.rpm.org/wiki/PackagerDocs/Autosetup

I changed it to say '%autosetup'.

> 13) The 'rm -rf %{buildroot}' is no longer necessary for building packages for
> Fedora. It is beening cleaned up automatically.

Removed.

> 14) %makeinstall macro is "forbidden". Use some other macros instead:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

I replaced it with '%make_install'.

> 15) You're not calling the ldconfig for (un)installation of devel subpackages,
> which is not correct:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries
> 
> So, basically, you're missing something like this:
> > %post devel -p /sbin/ldconfig
> > %postun devel -p /sbin/ldconfig

I'm confused about that one. The gawkextlib-devel package installs only
the symlink /usr/lib64/libgawkextlib.so. The guidelines say:

"In addition, every binary RPM package which contains shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun."

What does the "not just symlinks" parenthetical comment mean? I interpret
that to indicate that the call to ldconfig is not required. At runtime,
the link that ld.so must resolve is libgawkextlib.so.0, not libgawkextlib.so.
So why do we need to call ldconfig for just the symlink that's used when
compiling/linking the program during the build?

I'm happy to add those scriptlets, but I don't understand why it's useful
to call ldconfig for the development symlink. Maybe I'm just being stupid.
I note that zlib-devel does not call ldconfig:

[schorr@ajs-x1 ~]$ uname -a
Linux ajs-x1 4.6.7-300.fc24.x86_64 #1 SMP Wed Aug 17 18:48:43 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
[schorr@ajs-x1 ~]$ rpm -ql zlib-devel | fgrep so
/usr/lib64/libz.so
[schorr@ajs-x1 ~]$ rpm --scripts -q zlib-devel
[schorr@ajs-x1 ~]$ 

Is zlib-devel package incorrectly? I don't see it in gmp-devel either.

> 16) In %files section - %defattr is not needed anymore:
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

Done.

> 17) Please, consider if your package should use hardened build:
> https://fedoraproject.org/wiki/Packaging:Guidelines#PIE
> 
> You know it better than I do.

Isn't this currently the default behavior in Fedora? Please take a look at
   https://fedoraproject.org/wiki/Changes/Harden_All_Packages
In my f24 /usr/lib/rpm/redhat/macros, I see this:
   # Harden packages by default for Fedora 23:
   # https://fedorahosted.org/fesco/ticket/1384 (accepted on 2014-02-11)
   %_hardened_build        1
Also, since this is a shared library, -fPIC should be used to compile anyway,
so this won't make much difference unless linking with "-z now" breaks
something... But I guess it could be useful if included in RHEL where I suppose
this may not be the default. For now, I have added it.

> 18) Another thing I have noticed is that your devel supbackage does not use
> fully-qualified Requires tag. More info:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

I changed it to say 'Requires: %{name}%{?_isa} = %{version}-%{release}'.

> 19) Looking at your last changelog, I would suggest make it more descriptive in
> the future. In case it was a rebase to newer version (my guess), than it should
> be stated there explicitly. It helps in case someone is trying to find changes
> that caused some problems.

While in heavy development, I have not bothered to update the changelog
carefully. Once we get to a working version, I will add thorough comments
about subsequent changes. But it is changing too much right now for this
to be useful...

> 20) Since this package provides a library and other packages might be linking
> to it, is there a reason why this software is not packaged under some LGPL
> license? Legal stuff is generally problematic, this is just a question.

The gawk documentation requires that all extensions use the GPL license.
The gawk docs say:

   "Every dynamic extension must be distributed under a license that is
   compatible with the GNU GPL (*note Copying::)."

I'm not sure if that includes LGPL or not. It seems safe to me to use GPL
since gawk is GPL. I'm not a licensing expert, but I don't see why LGPL
would make more sense in this context. These libraries are explicitly
for use with gawk, which is under the GPL, and the extension libraries
bundled with gawk are also under the GPL.

> 21) I see the m4/ folder inside the source package. Are you using automate
> (autoconf) for anything? Because if you do, you probably need additional
> BuildRequires for 'autoconf'...

The tarball uses standard './configure && make && make check && make install'.
The autoconf stuff is required for developers, but it's not
needed after the tarball has been built. This is the same as for gawk.
If gawk.spec doesn't need it, then neither do we.

> 23) Looking at the content of README file - it describes the use of
> libgawkextlib. This is suitable for the %description devel section. And the
> notes on how to build package from git sources are considered irrelevant. IOW:
> * lets drop README file

OK. I removed '%doc README'.

> * move the first section of it into %description devel

I don't think it adds much to what's already there.

> * ignore the info about building from git-sources

Agreed. The file has been dropped.

> 24) Do you have any documentation for the library, for example in some markdown
> format? If so, it would be good to add it into the sourceball, because after it
> we could transform the markdown into man page... :)

No, I don't have any additional documentation. This is intended for gawk
extension library developers, not end users, so I must confess that the
documentation is not great. There are extensive comments in the gawkextlib.h
header file. That should be adequate for a developer trying to use this,
particularly when combined with examples from other projects.

Thanks,
Andy

Comment 5 David Kaspar [Dee'Kej] 2017-05-22 14:05:19 UTC
Hello Andrew,

(In reply to Andrew J. Schorr from comment #4)
> Hi, sorry for the long delay -- I have been extremely busy with regular
> work.
Don't worry about it. As you can see, I have been also completely engaged with my regular work... :-/ (I've become upstream maintainer/developer for initscripts.)


> > 0) Is this project already packaged for some other distributions? Which ones?
> > Could you please provide the links those packages?
> 
> No. This will be the first distribution to include it.
In that case, I thank you for chosing Fedora as your first choice. I'm sorry for the delay in my response, and I hope you will like our Fedora community at some point! ;)

> I am open to other names, but please consider that comparing gawkextlib
> to gawk-xml is like comparing apples to oranges. The gawkextlib package
> provides a support library, not an actual gawk extenstion. The gawkextlib
> shared library provides APIs that are used by various gawk extension
> libraries such as gawk-xml, gawk-pgsql, etc. So it's really a different
> beast. The gawkextlib rpm provides no end-user functionality. It provides
> only a header file and shared library for use by actual gawk extension
> libraries.

Yeah, I'm aware that those are not same. :) I was just thinking to come up with some naming schema to avoid some possible issues in the future. It's really pain in the a** to be forced to create a pacake with new name just to solve some problems. ;)

So, currently we have:
* gawk [the main package]
* gawk-devel [subpackage]
* gawk-doc [subpackage]
* gawk-debuginfo [automatically created subpackage]

In case we will have the 'gawkextlib' package (to follow FPG rule to have the name as close to upstream name as possible), then I wouls suggest this naming schema:
* gawkextlib [the main package, which is necessary for your other extensions]
* gawkextlib-devel [subpackage necessary for developers]
* gawkextlib-doc   [subpackage containing documentation, if any exists]
* gawkextlib-xml   [the module/extension used with gawkextlib]
* gawkextlib-pgsql [the module/extension used with gawkextlib]
* gawkextlib-mpfr  [the module/extension used with gawkextlib]

This way it would be clearly visible by the name that the modules/extensions are part of the the bigger "collection", and that they cannot function without the main (base) package.

I was even thinking that you could create these modules/extensions as subpackages of the gawkextlib, but I see that you versioning is not united across these packages. The maintenance for this approach might be simpler, but updating a module would require rebuild of whole thing.

If you do not want to create this "collection" as subpackages, you can just create each package separate as it is, which will look the same as above, but will be versioned & build separately. You will have more specfiles to maintain, but IMHO this might be the best course of action. :)

In case your modules/extensions are able to function *without* the 'gawkextlib', then I would follow this naming scheme:
* gawkextlib [optional library for gawk]
* gawkextlib-devel [subpackage necessary for developers]
* gawkextlib-doc   [subpackage containing documentation, if any exists]
* gawk-xml         [independent extension to gawkextlib used for gawk itself]
* gawk-pgsql       [independent extension to gawkextlib used for gawk itself]
* gawk-mpfr        [independent extension to gawkextlib used for gawk itself]

> > 3) Try to keep the 'Summary' more simple, like for example:
> > >  Extension libraries for gawk
> > ... Current summarry seems to me to vague, because using word library 2 times
> > in a simple sentence is kind of confusing.
> 
> Hmmm. "Extension libraries for gawk" is not accurate. This is a library that
> provides APIs for use by gawk extension libraries. I'm not sure how else to
> say
> it. Would it be more clear if "gawk extension libraries" were replaced by
> "gawk
> extension modules"? I could also replace "infrastructure" if that's somehow
> problematic. Here's a possible rewrite: "Library providing support functions
> used by several gawk extension modules." Is that better than "Library
> providing
> common infrastructure for gawk extension libraries"?

Yeah, I think
> Library providing common infrastructure for gawk extension modules
is OK. ;)

> > 6) (In reply to Andrew Schorr from comment #1)
> > > FYI, I updated the spec file to make this minor change:
> > > 
> > > @@ -5,7 +5,7 @@ Release:        1%{?dist}
> > >  License:       GPLv3+
> > >  Group:         Development/Libraries
> > >  URL:           http://sourceforge.net/projects/gawkextlib
> > > -Source0:      
> > > http://sourceforge.net/projects/gawkextlib/files/%{name}-%{version}.tar.gz
> > > +Source0:       %{url}/files/%{name}-%{version}.tar.gz
> > >  BuildRequires: /usr/include/gawkapi.h
> > >  Requires:      gawk
> > 
> > Actually, I think this change make the Source URL shorter, but for new
> > maintainers in the future, it might be a little bit obfuscating. My personal
> > preference is to keep the full URL for the source tarball, with only %{name}
> > and %{version} in it. However, this is just my opinion, the final decision
> > about this will be on you... :)
> 
> I made this change because I searched for other new packages currently in the
> review process and noted that another maintainer had requested that this
> change
> be made. I can't remember the name of that package at the moment. It seems
> that
> this may be a matter of personal preference. I prefer to avoid repetition,
> so I
> like using %{url} in the Source definition.

Noted, thank you for stating that. It's OK to keep it like this. ;)

> > 7) I see you have used the /usr/include/gawkapi.h as the BuildRequires. Again,
> > this is too much obfuscated, because other people would have to find out from
> > where this header file comes from (is it from gawk or gawkextlib?). Since this
> > is normally part of the gawk package (according to 'dnf whatprovides
> > <path/file>), you should just use simple form of:
> > > BuildRequires:  gawk
> > 
> > UPDATE: Looking at the gawk package itself, I will most likely split it into
> > base package and devel supbackage. The /usr/include/gawkapi.h will be located
> > inside the gawk-devel subpackage.
> 
> I am changing this to say "BuildRequires: gawk-devel".

Thank you for this, 'gawk-devel' already exists. :)

> > 8) %description - it is generally more save to use the name of 'gawkextlib' in
> > the description. According to Fedora Package Guidelines (FPG), there might be
> > instances where the %{name} might not expand properly, which is not correct.
> 
> I don't see where that's mentioned. Particularly since you seem to dislike
> the
> name "gawkextlib", it seems much safer to use %{name} where possible in case
> it
> changes in the future.  Why might it not expand properly?

To be honest, I really don't remember where exactly did I find this in FPG. This is nothing serious, so lets keep this. In case this proves being problematic in the future, we can do the change if it happens. Right now we should focus on getting your package into Fedora as soon as possible. :)

> > And I wouldn't be afraid to use more specific description, like the one you
> > have on the sourceforge webpage:
> > 
> > "The gawkextlib project provides several extension libraries for gawk (GNU
> > AWK), as well as libgawkextlib containing some APIs that are useful for
> > building gawk extension libraries."
> > 
> > In that case, it's the same for people comparing if the source code @
> > sourceforg is the same as the source code packaged in Fedora. :)
> 
> Sorry -- I know this is confusing, but we have made the mistake of using
> gawkextlib in 2 different ways. This particular package's purpose is
> to provide libgawkextlib, an API library used by various gawk extension
> libraries. In addition, the "gawkextlib" project is a collection of gawk
> extension libraries. It's a naming mess. But the bottom line is that the
> description from the web site refers to the collection of gawk modules
> and is not appropriate for this rpm.
Ah, OK. In that case I would just slightly rewrite the current %description to match the *Summary*, maybe like this? :)

"%{name} is a library providing common support infrastructure for gawk extensions. This particular package provides the 'libgawkextlib', which is used by various gawk extension modules -- for example gawk-xml, gawk-pgsql, and more."

> > 10) The 'Requires' tag for /usr/include/gawkapi.h in devel supbackage is not
> > necessary, because the gawkextlib (base package) already requires gawk, and the
> > devel subpackage is depending on its base package.
> 
> Hmmm. I'm confused about this one. The gawkextlib base package requires gawk,
> but BUILDING the gawkextlib package requires gawk-devel. On the other hand,
> INSTALLING gawkextlib-devel requires gawk-devel (which is not required for
> installing gawkextlib). So I don't see why the Requires: gawk-devel is
> superflous for the devel package. What am I misunderstanding? Perhaps you
> wrote this before realizing that the header file belongs in a separate
> gawk-devel subpackage...

I don't know what the h*ll I wrote there... :D Seriously, that sentence from me does not make any sense. I'm not surprised you were confused... :D Looking at the current specfile, it seems OK. ;)

> 
> > 11) Similar to point 9), you can just use "%description devel".
> 
> Done. Thanks.

Here's my proposal of updated %description for devel subpackage, to unite it with the %description proposed above:

"The %{name}-devel package contains the header files and libraries
needed to develop gawk extension modules that use %{name} facilities."

> > 15) You're not calling the ldconfig for (un)installation of devel subpackages,
> > which is not correct:
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries
> > 
> > So, basically, you're missing something like this:
> > > %post devel -p /sbin/ldconfig
> > > %postun devel -p /sbin/ldconfig
> 
> I'm confused about that one. The gawkextlib-devel package installs only
> the symlink /usr/lib64/libgawkextlib.so.

Ah, that explains everything. :) I haven't tried compiling the source code myself, so I didn't realize it was just a symlink and header file. Initially I thought it was another shared library. ;) This part can stay as it is.

> > 17) Please, consider if your package should use hardened build:
> > https://fedoraproject.org/wiki/Packaging:Guidelines#PIE
> > 
> > You know it better than I do.
> 
> Isn't this currently the default behavior in Fedora? Please take a look at
>    https://fedoraproject.org/wiki/Changes/Harden_All_Packages
Yes, you are right. It is by default enabled in current Fedora releases. However, there might come a point where we might like to get this package in RHEL or EPEL. In that case AFAIK the package should be build with preconfigured environment for RHEL/EPEL, which will most likely not have the hardened build enabled by default.

So I personally add the hardened build anywhere, just to be safe. :)

> > 19) Looking at your last changelog, I would suggest make it more descriptive in
> > the future. In case it was a rebase to newer version (my guess), than it should
> > be stated there explicitly. It helps in case someone is trying to find changes
> > that caused some problems.
> 
> While in heavy development, I have not bothered to update the changelog
> carefully. Once we get to a working version, I will add thorough comments
> about subsequent changes. But it is changing too much right now for this
> to be useful...

OK, noted. :) 

> > 20) Since this package provides a library and other packages might be linking
> > to it, is there a reason why this software is not packaged under some LGPL
> > license? Legal stuff is generally problematic, this is just a question.
> 
> The gawk documentation requires that all extensions use the GPL license.
> The gawk docs say:
> 
>    "Every dynamic extension must be distributed under a license that is
>    compatible with the GNU GPL (*note Copying::)."
> 
> I'm not sure if that includes LGPL or not. It seems safe to me to use GPL
> since gawk is GPL. I'm not a licensing expert, but I don't see why LGPL
> would make more sense in this context. These libraries are explicitly
> for use with gawk, which is under the GPL, and the extension libraries
> bundled with gawk are also under the GPL.

I'm not a legal expert myself, but your clarification makes sense. :) And changing licenses for FOSS projects can really be very painful, I wouldn't advise that much. That's why it was just a question I was intrigued by. ;)

> 
> > 21) I see the m4/ folder inside the source package. Are you using automate
> > (autoconf) for anything? Because if you do, you probably need additional
> > BuildRequires for 'autoconf'...
> 
> The tarball uses standard './configure && make && make check && make
> install'.
> The autoconf stuff is required for developers, but it's not
> needed after the tarball has been built. This is the same as for gawk.
> If gawk.spec doesn't need it, then neither do we.

Yes, the 'gawk' does not require the 'autoconf' anymore. Thank you for clarification. :)

> > 24) Do you have any documentation for the library, for example in some markdown
> > format? If so, it would be good to add it into the sourceball, because after it
> > we could transform the markdown into man page... :)
> 
> No, I don't have any additional documentation. This is intended for gawk
> extension library developers, not end users, so I must confess that the
> documentation is not great. There are extensive comments in the gawkextlib.h
> header file. That should be adequate for a developer trying to use this,
> particularly when combined with examples from other projects.

Thanks for the info, and all the changes you have made to the spec to confirm to FPG rules. I really like it. :)

The main (only?) question right now remains on how do you decide to proceed with the naming of packages (see above).

I will be looking for hearing from you! ;)

Dee'Kej

Comment 6 David Kaspar [Dee'Kej] 2017-05-22 14:12:55 UTC
Created attachment 1281075 [details]
gawextlib.spec

Here's the specfile with the changes I've proposed, and slightly adjusted indentation (nothing significant), in case you want to use it. :)

Comment 7 Andrew J. Schorr 2017-05-22 15:18:50 UTC
Hi!

(In reply to David Kaspar [Dee'Kej] from comment #5)
> Yeah, I'm aware that those are not same. :) I was just thinking to come up
> with some naming schema to avoid some possible issues in the future. It's
> really pain in the a** to be forced to create a pacake with new name just to
> solve some problems. ;)

Understood. I agree that it's important to get this right.

> So, currently we have:
> * gawk [the main package]
> * gawk-devel [subpackage]
> * gawk-doc [subpackage]
> * gawk-debuginfo [automatically created subpackage]
> 
> In case we will have the 'gawkextlib' package (to follow FPG rule to have
> the name as close to upstream name as possible), then I wouls suggest this
> naming schema:
> * gawkextlib [the main package, which is necessary for your other extensions]
> * gawkextlib-devel [subpackage necessary for developers]
> * gawkextlib-doc   [subpackage containing documentation, if any exists]
> * gawkextlib-xml   [the module/extension used with gawkextlib]
> * gawkextlib-pgsql [the module/extension used with gawkextlib]
> * gawkextlib-mpfr  [the module/extension used with gawkextlib]
> 
> This way it would be clearly visible by the name that the modules/extensions
> are part of the the bigger "collection", and that they cannot function
> without the main (base) package.

I understand where you're coming from, but not all of the modules in the
gawkextlib collection require the use of the gawkextlib support library.
Some modules use it, and others do not. We would like these to be thought
of as gawk extension modules, not gawkextlib extension modules. Conceptually,
we're aiming for something like CPAN. Those are perl modules, not CPAN modules,
as I understand it.

> I was even thinking that you could create these modules/extensions as
> subpackages of the gawkextlib, but I see that you versioning is not united
> across these packages. The maintenance for this approach might be simpler,
> but updating a module would require rebuild of whole thing.

I definitely do not want to do that. Again, the conceptual model here is
CPAN. We want each module to be maintained independently. IMHO, it would not
be a good idea to package them together.

> If you do not want to create this "collection" as subpackages, you can just
> create each package separate as it is, which will look the same as above,
> but will be versioned & build separately. You will have more specfiles to
> maintain, but IMHO this might be the best course of action. :)

I agree that they must be maintained separately, with a separate spec file
for each one. The gawkextlib project attempts to provide a template that will
make this easier.

> In case your modules/extensions are able to function *without* the
> 'gawkextlib', then I would follow this naming scheme:

(As I mentioned above, some of the modules are able to function without
gawkextlib, and some require it. That decision is made separately by each
developer depending on whether he wants to use the support functions in the
gawkextlib library.)

> * gawkextlib [optional library for gawk]
> * gawkextlib-devel [subpackage necessary for developers]
> * gawkextlib-doc   [subpackage containing documentation, if any exists]
> * gawk-xml         [independent extension to gawkextlib used for gawk itself]
> * gawk-pgsql       [independent extension to gawkextlib used for gawk itself]
> * gawk-mpfr        [independent extension to gawkextlib used for gawk itself]

That is my preferred naming convention, and I think is consistent with the
approach we have taken so far.

> Ah, OK. In that case I would just slightly rewrite the current %description
> to match the *Summary*, maybe like this? :)
> 
> "%{name} is a library providing common support infrastructure for gawk
> extensions. This particular package provides the 'libgawkextlib', which is
> used by various gawk extension modules -- for example gawk-xml, gawk-pgsql,
> and more."

I updated the description to the following:

%{name} is a library providing common support infrastructure for gawk
extensions. This package provides 'libgawkextlib', which is used by various
gawk extension modules -- for example gawk-xml, gawk-pgsql, and more.

Is that OK?

> I don't know what the h*ll I wrote there... :D Seriously, that sentence from
> me does not make any sense. I'm not surprised you were confused... :D
> Looking at the current specfile, it seems OK. ;)

:-)

> "The %{name}-devel package contains the header files and libraries
> needed to develop gawk extension modules that use %{name} facilities."

I made that change.

> Yes, you are right. It is by default enabled in current Fedora releases.
> However, there might come a point where we might like to get this package in
> RHEL or EPEL. In that case AFAIK the package should be build with
> preconfigured environment for RHEL/EPEL, which will most likely not have the
> hardened build enabled by default.
> 
> So I personally add the hardened build anywhere, just to be safe. :)

Makes sense. It's in there.

> I'm not a legal expert myself, but your clarification makes sense. :) And
> changing licenses for FOSS projects can really be very painful, I wouldn't
> advise that much. That's why it was just a question I was intrigued by. ;)

Fair enough. Let's stick with GPL.

> Thanks for the info, and all the changes you have made to the spec to
> confirm to FPG rules. I really like it. :)

Good. I'm glad we're getting close. :-)

> The main (only?) question right now remains on how do you decide to proceed
> with the naming of packages (see above).

Please see my thoughts above. I think we have the correct naming convention.
I will run it by Arnold to confirm that he agrees.

Thanks,
Andy

Comment 8 Andrew J. Schorr 2017-05-22 15:20:48 UTC
(In reply to David Kaspar [Dee'Kej] from comment #6)
> Created attachment 1281075 [details]
> gawextlib.spec
> 
> Here's the specfile with the changes I've proposed, and slightly adjusted
> indentation (nothing significant), in case you want to use it. :)

I incorporated most of the changes, but I'm a bit confused by the %description section. You have in your attachment:

%description
%{name} is a library providing helpful support functions for gawk extension
libraries.

%{name} is a library providing common support infrastructure for gawk
extensions. This particular package provides the 'libgawkextlib', which is used
by various gawk extension modules -- for example gawk-xml, gawk-pgsql, and more.

Do you really intend to have 2 separate paragraphs like that? It seems 
repetitive. I'm guessing that you intended to delete the first sentence. I
am attaching an updated version with this description:

%description
%{name} is a library providing common support infrastructure for gawk
extensions. This package provides 'libgawkextlib', which is used by various
gawk extension modules -- for example gawk-xml, gawk-pgsql, and more.


Does that look good?

Thanks,
Andy

Comment 9 Andrew J. Schorr 2017-05-22 15:21:43 UTC
Created attachment 1281118 [details]
gawkextlib.spec

Here is my latest version. It should be the same as yours except for the description. How does this look?

Thanks,
Andy

Comment 10 David Kaspar [Dee'Kej] 2017-05-22 15:52:40 UTC
(In reply to Andrew J. Schorr from comment #8)
> (In reply to David Kaspar [Dee'Kej] from comment #6)
> > Created attachment 1281075 [details]
> > gawextlib.spec
> > 
> > Here's the specfile with the changes I've proposed, and slightly adjusted
> > indentation (nothing significant), in case you want to use it. :)
> 
> I incorporated most of the changes, but I'm a bit confused by the
> %description section.
> 
> ...
>
> Do you really intend to have 2 separate paragraphs like that? It seems 
> repetitive. I'm guessing that you intended to delete the first sentence.

Yes, you're right. I guess I forgot to press the 'dd' in Vim. :)

Regarding the naming scheme, I see now the details completely, so I think we can proceed now. ;)

I'm giving this the fedora-review+, and I check the FPG to what else needs to be done from my side tomorrow (I need to leave office soon).

Best regards,

Dee'Kej

Comment 11 Andrew J. Schorr 2017-05-23 14:11:35 UTC
Thanks. I updated the files in https://sourceforge.net/projects/gawkextlib/files/rpms/

Actually, I updated the gawkextlib.spec file, and I uploaded a new version of the source rpm built now on Fedora 25 (gawkextlib-1.0.2-1.fc25.src.rpm), since I no longer use F24.

Regards,
Andy

Comment 12 David Kaspar [Dee'Kej] 2017-05-24 11:23:53 UTC
Sorry for the delay, I was struggling to run fedora-review tool because of BZ #1350930. Anyway, I made it to work, so here are the rest of necessary formalities...

rpmlint result (specfile):
==========================
> Checking: gawkextlib-debuginfo-1.0.2-1.fc27.x86_64.rpm
> 1 packages and 0 specfiles checked; 0 errors, 0 warnings.

rmplint result (*.rpm):
=======================
> gawkextlib.x86_64: W: spelling-error %description -l en_US xml -> XML, ml, x ml
->> The %description is mentioning the exact package 'gawk-xml' [OK]

> gawkextlib.x86_64: W: spelling-error %description -l en_US pgsql -> SQL
->> The %description is mentioning the exact package 'gawk-pgsql' [OK]

> gawkextlib.x86_64: W: shared-lib-calls-exit /usr/lib64/libgawkextlib.so.0.0.0 exit@GLIBC_2.2.5
->> Shared library calls exit(3) - this is something that should be either fixed or explained if this is safe to do so. [WARNING]

> gawkextlib.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
> gawkextlib.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
->> This is "feature" of fedora-rawhide builds. [OK]

> 1 packages and 0 specfiles checked; 0 errors, 5 warnings.

rpmlint result (*-devel.rpm):
=============================
> gawkextlib-devel.x86_64: W: only-non-binary-in-usr-lib
->> The header file is in correct location. The other file is symlink, which is non-binary, but this is correct. [OK]

> gawkextlib-devel.x86_64: W: no-documentation
->> Andrew already mentioned the lack of documentation in upstream ATM, but the header file itself should be commented sufficiently. [OK]

> 1 packages and 0 specfiles checked; 0 errors, 2 warnings.

===========================================================
fedora-review results:
===========================================================
Issues:
=======
- Sources used to build the package match the upstream source, as provided
  in the spec URL.
  Note: Upstream MD5sum check error, diff is in
  /home/dkaspar/Downloads/reviews/review-gawkextlib/diff.txt
  See: http://fedoraproject.org/wiki/Packaging/SourceURL

>> Andrew most likely modified the package again after I donwloaded it and ran the fedora-review on it. I've checked the *.src.rpm and its sane. [OK]


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

C/C++:
[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:
[ ]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "MIT/X11 (BSD like)", "GPL", "FSF All Permissive", "Unknown or
     generated". 31 files have unknown license.
> The whole project is licensed under GPLv3+. Some files use FSF copyright note stating that re-licensing to GPL is allowed, and therefore used.

[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[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]: 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]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[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

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

Generic:
[ ]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
     gawkextlib-debuginfo
> We are not genereating debuginfo manually, mock does it now automatically. This is most likely bug either of mock or fedora-review package. [OK]

[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]: Uses parallel make %{?_smp_mflags} macro.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.

Requires
--------
gawkextlib-devel (rpmlib, GLIBC filtered):
    gawk-devel
    gawkextlib(x86-64)
    libgawkextlib.so.0()(64bit)

gawkextlib-debuginfo (rpmlib, GLIBC filtered):

gawkextlib (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    gawk
    libc.so.6()(64bit)
    rtld(GNU_HASH)



Provides
--------
gawkextlib-devel:
    gawkextlib-devel
    gawkextlib-devel(x86-64)

gawkextlib-debuginfo:
    gawkextlib-debuginfo
    gawkextlib-debuginfo(x86-64)

gawkextlib:
    gawkextlib
    gawkextlib(x86-64)
    libgawkextlib.so.0()(64bit)



Source checksums
----------------
http://sourceforge.net/projects/gawkextlib/files/gawkextlib-1.0.2.tar.gz :
  CHECKSUM(SHA256) this package     : 1194f9f76b0db9b0d27146f349efb57d09d63771c9bddc71640b81c9515d9678
  CHECKSUM(SHA256) upstream package : 271ea0d473fbbbbc921db65cbc38e74e3bde42a095a38dbac0207e199dfda705
diff -r also reports differences

> Already mentioned the reason above.


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -n gawkextlib
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 13 David Kaspar [Dee'Kej] 2017-05-24 11:46:34 UTC
Okay, so for some reason the SHA256 hashes still don't match for the package I have downloaded and the package fedore-review has downloaded.

However, doing manual check on both of these sources, I get the same result:
271ea0d473fbbbbc921db65cbc38e74e3bde42a095a38dbac0207e199dfda705  gawkextlib-1.0.2.tar.gz

This looks like some issue of fedora-review package.

Comment 14 Andrew J. Schorr 2017-05-24 14:03:42 UTC
I guess this is probably due to changes in the spec file over time. I just uploaded a new version of:

https://sourceforge.net/projects/gawkextlib/files/gawkextlib-1.0.2.tar.gz

I believe that this should match the tarball inside of:

https://sourceforge.net/projects/gawkextlib/files/rpms/gawkextlib-1.0.2-1.fc25.src.rpm

I hope that helps.

Comment 15 Kamil Dudka 2017-05-24 14:04:59 UTC
(In reply to David Kaspar [Dee'Kej] from comment #13)
> Okay, so for some reason the SHA256 hashes still don't match for the package
> I have downloaded and the package fedore-review has downloaded.
> 
> However, doing manual check on both of these sources, I get the same result:
> 271ea0d473fbbbbc921db65cbc38e74e3bde42a095a38dbac0207e199dfda705 
> gawkextlib-1.0.2.tar.gz
> 
> This looks like some issue of fedora-review package.

It can be caused by the fact that the source URL points to a multi-level redirection to the actual URL containing the data:

$ curl -svo/dev/null http://sourceforge.net/projects/gawkextlib/files/gawkextlib-1.0.2.tar.gz -L 2>&1 | grep Location
< Location: https://sourceforge.net/projects/gawkextlib/files/gawkextlib-1.0.2.tar.gz
< Location: https://sourceforge.net/projects/gawkextlib/files/gawkextlib-1.0.2.tar.gz/download
< Location: https://downloads.sourceforge.net/project/gawkextlib/gawkextlib-1.0.2.tar.gz?r=&ts=1495634539&use_mirror=master
< Location: https://master.dl.sourceforge.net/project/gawkextlib/gawkextlib-1.0.2.tar.gz

I am not sure if fedora-review follows these redirects while checking hashes.  Anyway, it is a good practice to use https:// source URL where possible.

Comment 16 David Kaspar [Dee'Kej] 2017-05-24 14:11:02 UTC
(In reply to Kamil Dudka from comment #15)
> (In reply to David Kaspar [Dee'Kej] from comment #13)
> > Okay, so for some reason the SHA256 hashes still don't match for the package
> > I have downloaded and the package fedore-review has downloaded.
> > 
> > However, doing manual check on both of these sources, I get the same result:
> > 271ea0d473fbbbbc921db65cbc38e74e3bde42a095a38dbac0207e199dfda705 
> > gawkextlib-1.0.2.tar.gz
> > 
> > This looks like some issue of fedora-review package.
> 
> It can be caused by the fact that the source URL points to a multi-level
> redirection to the actual URL containing the data:
> 
> $ curl -svo/dev/null
> http://sourceforge.net/projects/gawkextlib/files/gawkextlib-1.0.2.tar.gz -L
> 2>&1 | grep Location
> < Location:
> https://sourceforge.net/projects/gawkextlib/files/gawkextlib-1.0.2.tar.gz
> < Location:
> https://sourceforge.net/projects/gawkextlib/files/gawkextlib-1.0.2.tar.gz/
> download
> < Location:
> https://downloads.sourceforge.net/project/gawkextlib/gawkextlib-1.0.2.tar.
> gz?r=&ts=1495634539&use_mirror=master
> < Location:
> https://master.dl.sourceforge.net/project/gawkextlib/gawkextlib-1.0.2.tar.gz

Hmm, but still that should contain the same package, right? Looks like, according to Andrew, it was just outdated... :)

> I am not sure if fedora-review follows these redirects while checking
> hashes.  Anyway, it is a good practice to use https:// source URL where
> possible.

Yes, I agree. We should make the use of https:// where possible. :)

Comment 17 Andrew J. Schorr 2017-05-24 15:27:34 UTC
I updated the URL in the spec file to say https instead of http. I uploaded a new tarball, spec file, and source rpm.

Thanks,
Andy

Comment 18 Andrew Schorr 2017-09-03 21:05:31 UTC
Hi David,

Just as I thought we were all set to go, I realized that there is a subtle versioning issue here. We are soon going to release gawk version 4.2, and that versions bumps the major API version from 1 to 2. When the major API version changes, that signals an incompatibility with previous versions. If gawk 4.2 tries to load a gawk-xml extension that was compiled in a gawk 4.1 build environment, the program will issue an error message about the version mismatch, and then it will exit.

I'm not quite sure how to address this. The current gawk-xml 1.0.6 version I just realized should build successfully against both the current gawk 4.1 releases and the new 4.2 release coming soon. But if it's built against 4.1 and then gawk is upgraded to 4.2, it will stop working. It doesn't seem as if the standard "Requires:" and "BuildRequires:" tags capture the subtlety of this situation.

It's not quite accurate to say "Requires: gawk = 4.1", since this package could be built against 4.2. But lacking a better solution, is it safest to change the Requires tags to say "gawk = 4.1"? Or is there a better way to address this situation?

I suppose we might want to patch the gawk.spec file to add "Provides:" tags for the gawk major and minor API versions:
Provides: gawk-api-major = 1
Provides: gawk-api-minor = 1
But is there then some way to specify that the gawk extension libraries will require versions calculated at build time?

In any case, I think it's probably a good idea to add the gawk-api versions to the gawk spec, since gawk versions may change more quickly than API versions.

Thanks,
Andy

Comment 19 David Kaspar [Dee'Kej] 2017-09-14 11:01:23 UTC
Hello Andy,

first to start - we need to take into account that rpmbuild is actually quite clever regarding the automatic detection of library requirements. If the gawk-xml will be build against gawk-4.1, it will require at least the gawk-4.1 version. (Taken that gawk is correctly versioning its shared libraries...)

In other words, if we make sure the correct version of gawk will be used during build of gawk-xml, we should be at least ok regarding the ABI compatibility. That can be achieved by using the Requires and/or BuildRequires correctly.

Regarding the API compatibility... AFAIK, we don't have any gawk-api-major package/binary file, right? I'm not sure that

> Provides: gawk-api-major = 1
> Provides: gawk-api-minor = 1

would work. I will discuss it with my colleague.

My suggested approach to this (if I understood the problem correctly) would be this:
1) Wait for gawk-4.2 release, rebase it in Rawhide and see if it would be possible to get it into F27 as well.
2) Make necessary changes in gawk-xml and gawkextlib specfiles as needed, for Rawhide (and F27 if needed).
3) Make sure we release the gawk-xml and gawkextlib after the gawk-4.2 lands in Rawhide (and F27 possibly).

The necessary changes in 2) that should be enough (IMHO):

* In gawkextlib:

Requires: gawk >= 4.2.0     # The exact version will be confirmed later

IIRC, the gawkextlib is required by any extensions for gawk. And because the gawkextlib will be build against correct gawk version, therefore the extension (like gawk-xml) should also be using correct version of gawk (because the dependency in this case is transitive).

Do you agree that this should be enough, or did I miss some important point that will needs to be addressed as well? :)

  -- Dee'Kej --

Comment 20 Andrew Schorr 2017-09-14 12:56:06 UTC
Hi Dee'Kej,

Maybe I am confused, but on my Fedora 26 laptop with gawkextlib and gawk-xml installed, I see this:

[schorr@ajs-t530 ~]$ rpm --requires -q gawkextlib
/sbin/ldconfig
/sbin/ldconfig
gawk
libc.so.6()(64bit)
libc.so.6(GLIBC_2.14)(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3.4)(64bit)
libc.so.6(GLIBC_2.4)(64bit)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1
rtld(GNU_HASH)
[schorr@ajs-t530 ~]$ rpm --requires -q gawk-xml
/bin/sh
/bin/sh
/bin/sh
expat
gawk
info
info
libc.so.6()(64bit)
libc.so.6(GLIBC_2.14)(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3.4)(64bit)
libc.so.6(GLIBC_2.4)(64bit)
libexpat.so.1()(64bit)
libgawkextlib.so.0()(64bit)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1
rtld(GNU_HASH)

So yes, I can modify the gawkextlib rpm spec file to require a specific version of gawk, but I don't think that's a great idea. The gawk version and the API version are two different things, and I think the gawk rpm should provide the API version that it actually supports. If gawkextlib requires gawk >= 4.2.0, that won't help us if gawk 4.3 actually changes the API. Then we would have breakage.

Also, as you can see, the gawk-xml rpm does not currently require any particular version of gawkextlib, so we are not transitively saved on that front. I think both gawkextlib and gawk-xml may need to require the correct gawk API version.

Am I confused, or do we need to add a Provides to the gawk rpm specifying the API version? And if so, how do we handle major/minor issues?

Thanks,
Andy

Comment 21 Andrew Schorr 2017-09-14 13:06:07 UTC
I should also note that not all gawk extension libraries will require gawkextlib. It is an optional support library. So we cannot rely upon solving the problem that way. I guess you are saying that the new gawk could say that it conflicts with a specific version of gawkextlib, but that gets messy if it also needs to Conflict with 10 other gawk extension libraries. I think the clean solution is to specify the API version. Am I missing something?

Regards,
Andy

Comment 22 Andrew Schorr 2017-10-06 15:48:08 UTC
Updated spec file here:
https://sourceforge.net/projects/gawkextlib/files/rpm-specs/gawkextlib.spec

Comment 23 David Kaspar [Dee'Kej] 2017-10-06 15:58:36 UTC
(In reply to Andrew Schorr from comment #22)
> Updated spec file here:
> https://sourceforge.net/projects/gawkextlib/files/rpm-specs/gawkextlib.spec

LGTM!

Comment 24 Gwyn Ciesla 2017-10-06 16:12:07 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/gawkextlib


Note You need to log in before you can comment on or make changes to this bug.