Bug 458685 - Review Request: R2spec - Python script to generate R spec file
Review Request: R2spec - Python script to generate R spec file
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-11 11:50 EDT by Pierre-YvesChibon
Modified: 2008-10-07 05:54 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-07 05:53:41 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora‑review+
huzaifas: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Pierre-YvesChibon 2008-08-11 11:50:29 EDT
Spec URL: https://fedorahosted.org/r2spec/browser/R2spec.spec
SRPM URL: https://fedorahosted.org/r2spec/browser/R2spec-2.3-2.fc9.src.rpm?format=raw
Description: 
This is a small python script that generates spec file for R libraries.
It can work from a URL or a tarball.
Comment 1 R P Herrold 2008-08-11 12:36:22 EDT
The website at:  
   https://fedorahosted.org/r2spec/

speaks of downloading:
   ... R2spec-2.3.tar.gz

but clearly the content is NOT gzipped.  The next part states (correctly):
   tar cvf R2spec\*

There is a 'impedence mismatch' between what is said, and what exists.

-- Russ herrold
Comment 2 R P Herrold 2008-08-11 12:44:17 EDT
Further, retrieving the .spec file thus:
    lynx -dump https://fedorahosted.org/r2spec/browser/R2spec.spec?format=raw > R2spec.spec

Clening up the line breaks, and gzipping the .tar archive:
     gzip R2spec-2.3.tar
     cp R2spec-2.3.tar.gz ~/rpmbuild/SOURCES/

and trying a local build:
      rpmbuild -ba R2spec.spec

This too fails:

[herrold@centos-5 r2spec]$ rpmbuild -ba R2spec.spec
  File "<string>", line 1
    from distutils.sys
                     ^
SyntaxError: invalid syntax
  File "<string>", line 1
    from distutils.sys
                     ^
SyntaxError: invalid syntax
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.59142
+ umask 022
+ cd /home/herrold/rpmbuild/BUILD
+ LANG=C

   ...

copying R2spec.1 -> /var/tmp/R2spec-2.3-2-root-herrold/usr/share/man/man1
creating /var/tmp/R2spec-2.3-2-root-herrold/etc
copying R2spec.conf -> /var/tmp/R2spec-2.3-2-root-herrold/etc/
+ chmod +x /var/tmp/R2spec-2.3-2-root-herrold//r2spec/R2spec.py
chmod: cannot access `/var/tmp/R2spec-2.3-2-root-herrold//r2spec/R2spec.py': No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.5028 (%install)

Implying a missing BR.

-- Russ herrold
Comment 3 Pierre-YvesChibon 2008-08-12 03:26:39 EDT
(In reply to comment #1)
> The website at:  
>    https://fedorahosted.org/r2spec/
> 
> speaks of downloading:
>    ... R2spec-2.3.tar.gz
> 
> but clearly the content is NOT gzipped.  The next part states (correctly):
>    tar cvf R2spec\*
> 
> There is a 'impedence mismatch' between what is said, and what exists.
> 

I think the error came from the command tar cvf which is actually tar xvf, sorry for the typo.

For the build, I think you are doing it wrong.

You should:
* download the SRPM
* rpm -ivh R2spec\*.src.rpm 
* cd rpmbuild/SPECS/ 
* rpmbuild -ba R2spec.spec

BTW, I see you are running a CentOS, let me know please if you have any dependencies issue.

Thanks,
Pierre
Comment 4 Pierre-YvesChibon 2008-08-12 03:27:18 EDT
For information:
http://koji.fedoraproject.org/koji/getfile?taskID=773131
Comment 5 R P Herrold 2008-08-12 10:24:54 EDT
The .spec file is NOT present in the tarball previously referred to.  That is the reason I did not use:
   rpmbuild -ta ...

[herrold@centos-5 r2spec]$ ls
R2spec-2.3  R2spec-2.3.tar.gz  R2spec.spec  README
[herrold@centos-5 r2spec]$ find . -name R2spec.spec
./R2spec.spec
[herrold@centos-5 r2spec]$ 

(the one 'fine' finds is the one I creates as previously outlined.)

I mentioned a missing BR already; see the last lines of comment #2 -- will there be an update?

-- Russ herrold
Comment 6 R P Herrold 2008-08-12 10:25:51 EDT
Comment #4 points to something broken:

http://koji.fedoraproject.org/koji/getfile?taskID=773131

yields this for me:

-----------------------------------------------------
Mod_python error: "PythonHandler mod_python.publisher"

Traceback (most recent call last):

  File "/usr/lib64/python2.4/site-packages/mod_python/apache.py", line 299, in HandlerDispatch
    result = object(req)

  File "/usr/lib64/python2.4/site-packages/mod_python/publisher.py", line 213, in handler
    published = publish_object(req, object)

  File "/usr/lib64/python2.4/site-packages/mod_python/publisher.py", line 412, in publish_object
    return publish_object(req,util.apply_fs_data(object, req.form, req=req))

  File "/usr/lib64/python2.4/site-packages/mod_python/util.py", line 439, in apply_fs_data
    return object(**args)

TypeError: getfile() takes exactly 3 non-keyword arguments (2 given)

----------------------------------------------------

-- Russ herrold
Comment 7 Pierre-YvesChibon 2008-08-12 10:44:48 EDT
euh... since when the spec file should be in the tarball ??

The spec file is in the src.rpm...

The error on the url seems more like a problem in koji than something else.
Here is a second:
http://koji.fedoraproject.org/koji/taskinfo?taskID=773877

Just curiosity, have you ever done a review ?
Comment 8 R P Herrold 2008-08-15 13:05:17 EDT
> euh... since when the spec file should be in the tarball ??

ummm ---  since forever?

rpmbuild -ta tarball.tar.gz 
  with works just fine when the .spec file is there.  This is a feature of rpmbuild and has been for a very long time.

This rpmbuild feature exists and a .spec file can live in a distributed tarball, because it permits a 'single point of truth' ("SPOT").  See: The Practice of Programming, Brian W. Kernighan, Rob Pike, Addison-Wesley, Reading, MA, USA, 1999 at p. 239

Cited at, eg: http://press.samedi-studios.com/publications/2004/jones2004pl4li/jones2004pl4li.pdf

> Single Point of Truth[30] If a single application has more than one description of the same information, then keeping these different descriptions from conflicting is difficult. The reason why such conflicts exist comes from the lack of ability to express the information in a single place. This may oc-
cur either because two or more implementation languages are used and are
not easily cross-callable or the one language is not powerful enough to state
the same truth only once. This is an example of the lack of a language (or
languages) ability to capture the information of a domain.

> For example, in the Press Pot Java annotation system[23], a single file main-
tains information describing bytecodes. By generating code from this descrip-
tion language to both C and Java, Press Pot is able to keep the semantic
description always in sync in both the Java front-end and the C back-end.
As another example, in C programs, it is difficult to express a data file format
in a single declaration and from that description drive both serialization
and deserialization without using some sort of description language. From
a description language, serializers and deserializers can be generated and
consistency insured.

=======================================
Comment 9 R P Herrold 2008-08-15 13:07:12 EDT
As to having done reviews, I have reviewed and packaged code for many, many years, but am not able to assent to the Fedora CLA, which I suppose is what you are really interested in.

-- Russ herrold
Comment 10 manuel wolfshant 2008-08-15 15:37:16 EDT
Sorry if I misunderstood the question raised by comment #5, but I guess that the reply from comment #7 should be read "since when is it mandatory for the spec file bundled in the src.rpm to be included in the bundled sources ?"


And the answer is "it is not". It's a "nice to have" but very few projects do that.
Comment 11 R P Herrold 2008-08-15 16:52:44 EDT
Hello, Michael,

The problem (as I noted it in #2) is that the .spec file as downloaded from the URL posted, is that it DOES NOT WORK, even after minor cleanup and repair.  SOMETHING is causing the error:

File "<string>", line 1
    from distutils.sys

But I decline to try to read the coder's mind.  


There is no indication that it has been fixed in this ticket.

Lots of other things in that .spec file are broken as well, and I have seen no update here to indicate that they have been fixed.

-- Russ herrold
   irc: orc_orc on freenode
Comment 12 manuel wolfshant 2008-08-16 11:43:41 EDT
Just for the record, I've built sucessfully in mock the src.rpm from #2, both for fedora-devel/x86_64 and for centos5/i386, without any intervention over the spec.


As of errors, the only one that I spot at a quick glance is a minor one: python is a useless BR because python-devel brings it in.


The only problem, but this has nothing to do with the packaging, is that upstream distributes the source as a file ending its name with ".tar.gz" despite the fact that it is a plain tar.
Comment 13 Pierre-YvesChibon 2008-08-18 11:20:25 EDT
Ok,
Sorry for the delay I have been away for few days.

> comment #8

Let me explain my though, this is my first project that I package, I have almost never seen the spec file in the sources that I packaged and did not know about the rpmbuild -ta.

I have always been working with src.rpm also because it is supposed to be the base of the review and what is available with yumdownload --source.

But I understand that some people could be interested on having the specfile with the source.

Thanks for the information.

> Comment #9
That was indeed my question I could not find you on the FAS.

> Comment #10

Thanks for the light, that was actually my question, I never faced a project with the spec file furnished (especially because I assume that the spec file can be rather different in OS like SuSE compare to Fedora -- just my guess I do not know enough SuSE).

Anyway there is a new version which include the spec file into the sources for the people who do not like src.rpm :)

http://pingoured.fr/public/R2spec/R2spec.spec
http://pingoured.fr/public/R2spec/R2spec-2.4.0.tar.gz
http://pingoured.fr/public/R2spec/R2spec-2.4.0-1.fc9.src.rpm

I corrected the python-devel and I think the tar.gz instead of tar

Fedorahosted server seems to be down at the moment but I will upload the new version as soon as I get an access back.

Regards,
Pierre
Comment 14 R P Herrold 2008-08-18 12:28:05 EDT
looks great to me so far ... with the files retrieved from your personal site, per comment #13, the tarball method of rebuilding, AND a simple rebuild of the SRPM work fine now.  The next step I will take is to package some R modules.

thanks -- Russ herrold
Comment 15 Pierre-YvesChibon 2008-08-18 16:49:17 EDT
Just for the record, did it work on the first SRPM or did it give the same error ?

If you are willing to put R package into Fedora repository, please drop me a line for the review and thanks for the testing :)

Regards,
Pierre
Comment 16 R P Herrold 2008-08-18 16:59:45 EDT
I did not test the first SRPM, as I was confused at the .tar v .tgz issue.  The 2.4.0 SRPM was what succeeded for me.

Perhaps Michael can do a Fedora review for you; I am involved with that project as a CLA signer.  Thanks.

-- Russ herrold
Comment 17 manuel wolfshant 2008-08-19 04:06:14 EDT
  Version:        2.3
  Release:        2%{?dist}

is the one that worked for me.
Comment 18 Pierre-YvesChibon 2008-08-20 03:38:05 EDT
I found a bug there are the updates

Spec URL: https://fedorahosted.org/r2spec/browser/R2spec-2.4.1/R2spec.spec
SRPM URL:
https://fedorahosted.org/r2spec/browser/R2spec-2.4.1-1.fc9.src.rpm?format=raw
Comment 19 R P Herrold 2008-08-20 16:31:44 EDT
As a matter of good programming style, it would be helpful to maintain a CHANGELOG file, or to complete %changelog stanza entries outlining what was done coming into a new tarball -- a version control system (something as simple and lightweight as RCS, or something more formal) will do this for you.

As near as I can make out, there was a path issue (which you cured with a swap), and an addition of a temporary file naming convention (which does NOT check for a collision, and so is potentially unsafe -- I would add a check there).

-- Russ herrold
Comment 20 Pierre-YvesChibon 2008-08-20 16:41:02 EDT
(In reply to comment #19)
> As a matter of good programming style, it would be helpful to maintain a
> CHANGELOG file, or to complete %changelog stanza entries outlining what was
> done coming into a new tarball -- a version control system (something as simple
> and lightweight as RCS, or something more formal) will do this for you.

Isn't there one ?
https://fedorahosted.org/r2spec/browser/R2spec-2.4.1/CHANGELOG
for the spec:
%doc README LICENSE CHANGELOG

> As near as I can make out, there was a path issue (which you cured with a
> swap), and an addition of a temporary file naming convention (which does NOT
> check for a collision, and so is potentially unsafe -- I would add a check
> there).

Your are referring to when it writes the spec file ?
Indeed it should check for the existence of the spec file before writing it, I'll patch that.

If no, I am not sure to see what you are referring to.

Thanks for the feed back
Comment 21 manuel wolfshant 2008-09-12 17:06:29 EDT
This is the review of R2spec-2.5.0-1.fc9.src.rpm ,as downloaded from https://fedorahosted.org/r2spec/browser/R2spec-2.5.0-1.fc9.src.rpm
The links provided in #18 are no longer valid

Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: devel/x86_64
 [x] Rpmlint output:
source RPM: W: strange-permission R2spec.spec 0600
binary RPM:empty
 [x] Package is not relocatable.
 [x] Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type:GPLv3+
 [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 %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     SHA1SUM of package:2f731969f1b994db723c1cd8d2e07e95e553fd04  R2spec-2.5.0.tar.gz
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot}.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: devel/x86_64
 [x] Package should compile and build into binary rpms on all supported architectures.
     Tested on: devel/x86_64
 [x] Package functions as described (according to comment #14 and #16).
 [-] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.

=== Issues ===
1. %Source0 does not work due to the web server; the comment below it is wrong, as it references version 2.4.0. Would be nice to fix it, so as to allow checking with less hassle. I had to hunt around the site for the links.
2. Is the empty __init__.py file needed ?
3. Permissions of the R2spec.spec files are 600. Not a blocker, but ugly. Please fix before you commit to CVS.
4. Please consider using %defattr(-,root,root,-) instead of %defattr(-,root,root).


Please fix/comment the above issues and I'll approve the package.
Comment 22 Pierre-YvesChibon 2008-09-15 14:25:40 EDT
(In reply to comment #21)
> === Issues ===
> 1. %Source0 does not work due to the web server; the comment below it is wrong,
> as it references version 2.4.0. Would be nice to fix it, so as to allow
> checking with less hassle. I had to hunt around the site for the links.
Fixed
> 2. Is the empty __init__.py file needed ?
Fixed (Removed)
> 3. Permissions of the R2spec.spec files are 600. Not a blocker, but ugly.
> Please fix before you commit to CVS.
Fixed
> 4. Please consider using %defattr(-,root,root,-) instead of
> %defattr(-,root,root).
Fixed

New release:
https://fedorahosted.org/releases/r/2/r2spec/R2spec-2.5.0-2.fc9.src.rpm
Comment 23 manuel wolfshant 2008-09-15 16:15:11 EDT
All looks well now. Package APPROVED
Comment 24 José Matos 2008-09-15 16:19:25 EDT
A small note regarding item 3. above. It is not unusual to have a zero size __init__.py file.

In python the __init__.py file means that directory is package even if __init__.py itself is empty.

So I would suggest to leave the empty file in this context.
Comment 25 Pierre-YvesChibon 2008-09-16 03:58:47 EDT
Ok thanks José I will change that at commit time.

New Package CVS Request
=======================
Package Name: R2spec
Short Description: Python script to generate R spec file
Owners: pingou
Branches: F-8 F-9 EL-4 EL-5
Comment 26 Huzaifa S. Sidhpurwala 2008-09-16 23:39:59 EDT
cvs done
Comment 27 Fedora Update System 2008-09-18 07:16:44 EDT
R2spec-2.5.0-3.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/R2spec-2.5.0-3.fc8
Comment 28 Fedora Update System 2008-09-18 07:16:49 EDT
R2spec-2.5.0-3.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/R2spec-2.5.0-3.fc9
Comment 29 Fedora Update System 2008-09-24 20:08:37 EDT
R2spec-2.5.0-3.fc8 has been pushed to the Fedora 8 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update R2spec'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F8/FEDORA-2008-8229
Comment 30 Fedora Update System 2008-09-24 20:11:17 EDT
R2spec-2.5.0-3.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update R2spec'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-8246
Comment 31 Fedora Update System 2008-10-07 05:53:35 EDT
R2spec-2.5.0-3.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 32 Fedora Update System 2008-10-07 05:54:08 EDT
R2spec-2.5.0-3.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

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