https://bugzilla.redhat.com/show_bug.cgi?id=1359412 Andrew J. Schorr <aschorr@xxxxxxxxxxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aschorr@telemetry-investmen | |ts.com Flags|needinfo? | --- Comment #4 from Andrew J. Schorr <aschorr@xxxxxxxxxxxxxxxxxxxxxxxxx> --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
