I'll review it.
No, you don't need FE-NEEDSPONSOR anymore - I sponsored you already.

(In reply to comment #0)
> how can i fix the error for debuginfo subpackage?

The debuginfo subpackage from Koji does have source files. I'm not sure what went wrong with yours, but I've seen similar problems before. Are there any symbolic links or bind mounts involved in the path to your rpmbuild directory?

> Another question: Should GMGPL be added to the accepted licenses? It
> basically is a GPLv2+ with linking exception. Or is "GPLv2+ with exceptions"
> good enough?

I think "GPLv2+ with exceptions" is good, but if you want a more authoritative answer you can ask on the legal mailing list, legal.org.

Here's what I have found in the package:

· Instead of pointing to the AWS page that doesn't describe Templates Parser, I suggest pointing to the documentation:
http://docs.adacore.com/aws-docs/templates_parser.html

· The tarball shrinks by 93% if you add --exclude-vcs to the tar command.

· Please build and package the documentation. Add "BuildRequires: texinfo-tex", and "make doc" under %build. (Don't use _smp_mflags here, because that causes docs/makefile to break.)

· Don't include the license field in subpackages unless it differs from the base package.

· _GNAT_project_dir must be used. Pass "I_GPR=%{_GNAT_project_dir} I_TGP=%{_GNAT_project_dir}/templates_parser" to make install.

· The link named %{_libdir}/lib%{name}.so.%{version} doesn't help with anything. Nothing will be looking for that filename because the library has no soname, so the library won't be found at run time unless a runpath is used. Here's a patch to add a soname:
http://lists.forge.open-do.org/pipermail/aws-patches/2012-June/000038.html

· ldconfig must be called. Add the following to the spec file:

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

· Wouldn't it be nice to also package templates2ada and templatespp? I suggest putting them in a subpackage named templates_parser-tools with the group field set to "Applications/Text".

See also these patches:
http://lists.forge.open-do.org/pipermail/aws-patches/2012-June/000036.html
http://lists.forge.open-do.org/pipermail/aws-patches/2012-June/000037.html

(In reply to comment #2)

Thanks for your nice review! Helps me a lot.

> The debuginfo subpackage from Koji does have source files. I'm not sure what
> went wrong with yours, but I've seen similar problems before. Are there any
> symbolic links or bind mounts involved in the path to your rpmbuild
> directory?

yeah.. I have put the rpmbuild directory on a different location and symlinked it from $HOME. I will try out without symlink.

> Here's what I have found in the package:
> 
> · Instead of pointing to the AWS page that doesn't describe Templates
> Parser, I suggest pointing to the documentation:
> http://docs.adacore.com/aws-docs/templates_parser.html

Okay.

> · The tarball shrinks by 93% if you add --exclude-vcs to the tar command.

I didn't know this option, thanks!

> · Please build and package the documentation. Add "BuildRequires:
> texinfo-tex", and "make doc" under %build. (Don't use _smp_mflags here,
> because that causes docs/makefile to break.)

Should the documentation go into a separate subpackage? %name-docs?

> · Don't include the license field in subpackages unless it differs from the
> base package.

Okay.

> · _GNAT_project_dir must be used. Pass "I_GPR=%{_GNAT_project_dir}
> I_TGP=%{_GNAT_project_dir}/templates_parser" to make install.

I left this out, because it was the same. But I do see the point, it might be somewhere else. Will change.

> · The link named %{_libdir}/lib%{name}.so.%{version} doesn't help with
> anything. Nothing will be looking for that filename because the library has
> no soname, so the library won't be found at run time unless a runpath is
> used. Here's a patch to add a soname:
> http://lists.forge.open-do.org/pipermail/aws-patches/2012-June/000038.html

Didn't really know if soname is necessary or not. In #fedora-devel nobody could give me a definite answer. About the patch you mention: if I understand correctly, it makes the library file "lib%{name}-%{version}.so", the version before the ".so". Should I change it like that?

> · ldconfig must be called. Add the following to the spec file:
> 
> %post -p /sbin/ldconfig
> 
> %postun -p /sbin/ldconfig

Yeah, forgot those.. This is my first library package, will add them.

> · Wouldn't it be nice to also package templates2ada and templatespp? I
> suggest putting them in a subpackage named templates_parser-tools with the
> group field set to "Applications/Text".
> 
> See also these patches:
> http://lists.forge.open-do.org/pipermail/aws-patches/2012-June/000036.html
> http://lists.forge.open-do.org/pipermail/aws-patches/2012-June/000037.html

Thanks, I will add the tools subpackage.

This library can be compiled for static linking. Should this be added, too?

If yes, xmlada should add the static libraries, too.

Sorry Peter if I stole your review. Since I had been working on Templates Parser I figured I should submit the notes I had.

(In reply to comment #3)
> Should the documentation go into a separate subpackage? %name-docs?

You may choose to make a subpackage if you consider the documentation to be "a lot" as the Packaging Guidelines say, or "large" as the Review Guidelines put it. (It's a bit funny that the Review Guidelines have it as a MUST item that large documentation must be in a subpackage, but leave it entirely to the packager to define "large".)
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

Note that there is a risk that developers who install templates_parser-devel won't notice that templates_parser-doc exists.

By the way, it may be a good idea to install the .info file in %{_infodir}, where the info command will presumably find it, but that's not something I'm familiar with.

> > · _GNAT_project_dir must be used. Pass "I_GPR=%{_GNAT_project_dir}
> > I_TGP=%{_GNAT_project_dir}/templates_parser" to make install.
> 
> I left this out, because it was the same. But I do see the point, it might
> be somewhere else. Will change.

It will be somewhere else in Fedora 18. So far /usr/lib/gnat has been the only place where Gnatmake would look for project files. In GCC 4.7 it also looks in /usr/share/gpr, which is a better place for architecture-independent files. GPRbuild also knows to look in /usr/share/gpr, so I changed _GNAT_project_dir in Rawhide.

> Didn't really know if soname is necessary or not. In #fedora-devel nobody
> could give me a definite answer.

The way I read the guidelines it's not a blocker if there is no soname: "When a shared library file is only provided in an unversioned format, the packager should ask upstream to consider providing a properly versioned library file. However, in such cases, if the shared library file is necessary for users to run programs linked against it, it must go into the base package." (https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages) That means that you can package the library without a soname, but then the unversioned name must be present in %{_libdir}, and not only in the devel subpackage. The problem with the current package is that the library is hidden in a subdirectory and the link in %{_libdir} doesn't have the name that the loader will look for unless the devel subpackage is installed.

If you add a soname, then the filename in %{_libdir} shall match the soname, and the unversioned link goes in the devel subpackage.

> About the patch you mention: if I
> understand correctly, it makes the library file "lib%{name}-%{version}.so",
> the version before the ".so". Should I change it like that?

I chose that format so that the soname will change in every release. Libgnat for example uses that format. If you put the version after ".so", then it will be assumed that minor releases are guaranteed to be ABI-compatible, and that the ABI changes only in major releases, so the soname will become "libtemplates_parser.so.11". The upstream developers make no such guarantees. (If they did, the way to communicate it would be to include such a soname.)

(In reply to comment #4)
> This library can be compiled for static linking. Should this be added, too?

Only if there is a compelling reason, which I don't think there is.
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

Oh, and the recommended suffix for a documentation subpackage is "-doc", without an S.

updated the package.

Spec file: http://vgai.de/fedora/2012-06-28/templates_parser.spec
SRPM: http://vgai.de/fedora/2012-06-28/templates_parser-11.6.0-2.fc17.src.rpm

koji build f17: http://koji.fedoraproject.org/koji/taskinfo?taskID=4207908
koji biuld f18: http://koji.fedoraproject.org/koji/taskinfo?taskID=4207903

rpmlint output:

]$ rpmlint RPMS/x86_64/templates_parser-* SRPMS/templates_parser-11.6.0-2.fc17.src.rpm 
templates_parser.x86_64: I: enchant-dictionary-not-found en_US
templates_parser.x86_64: W: executable-stack /usr/lib64/templates_parser/libtemplates_parser-11.6.0.so
templates_parser.x86_64: W: no-documentation
templates_parser-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/templates_parser-11.6.0/.build
templates_parser-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/templates_parser-11.6.0/.build
templates_parser-tools.x86_64: W: executable-stack /usr/bin/templates2ada
templates_parser-tools.x86_64: W: no-manual-page-for-binary templates2ada
templates_parser-tools.x86_64: W: no-manual-page-for-binary templatespp
templates_parser.src: W: invalid-url Source0: templates_parser-11.6.0.tar.xz
5 packages and 0 specfiles checked; 0 errors, 8 warnings.

This package looks OK to me, so I pass the ball to Peter.

Sorry, wrong URLs.. Here the correct ones:

Spec file: http://vgai.de/fedora/2012-06-29/templates_parser.spec
SRPM: http://vgai.de/fedora/2012-06-29/templates_parser-11.6.0-2.fc17.src.rpm

Peter says he doesn't mind if I take over this ticket, so here's my formal review:


Generic MUST Items:

· rpmlint must be run on the source rpm and all binary rpms the build produces.

  templates_parser.src: W: invalid-url Source0: templates_parser-11.6.0.tar.xz
  templates_parser-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/templates_parser-11.6.0/.build
  templates_parser-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/templates_parser-11.6.0/.build
  templates_parser-tools.x86_64: W: executable-stack /usr/bin/templates2ada
  templates_parser-tools.x86_64: W: no-manual-page-for-binary templates2ada
  templates_parser-tools.x86_64: W: no-manual-page-for-binary templatespp
  templates_parser.x86_64: W: executable-stack /usr/lib64/templates_parser/libtemplates_parser-11.6.0.so
  templates_parser.x86_64: W: no-documentation

  · There is no URL to the upstream source because it was taken from Git.
  · The hidden directory in the debuginfo package is odd, but not something a packager should be required to change. 
  · Executable stack is OK as noted in the Ada packaging guidelines.
  · There are no documentation files to include in the base package. (README contains only installation instructions.)

  About man pages, see the separate point below. None of the other warnings are blocking issues.

· The package must be named according to the Package Naming Guidelines.
  → OK.

· The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
  → OK. The names match.

· The package must meet the Packaging Guidelines.
  → OK.

· The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
  → OK. The license is GPLv2+ with exceptions (GMGPL) according to the source file headers.

· The License field in the package spec file must match the actual license.
  → OK.

· 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 must be included in %doc.
  → N/A. There is no separate license file.

· The spec file must be written in American English.
  → OK. The grammar isn't perfect but it's comprehensible.

· The spec file for the package MUST be legible.
  → OK.

· The sources used to build the package must match the upstream source, as provided in the spec URL.
  → OK. The contents of the tarball are identical to what I got from upstream Git.

· The package MUST successfully compile and build into binary rpms on at least one primary architecture.
  → OK. It builds in Koji on at least x86 and x86-64.

· If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
  → OK. GNAT_arches is used.

· All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines.
  → OK.

· The spec file MUST handle locales properly.
  → N/A. No translations are included.

· Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
  → OK. ldconfig is called.

· The package must NOT bundle copies of system libraries.
  → OK.

· If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
  → OK. The package isn't relocatable.

· The package must own all directories that it creates.
  → OK.

· The package must not list a file more than once in the spec file's %files listings.
  → OK.

· Permissions on files must be set properly.
  → OK.

· The package must consistently use macros.
  → OK.

· The package must contain code, or permissable content.
  → OK. Code.

· Large documentation files must go in a -doc subpackage.
  → OK. The documentation can reasonably be considered to not be large.

· If a package includes something as %doc, it must not affect the runtime of the application.
  → OK.

· Static libraries must be in a -static package.
  → N/A. Only shared libraries are packaged.

· Development files must be in a -devel package.
  → OK.

· In the vast majority of cases, devel packages must require the base package using a fully versioned dependency.
  → OK.

· Packages must NOT contain any .la libtool archives.
  → OK.

· Packages containing GUI applications must include a %{name}.desktop file.
  → N/A. This isn't a GUI application.

· Packages must not own files or directories already owned by other packages.
  → OK.

· All filenames in rpm packages must be valid UTF-8.
  → OK.


Ada-specific MUST items:

· The package must have "BuildRequires: gcc-gnat".
  → OK.

· The appropriate flags macro must be used for each GNAT tool that is invoked.
  → OK. Gnatmake_optflags is used.

· If there is a need to prevent attempts to build the package on secondary architectures where GNAT has not been bootstrapped, then this MUST be done with "ExclusiveArch: %{GNAT_arches}". 
  → OK. GNAT_arches is used.

· The package must have "BuildRequires: fedora-gnat-project-common".
  → OK.

· Ada library packages MUST have a -devel subpackage containing all the files that are necessary for compilation of code that uses the library.
  → OK.

· The -devel package MUST NOT contain any makefiles or other files that are only used for recompiling the library.
  → OK.

· The -devel package MUST NOT contain any *.o files. 
  → OK.

· The -devel package MUST contain one or more GNAT project files to be imported by other projects that use the library.
  → OK.

· Project files MUST be architecture-independent.
  → OK.

· Project files MUST NOT contain hard-coded directory names.
  → OK.

· If the "directories" project is used, then the -devel package MUST have an explicit "Requires: fedora-gnat-project-common".
  → OK.

· Project files MUST have an Externally_Built attribute equal to "true". 
  → OK.

· Ada source files in -devel packages MUST be placed in the %{_includedir} directory or a subdirectory thereof.
  → OK.

· Ada library information files MUST be placed in a subdirectory of %{_libdir}.
  → OK.

· GNAT projects files MUST be placed in the %{_GNAT_project_dir} directory or a subdirectory thereof. 
  → OK.


SHOULD items:

· If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
  → NOTE: There is no separate license file. This is not required by the GPL, but you should contact the developers and encourage them to add a license file.

· The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
  → No translations are included but that's OK.

· The reviewer should test that the package builds in mock.
  → OK.

· The package should compile and build into binary rpms on all supported architectures.
  → OK. It builds on all two primary architectures.

· The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
  → OK. The tools pass a smoke test, which implies that the library does too.

· If scriptlets are used, those scriptlets must be sane.
  → OK.

· Usually, subpackages other than devel should require the base package using a fully versioned dependency.
  → OK. The -tools subpackage has an automatic dependency on the soname.

· The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg.
  → N/A. There are no pkgconfig files.

· If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
  → OK. There are no file dependencies other than /sbin/ldconfig.

· The package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.
  → ISSUE: There are no man pages for templates2ada and templatespp. Debian has man pages however, so you should look at those to see if they can be used in Fedora. You should also try to get the man pages upstreamed.


To summarize: Add Debian's man pages to the -tools subpackage (unless you find some big problem with them). You should also ask the developers to add the man pages and the GPL version 2 (an updated version with the FSF's new address) to the upstream Git repository. (The best way is probably to send them Git patches. They told me it's OK to send patches for Templates Parser to the aws-patches mailing list.)

Hi Björn,

thanks for the review.

I updated the package, including the manpages from the debian packages.

URLS:
SRPM: http://vgai.de/fedora/2012-08-06/templates_parser-11.6.0-3.fc17.src.rpm
SPEC: http://vgai.de/fedora/2012-08-06/templates_parser.spec

Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4363764

rpmlint:
$ rpmlint RPMS/x86_64/templates_parser-*11.6.0-3*.rpm SRPMS/templates_parser-11.6.0-3.fc17.src.rpm 
templates_parser.x86_64: W: executable-stack /usr/lib64/templates_parser/libtemplates_parser-11.6.0.so
templates_parser.x86_64: W: no-documentation
templates_parser-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/templates_parser-11.6.0/.build
templates_parser-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/templates_parser-11.6.0/.build
templates_parser-tools.x86_64: W: executable-stack /usr/bin/templates2ada
templates_parser.src: W: invalid-url Source0: templates_parser-11.6.0.tar.xz
5 packages and 0 specfiles checked; 0 errors, 6 warnings.

Does this look okay now?

This looks good. This package is APPROVED.

If you're quick with your SCM request you may be able to get it processed before Fedora 18 is branched tomorrow. Otherwise you'll need to request an f18 branch in the SCM request.

If you want an f16 or el6 branch, then remember to hard-code the value of ExclusiveArch in those branches, as the macro GNAT_arches doesn't exist there.

New Package SCM Request
=======================
Package Name: templates_parser
Short Description: AWS templates engine
Owners: oenone
Branches: f17
InitialCC:

Git done (by process-git-requests).

templates_parser-11.6.0-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/templates_parser-11.6.0-3.fc17

templates_parser-11.6.0-3.fc17 has been pushed to the Fedora 17 testing repository.

Please built templates_parser in rawhide. 
Thanks

(In reply to comment #17)
> Please built templates_parser in rawhide. 
> Thanks

Sorry, must have missed that. Will do this evening.

Should be built now, unless I understand it wrongly:

http://koji.fedoraproject.org/koji/packageinfo?packageID=14460

Yes, it's OK

templates_parser-11.6.0-3.fc17 has been pushed to the Fedora 17 stable repository.