Bug 834747

Sorry, forgot to re-upload the files after realizing that I had forgotten a dependency..
Please download again if you downloaded it before this message was sent.

I'll review it (and I'll sponsor you).

Clearing FE-NEEDSPONSOR - I've just sponsored Julian Leyh.

First of all I really don't like the package's name. Initially I thought it's about Global Positioning Systems and probably others could be mistaken as well.

Could you please rename it? For example into gnat-programming-studio (to be honest, I really don't know much about a typical Ada workflow and how you are (Ada developers) usually refer to the tools you're using so keep in mind this while taking my advices).

Otherwise the package looks quite good (as it should be assuming your package experience so far) except some minor things (which I am not fully aware of, so I'm going to cast the reat of Fedora Ada SIG into this ticket)  and here is my formal


REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

- rpmlint is NOT silent. Except bogus messages aout spelling mistakes, could you please explain the rest? I'm especially concerned about rpath,, executable-stack and zero-length files.

sulaco ~: rpmlint ~/Desktop/gps-*
gps.src: W: spelling-error %description -l en_US customizable -> customization
gps.src: W: spelling-error %description -l en_US customizations -> customization, customization's, customization s
gps.x86_64: W: spelling-error %description -l en_US customizable -> customization
gps.x86_64: W: spelling-error %description -l en_US customizations -> customization, customization's, customization s
gps.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/gps ['/usr/lib/gcc/x86_64-redhat-linux/4.7.0/adalib/', '/usr/lib64', '/usr/lib', '$ORIGIN/../../templates_parser/.build/native/release/relocatable/lib/', '/usr/lib64/xmlada/relocatable/', '$ORIGIN/../../gnatlib/src/lib/gtk/relocatable/', '$ORIGIN/../../gnatlib/src/lib/python/relocatable/', '$ORIGIN/../../gnatlib/src/lib/gnatcoll/relocatable/', '/usr/lib64/']
gps.x86_64: W: executable-stack /usr/bin/gps
gps.x86_64: W: no-documentation
gps.x86_64: E: zero-length /usr/share/gps/examples/tutorial/projects/prj1/src1.adb
gps.x86_64: W: devel-file-in-non-devel-package /usr/share/gps/examples/language/language_custom.h
gps.x86_64: W: devel-file-in-non-devel-package /usr/share/gps/examples/demo/matrix_handling/matrix_utils.c
gps.x86_64: E: non-executable-script /usr/share/gps/examples/remote/my_ssh 0644L /bin/sh
gps.x86_64: E: zero-length /usr/share/gps/examples/demo/projects/prj4/src4.adb
gps.x86_64: E: zero-length /usr/share/gps/examples/demo/projects/prj3/src3.adb
gps.x86_64: W: devel-file-in-non-devel-package /usr/share/gps/examples/language/gprcustom.c
gps.x86_64: E: zero-length /usr/share/gps/examples/tutorial/projects/prj4/src4.adb
gps.x86_64: E: zero-length /usr/share/gps/examples/demo/projects/prj1/src1.adb
gps.x86_64: E: incorrect-fsf-address /usr/share/gps/templates/aws_web_server_blocks/js/aws_kernel.tjs
gps.x86_64: E: zero-length /usr/share/gps/examples/tutorial/projects/prj3/src3.adb
gps.x86_64: E: zero-length /usr/share/gps/examples/demo/projects/prj2/src2.adb
gps.x86_64: W: devel-file-in-non-devel-package /usr/share/gps/examples/demo/matrix_handling/matrix.c
gps.x86_64: W: devel-file-in-non-devel-package /usr/share/gps/examples/demo/matrix_handling/matrix.h
gps.x86_64: E: zero-length /usr/share/gps/examples/tutorial/projects/prj2/src2.adb
gps.x86_64: W: no-manual-page-for-binary gps
4 packages and 0 specfiles checked; 11 errors, 12 warnings.
sulaco ~: 

+/- The package is named according to the  Package Naming Guidelines. However I've made some advices to the packager. See above.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license (GPLv2 or later as stated in the sources).


 -The file, containing the text of the license(s) for the package, is included in %doc so it MUST be marked as %doc in the %files section. Please do.

+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

sulaco ~/rpmbuild/SOURCES: sha256sum gps-5.0.1-gpl-src.tgz*
008a04eac17088144d25532bdad933b80c8198ac40b3aa67d8e3df4268f0b9ae  gps-5.0.1-gpl-src.tgz
008a04eac17088144d25532bdad933b80c8198ac40b3aa67d8e3df4268f0b9ae  gps-5.0.1-gpl-src.tgz.1
sulaco ~/rpmbuild/SOURCES:

+ The package successfully compiles and builds into binary rpms on at least one primary architecture.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files in some of the dynamic linker's default paths.
+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
0 The package DOESN'T have a %clean section, so it won't build cleanly on systems with old rpm (EL-4 and EL-5, not sure about EL-6). Beware.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No C/C++ header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files without a suffix (e.g. libfoo.so).
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.

- The package includes a %{name}.desktop file, and this file does not installed with desktop-file-install in the %install section so please use desktop-file-validate for that.

+ The package does not own files or directories already owned by other packages.
0 At the beginning of %install, the package  does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) so it won't build cleanly on systems with old rpm (EL-4 and EL-5, not sure about EL-6). Beware.
+ All filenames in rpm packages are valid UTF-8.

So here is a TODO list:

* Lets discuss naming scheme.
* Explain some rpmlint messages.
* Fedora Ada SIG members, could you please take a look at the build log:

http://kojipkgs.fedoraproject.org//work/tasks/9270/4189270/build.log

Are these gcc arguments are valid Fedora gcc arguments for Ada or not? In case we do have them.

* Add licensing info to the main package.
* Run desktop-file-validate within the %install section.

Added several new email addresses to the watchlist.

Bjorn, Pavel, maybe it's time to create a dedicated mail-list (and create a virtal Bugzilla account for tracking Ada-related tickets) for Fedora Ada SIG?

How would a virtual Bugzilla account work? Would bugs filed against a package be assigned to that account instead of the maintainer of the package? Would I be expected to log in to that account instead of my own?

(In reply to comment #4)
> Could you please rename it? For example into gnat-programming-studio (to be
> honest, I really don't know much about a typical Ada workflow and how you
> are (Ada developers) usually refer to the tools you're using so keep in mind
> this while taking my advices).

Most other Linux distributions name it "gnat-gps". I called it "gps", since there is no other package with that name yet, and i could use %name in the spec file. I would be okay with gnat-gps or even gnat-programming-studio, but would prefer the shorter one.
Would this mean renaming the /usr/share/gps directory, too?

> - rpmlint is NOT silent. Except bogus messages aout spelling mistakes, could
> you please explain the rest? I'm especially concerned about rpath,,
> executable-stack and zero-length files.

> gps.src: W: spelling-error %description -l en_US customizable ->
> customization
> gps.src: W: spelling-error %description -l en_US customizations ->
> customization, customization's, customization s
> gps.x86_64: W: spelling-error %description -l en_US customizable ->
> customization
> gps.x86_64: W: spelling-error %description -l en_US customizations ->
> customization, customization's, customization s

Well, I took the description from the debian package..

> gps.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/gps
> ['/usr/lib/gcc/x86_64-redhat-linux/4.7.0/adalib/', '/usr/lib64', '/usr/lib',
> '$ORIGIN/../../templates_parser/.build/native/release/relocatable/lib/',
> '/usr/lib64/xmlada/relocatable/',
> '$ORIGIN/../../gnatlib/src/lib/gtk/relocatable/',
> '$ORIGIN/../../gnatlib/src/lib/python/relocatable/',
> '$ORIGIN/../../gnatlib/src/lib/gnatcoll/relocatable/', '/usr/lib64/']

This should be fixed with gprbuild bug number 834425.

> gps.x86_64: W: executable-stack /usr/bin/gps

I don't really know what this means, will read about it.

> gps.x86_64: E: zero-length
> /usr/share/gps/examples/tutorial/projects/prj1/src1.adb
> gps.x86_64: W: devel-file-in-non-devel-package
> /usr/share/gps/examples/language/language_custom.h
> gps.x86_64: W: devel-file-in-non-devel-package
> /usr/share/gps/examples/demo/matrix_handling/matrix_utils.c

These and the other zero-length files in the examples folder are part of the tutorials and examples. Should they be moved into a separate package?

> gps.x86_64: E: non-executable-script /usr/share/gps/examples/remote/my_ssh
> 0644L /bin/sh

Belongs to examples, too, maybe the install routine installed it with permissions of data files. Do example scripts have to be executable?

> gps.x86_64: E: incorrect-fsf-address
> /usr/share/gps/templates/aws_web_server_blocks/js/aws_kernel.tjs

Should be easy to fix..

> gps.x86_64: W: no-manual-page-for-binary gps

GPS only supplies documentation in html, texinfo, pdf, and txt format - no manpage.

>  -The file, containing the text of the license(s) for the package, is
> included in %doc so it MUST be marked as %doc in the %files section. Please
> do.

Will add it.

> 0 The package DOESN'T have a %clean section, so it won't build cleanly on
> systems with old rpm (EL-4 and EL-5, not sure about EL-6). Beware.

Will add it.

> - The package includes a %{name}.desktop file, and this file does not
> installed with desktop-file-install in the %install section so please use
> desktop-file-validate for that.

I did use desktop-file-install, should I change this?

> 0 At the beginning of %install, the package  does not run rm -rf
> %{buildroot} (or $RPM_BUILD_ROOT) so it won't build cleanly on systems with
> old rpm (EL-4 and EL-5, not sure about EL-6). Beware.

I checked, it does rm -rf it..


Thanks for reviewing my package. I hope to do better in the future.

(In reply to comment #4)
> First of all I really don't like the package's name. Initially I thought
> it's about Global Positioning Systems and probably others could be mistaken
> as well.
> 
> Could you please rename it? For example into gnat-programming-studio (to be
> honest, I really don't know much about a typical Ada workflow and how you
> are (Ada developers) usually refer to the tools you're using so keep in mind
> this while taking my advices).

I believe users typically call it GPS. I see the potential for confusion, but I would like "yum install gps" to install this package. (Could that be achieved with "Provides: gps"?) At the very least it should turn up when someone does "yum search gps".

> - rpmlint is NOT silent. Except bogus messages aout spelling mistakes, could
> you please explain the rest? I'm especially concerned about rpath,,
> executable-stack and zero-length files.

Let's get the parameters right first and then see if there are any runpaths left.

As noted on https://fedoraproject.org/wiki/Packaging:Ada executable stack is expected because GNAT uses trampolines for pointers to nested functions.

> + The package does NOT bundle copies of system libraries.

There's something that looks like a copy of Gnatcoll in there. Gnatcoll is already packaged. Is it possible to get GPS to use that?

I also found a copy of Templates Parser. Templates Parser is also included in AWS, which is under review in bug 810676. Templates Parser was once distributed as a standalone library, but since it was taken over by Adacore it has only been distributed inside other projects as far as I can find out. None the less it still has its own build system and its own manual. I think Templates Parser needs to be broken out into a subpackage built from either the AWS or the GPS source package (but named templates-parser, not gps-templates-parser). I'm not sure which is best, but I think the risk of circular dependencies may be smaller if it's built from the GPS source.

Adacore bundle libraries a lot. There could be more that I haven't found.

> 0 The package DOESN'T have a %clean section, so it won't build cleanly on
> systems with old rpm (EL-4 and EL-5, not sure about EL-6). Beware.

> 0 At the beginning of %install, the package  does not run rm -rf
> %{buildroot} (or $RPM_BUILD_ROOT) so it won't build cleanly on systems with
> old rpm (EL-4 and EL-5, not sure about EL-6). Beware.

EL-6 doesn't need those. We don't have any other Ada packages in EL-4 or EL-5.

> Are these gcc arguments are valid Fedora gcc arguments for Ada or not? In
> case we do have them.

No, all the standard compiler and linker flags are missing. The spec file does define GNAT_OPTFLAGS but this doesn't seem to have the intended effect. Maybe try overriding the Make variable on the command line instead?

(In reply to comment #8)
> No, all the standard compiler and linker flags are missing. The spec file
> does define GNAT_OPTFLAGS but this doesn't seem to have the intended effect.
> Maybe try overriding the Make variable on the command line instead?

Okay, I fixed this by overriding the GPRBUILD_FLAGS Make variable on command line.

About templates_parser and gnatcoll, templates_parser is statically linked into the binary, but gnatcoll seems to be dynamically linked. That's why I added it as a dependency. Reading the build output, it seems the bundled version is still built. I will try to get rid of it.

(In reply to comment #7)
> Would this mean renaming the /usr/share/gps directory, too?

Just so we're all aware of the implications: If /usr/share/gps is renamed, then we'll have to patch the makefiles of various libraries that want to install files in /usr/share/gps/plug-ins. I checked GTKada, XMLada and Gnatcoll, and the directory name "gps" is hard-coded in all of them.

I'm sure we need to keep internal files hierarchy intact (e.g. preserve /usr/share/gps ) but I still believe that we should rename in to gnat-gps. I did a quick googling and found that this app is called gnat-gps in Debian and Ubuntu. So I think it's better not to diverge too much from other distributions.

Also "Provides: gps  = %{version}-%{release}" allows you to install it as "sudo yum install gps".

I noticed that GPS brings its own version of AUnit, too. Will try to separate it.

Any progress?

The original URLs are also broken now.  Does anyone have a copy of the spec or src.rpm around?  I might have time to take the next steps, as well as update to gnat-gps 5.1.1 .

You can use this one: 
http://vgai.de/fedora/2012-06-23/gps-5.0.1-1.fc17.src.rpm

But I'm going to write new gpr files (please see AWS rpm for example) because original gprs contain a lot of crosslinks to bundled libraries, hard-coded  compilation flags etc.

(sorry a mistake)

Dead review