Bug 509158 (gnat-project-common)

If the upstream sources are Fedora specific, it may be nice, if you can ask on fedorahosted.org for creating a project in which the sources may be hosted.

here are some point of my pre-review:

Good:
+ Basename of the SPEC file matches with package name
+ Package doesn't contains subpackages
+ Local build works fine
+ Package has no %doc stanza
+ BuildRoot will be cleaned at the beginning of %clean and %install

Bad:
- Because the package should be fedora specific, it should be named as
  fedora-gnat-...
- Source tag contains not a fully qualified URI. Submitter should create
  a project on fedorahosted.org
- Could not check packaged tar ball agains upstream.
- Package doesn#t contains a URL tag
- License tag say 'Copyright only' this is not a valid OSS license
- Package contains not a verbatin copy of the license text
- Pleace use %{_sysconfdir} instead of /etc in the %files stanza
- you should use a version like 0.1 instead of 1, because I'm assume
  your are the upstream and this is the first release of the software
- Package has no proper BuildRoot definition

If you have any question, you may contact me via email

(In reply to comment #2)

Thanks for your comments, Jochen. I have fixed some of your points but I have questions on some others:

> - Because the package should be fedora specific, it should be named as
>   fedora-gnat-...

Fixed (but it looks like I can't update the title of this review request).

> - Source tag contains not a fully qualified URI. Submitter should create
>   a project on fedorahosted.org

Done.

> - Could not check packaged tar ball agains upstream.
> - Package doesn#t contains a URL tag

Tarballs will be at https://fedorahosted.org/released/fedora-gnat-project-common/.

> - License tag say 'Copyright only' this is not a valid OSS license

"Copyright only" is listed under Good Licenses at https://fedoraproject.org/wiki/Licensing:Main. Please explain why you think it isn't valid.

These files are so small that I don't care much about the license. I'd place them in the public domain, but as I understand it I can't do that under Swedish copyright law, so I figured I'd require copyright attribution only and otherwise let anyone do anything with them.

> - Package contains not a verbatin copy of the license text

common.gpr contains the complete license text:
-- Copyright 2009 B. Persson, Bjorn
-- No restrictions are placed on this code.

> - Pleace use %{_sysconfdir} instead of /etc in the %files stanza

Fixed.

> - you should use a version like 0.1 instead of 1, because I'm assume
>   your are the upstream and this is the first release of the software

Yes, I am upstream and this is the first release, but I don't understand what's wrong with the version number. Version 1 means it's the first version, which is what it is. The package is too small to have major and minor version numbers, so if I need to change something I'll release version 2. What's the problem?

> - Package has no proper BuildRoot definition

Could you please explain why a buildroot definition is still needed? I'm not planning to package this for anything older than Fedora 10, so any buildroot I define will be ignored and %_topdir/BUILDROOT will be used instead.


Before I upload a new package I want to understand why you think the version number is wrong.

(In reply to comment #3)
> (In reply to comment #2)

> Fixed (but it looks like I can't update the title of this review request).

You should be able to change the title of the bug by following the (edit) link right on the title on top of this bug
  
> "Copyright only" is listed under Good Licenses at
> https://fedoraproject.org/wiki/Licensing:Main. Please explain why you think it
> isn't valid.

Your right. 'Copyright only' sounds a little odd for a OSS license, but you have right that this is it one.
 
> These files are so small that I don't care much about the license. I'd place
> them in the public domain, but as I understand it I can't do that under Swedish
> copyright law, so I figured I'd require copyright attribution only and
> otherwise let anyone do anything with them.
 
> > - Package contains not a verbatin copy of the license text
> 
> common.gpr contains the complete license text:
> -- Copyright 2009 B. Persson, Bjorn
> -- No restrictions are placed on this code.
> 

It's a good style to distribute the verbatin text of the license. Of corse this may be a small file on your case, but in common it's a good idea, because the user will get the version of the license text which was valid at the time of publisching of your software. Additionally, It's not a blocker for approvement, if a packaging is missing one.

> Yes, I am upstream and this is the first release, but I don't understand what's
> wrong with the version number. Version 1 means it's the first version, which is
> what it is. The package is too small to have major and minor version numbers,
> so if I need to change something I'll release version 2. What's the problem?

first, because it's your first release, I have to assume, that this package is not widely tested and may contains bug. So it's a good idea to beginn the versioning with a version nummer less the 1.0.

Second, If you have a major and a minor version number you can disting between greater changes which may introduced new feature or have a API breakage and minor changes which only fixed bugs.

(In reply to comment #3)

> Could you please explain why a buildroot definition is still needed? I'm not
> planning to package this for anything older than Fedora 10, so any buildroot I
> define will be ignored and %_topdir/BUILDROOT will be used instead.

for the buildroot stuff please keep in mind, that there are the EL-4 and EL5 branches which based on RHEL 4 and RHEL 5. this releases contains older releases of rpm because this distributions are designed for a very long lifecycle in enterprises.

If you may interesting on RHEL without paying money, you may have a look on the CentOS project.

New version:
http://www.rombobjörn.se/packages/fedora-gnat-project-common.spec
http://www.rombobjörn.se/packages/fedora-gnat-project-common-1.1-2.fc11.src.rpm

(In reply to comment #4)
> It's a good style to distribute the verbatin text of the license. Of corse this
> may be a small file on your case, but in common it's a good idea, because the
> user will get the version of the license text which was valid at the time of
> publisching of your software.

The license text inside the actual project file is also the one that was valid at the time of publishing. Anyway, I've added a license file now. It should at least make it clearer that the license covers all four files.
 
> first, because it's your first release, I have to assume, that this package is
> not widely tested and may contains bug. So it's a good idea to beginn the
> versioning with a version nummer less the 1.0.

That version numbers would contain any information about the quality of the software is an illusion. The amount of bugs depends on how meticulous the programmer is and what measures he takes to avoid bugs, not on the version number. I've seen high-quality software with low version numbers, and garbage with high version numbers.

Version numbers should be ordinal numbers – first, second, third and so on. There is no such ordinal number as "zeroth". I'll call it the zeroth version if I must to get the package approved, not otherwise.

> Second, If you have a major and a minor version number you can disting between
> greater changes which may introduced new feature or have a API breakage and
> minor changes which only fixed bugs.  

Now that I think about it I can see situations where a minor version number would be useful, so this is the first major version, and the first minor version of that major version, that is version 1.1.

(In reply to comment #5)
> for the buildroot stuff please keep in mind, that there are the EL-4 and EL5
> branches which based on RHEL 4 and RHEL 5. this releases contains older
> releases of rpm because this distributions are designed for a very long
> lifecycle in enterprises.

When I started making my packages on Fedora 9 I had to hack around all sorts of problems – broken debuginfo packages, bogus dependencies, bad symlinks and whatnot. Then I found that several of my workarounds were unnecessary on Fedora 10, so I removed them. One more is unnecessary on Fedora 11. To build the packages for EPEL I'd have to insert the workarounds again, and probably run into even more problems.

I'll take one step at a time. When the time comes for RHEL 6, maybe I'll be ready to build my packages for EPEL 6 – which will hopefully not need any more workarounds than Fedora 11 (nor buildroot tags) – but for now Fedora 10 and 11 is enough.

Hmm, has this package been brought to attention of Fedora's GCC maintainers or upstream GCC?

I am having strong doubts on whether this package is required at all, useful or correct, for several reasons:
* gnat has been multilib'ed in gcc.
* the profile.d scripts pollute the envionment.
... however my knowledge on gnat is rather limited ...


Finally, this spec looks really bewildering to me
- The %global _GNAT_project_dir ... line
This can be done very much efficient during a %build
- We do not support translations in *.specs

(In reply to comment #7)
> Hmm, has this package been brought to attention of Fedora's GCC maintainers or
> upstream GCC?

Well, it blocks review request 509159, which depends on bug 507247, which is a bug in GCC that I hope at least Jakub Jelinek is aware of. Other than that I haven't contacted the GCC maintainers yet.

It's quite possible that some or all of this package ought to be included in the gcc-gnat package eventually, but I felt that it would be more flexible to make a separate package initially. If I find, as I continue packaging Ada software, that I need another RPM macro or project file variable, then I can add it to this package quickly without requiring an update to GCC. When my approach has stabilized and proven itself functional, as I hope it will, then it may be time to merge it into the gcc-gnat package, or even upstream GCC.

> * gnat has been multilib'ed in gcc.

I want to use GNAT project files to make Ada libraries easy to use. The project files have to reference architecture-specific library files in either /usr/lib or /usr/lib64. That means either the project files have to be architecture-specific, or else a single project file must be made to change its behaviour depending on the architecture. Architecture-specific project files would have to be placed in architecture-specific directories, so that both versions could be installed simultaneously in a multilib environment. Then GNAT would have to look for project files in the one or the other directory, not depending on which version of GNAT is installed, but depending on which architecture it's compiling for at the moment. As far as I can see, GNAT has no support for that. Therefore I chose the other approach, to make the project files adapt to the architecture. Rather than duplicating the code to do that in each project file, they should import common.gpr.

> * the profile.d scripts pollute the envionment.

They do, and I'm not happy about it. It's not even a complete solution as (I think) they're effective only in interactive shells. Nonetheless it's the best solution I could find. I can't invoke uname from common.gpr because GNAT project files have no shell-out feature. I thought I had found the solution when I saw the variable HOSTTYPE, but apparently Bash doesn't export that to child processes, and it's not affected by setarch. At least these scripts will pollute the environment only for programmers who install -devel packages that depend on this package.

> Finally, this spec looks really bewildering to me
> - The %global _GNAT_project_dir ... line
> This can be done very much efficient during a %build

I agree that it's ugly but I figured that the SPOT rule was more important. This directory is already defined twice, in macros.gnat in this package, and somewhere in the GCC source. I didn't want to add a third location by hard-coding it in the spec file.

How do you mean it should be done in %build? By defining the directory in the spec file and writing it to macros.gnat during %build? I considered that but then the "upstream project" wouldn't contain the complete source code, which I found a bit weird. I can do it that way however, if it's deemed preferable.

> - We do not support translations in *.specs  

Then why do I find this SHOULD item on https://fedoraproject.org/wiki/Packaging/ReviewGuidelines?
"The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available."

How else do you think translations should be done?

New version:
http://www.rombobjörn.se/packages/fedora-gnat-project-common.spec
http://www.rombobjörn.se/packages/fedora-gnat-project-common-1.1-3.fc11.src.rpm

Apparently the buildroot tag is still required for bureaucratic reasons, so I added one. I also fixed some other RPMlint warnings. The only one that remains now is "only-non-binary-in-usr-lib", and I can't really do anything about that without also modifying GNAT.

Updated to version 1.2:
http://www.rombobjörn.se/packages/fedora-gnat-project-common.spec
http://www.rombobjörn.se/packages/fedora-gnat-project-common-1.2-1.fc11.src.rpm

Potential sponsors may want to look at my comments on these review requests:
https://bugzilla.redhat.com/show_bug.cgi?id=508126#c2
https://bugzilla.redhat.com/show_bug.cgi?id=510743#c3
https://bugzilla.redhat.com/show_bug.cgi?id=514351#c1
https://bugzilla.redhat.com/show_bug.cgi?id=514509#c2
https://bugzilla.redhat.com/show_bug.cgi?id=458866#c4

I'll review it.

Björn, are you still here?

I'm here.

unblocking FE-NEEDSPONSOR - I just sponsored Björn Persson.

I don't know much about Ada and GNAT, but you hardcoded %_GNAT_project_dir to /usr/lib/gnat. Even on 64-bit architectures. Is it intentional?

Yes, that's intentional. That's where GNAT looks for project files by default, even on 64-bit architectures. (Well, at least on x86_64. I don't have a PPC64 to test on.) It could be argued that /usr/share/gnat would be a better location but that would require a change in GNAT. I made an RPM macro so that it will be easy to change for all Ada libraries if the location changes in the future.

Ok, understood. Here is my 

REVIEW:

+/- rpmlint isn't silent.

[petro@Sulaco SPECS]$ rpmlint ../RPMS/noarch/fedora-gnat-project-common-1.2-1.fc11.noarch.rpm 
fedora-gnat-project-common.noarch: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
[petro@Sulaco SPECS]$

However this warning may be safely ignored (keeping in mind your notes about common place for GNAT projects).

+ The package is named according to the Package Naming Guidelines.
+ 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.
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.

+/- The spec file for the package is legible, except the dirty trick with _GNAT_project_dir (fortunately, this would go away then the package hits Fedora packages collection).

+ The sources used to build the package matches the upstream source.

[petro@Sulaco SOURCES]$ sha256sum fedora-gnat-project-common-1.2.tar.gz*
f4f63a0cb90193966e21af236b07fd63b725e80617c26cba98b082ad98067146  fedora-gnat-project-common-1.2.tar.gz
f4f63a0cb90193966e21af236b07fd63b725e80617c26cba98b082ad98067146  fedora-gnat-project-common-1.2.tar.gz.1
[petro@Sulaco SOURCES]$ 

+ The package successfully compiles and builds into binary rpms on at least one primary architecture. (ppc)
+ The package does NOT bundle copies of system libraries.
+ 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.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package contains code or permissible content.
+ Everything, the package includes as %doc, does not affect the runtime of the application.
+ the package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
+ All filenames in rpm packages are valid UTF-8.


APPROVED.

@Björn,

Did you have create a FAS account.

If Peter is not a sponsor, I can do this job for him.

(In reply to comment #15)
> I don't know much about Ada and GNAT,

(In reply to comment #17)
> APPROVED.  
The combination of comment #15 and #17 makes me sad.

Also, I stand to what I said in comment #7.

Thanks Peter, for the review and for sponsoring me.

Jochen, my FAS username is rombobeorn.

Ralf, since you never replied to comment #8 I assumed that you were satisfied with how I addressed your concerns.

New Package CVS Request
=======================
Package Name: fedora-gnat-project-common
Short Description: Files shared by Ada libraries
Owners: rombobeorn
Branches: F-10 F-11 F-12
InitialCC:

cvs done.

fedora-gnat-project-common-1.2-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/fedora-gnat-project-common-1.2-1.fc11

fedora-gnat-project-common-1.2-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/fedora-gnat-project-common-1.2-1.fc10

fedora-gnat-project-common-1.2-1.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/fedora-gnat-project-common-1.2-1.fc12

fedora-gnat-project-common-1.2-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

fedora-gnat-project-common-1.2-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: fedora-gnat-project-common
New Branches: el6
Owners: rombobeorn

Git done (by process-git-requests).