Bug 389971 - Review Request: diveintopython - The html book
Summary: Review Request: diveintopython - The html book
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-11-19 05:21 UTC by Marc Wiriadisastra
Modified: 2008-01-12 09:07 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-01-12 09:07:42 UTC
Type: ---
Embargoed:
kevin: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Marc Wiriadisastra 2007-11-19 05:21:29 UTC
Spec URL:http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython-html.spec
SRPM URL: http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython-5.4-1.fc8.src.rpm
Description: 
Dive into Python is a book on how to program in python.

Comment 1 Marc Wiriadisastra 2007-11-19 05:22:37 UTC
Lots of rpmlint errors that I need help with. I think the spec file can be
improved and would love suggestions on improvements.



Comment 2 Jerry James 2007-11-21 18:17:29 UTC
The name of the spec file should be diveintopython.spec.  If you are thinking
that you will someday offer up other formats besides HTML, then you might want
to consider starting with the DocBook sources and making subpackages for each
target format (PDF, etc.) instead.

Most of your rpmlint errors occur because of Windows-style line endings in the
HTML files and some (but not all) of the Python files.  If you generate the HTML
files from the DocBook sources, that won't happen for them (but will still be a
problem for the Python files).  Otherwise, you need to do something like this:

# Change Windows line endings to Unix line endings
for file in $(find . -type f); do
  sed -i 's/\r//' $file
done

There are also one XML file that is KOI8-R encoded.  Even though rpmlint
complains about it, you shouldn't touch it.  It is KOI8-R on purpose.

The license tag should be GFDL.

You don't need to list basesystem in your BuildRequires or Requires.  You also
should not list python in Requires, since it is not necessary to use this
package.  True, you need python to run any of the examples, but you can look at
the web pages without it, so it is required in sense that people need it to get
full utility out of your package, but it is not Required in the RPM sense that
the package doesn't work without it.  On the other hand, your package does
Require (in the RPM sense) xdg-utils.

Your %postun script is wrong.  If you have to remove something afterwards, that
means your package didn't "own" enough.  To fix this problem, remove the %postun
script entirely, and change your %install and %files sections to these:

%install
rm -rf $RPM_BUILD_ROOT
mkdir -p $RPM_BUILD_ROOT%{_datadir}/applications

desktop-file-install --vendor="fedora"                  \
        --dir=${RPM_BUILD_ROOT}%{_datadir}/applications \
        --add-category="Documentation"                  \
        %{SOURCE1}

%files
%defattr(-,root,root,-)
%doc .
%{_datadir}/applications/fedora-diveintopython.desktop


Comment 4 Jerry James 2007-12-03 23:15:26 UTC
Marc, do you need a sponsor?  I can review your package, but I can't sponsor you.

Comment 5 Marc Wiriadisastra 2007-12-04 01:54:34 UTC
Would love a review and yes I need a sponsor.

Comment 6 Kevin Fenzi 2008-01-02 00:32:15 UTC
I would be happy to review this and look at sponsoring you... 
expect a full review in a bit here. 

Comment 7 Kevin Fenzi 2008-01-02 00:56:27 UTC
After looking, I think it might be best to use the upstream docbook source and
build subpackages for html, txt, pdf, etc.

Would you be willing to take a stab at doing that?

Comment 8 Marc Wiriadisastra 2008-01-02 01:31:40 UTC
Yep I'll give that a go I'll speak to stickster or those in the docs project for
advice on the docbook and how to go about it.

Comment 9 Marc Wiriadisastra 2008-01-02 09:12:49 UTC
I might have had a few issues and the SRPM is significantly bigger because of
having to download the common files to compile it which I used to build the xml
-> html.

I haven't had a chance to speak to stickster as of yet but if I do and can learn
of a better way I'll use that or if you can point me in the right direction that
would be grat.

http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython.spec
http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython-5.4-3.fc8.src.rpm
All the patches are located in the following directory
http://mwiriadi.fedorapeople.org/packages/diveintopython/

Comment 10 Kevin Fenzi 2008-01-02 23:52:48 UTC
Well, it builds ok in mock here and rpmlint says only: 

diveintopython.noarch: W: wrong-file-end-of-line-encoding
/usr/share/diveintopython/diveintopython.css
diveintopython.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 10)

I only see the html output though... should it be generating the txt and pdf
outputs as well?

I can look further later tonight.

Comment 11 Marc Wiriadisastra 2008-01-03 00:10:24 UTC
I think it depends on the build.info which only has the html as the default
type.  The recommendation from stickster was to use yelp to display the xml
which I'm trying to get set up and tested at the moment.

My personal preference is to stick with html, pdf and txt

Comment 12 Mamoru TASAKA 2008-01-03 01:42:51 UTC
(Removing NEEDSPONSOR: bug 426733)

Comment 13 Marc Wiriadisastra 2008-01-03 14:44:17 UTC
I've got a problem at the moment. In order to build the pdf, html and txt
documents I need to go into the build.info and change the default build to pdf,
txt or info.

<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE project
[
<!ENTITY % version SYSTEM "xml/version.xml">
%version;
<!ENTITY common "xml/common">
<!ENTITY buildcommon SYSTEM "xml/common/build_common.xml">
]>
<project name="diveintopython" default="pdf" basedir=".">

<property name="lang" value=""/>
<property name="ftpdir" value=""/>

<target name="postprocess">
</target>

&buildcommon;

</project>

Should I make separate packages completely or can I separate the package out and
apply separate packages to patch the pdf, txt?

Comment 14 Marc Wiriadisastra 2008-01-03 14:53:57 UTC
The choices are htmlflat, html and pdf I have tried doc, txt both of those fail
as a build option.

Comment 15 Marc Wiriadisastra 2008-01-03 16:36:35 UTC
There is a all choice which I have patched so it will make all three.  I'm
currently finalizing the spec file and a new version will be uploaded tomorrow
my time.

This should have the resulting 3 packages from the source files listed in the
spec file currently.

Comment 16 Kevin Fenzi 2008-01-04 00:15:43 UTC
Excellent. Once you post that, I will do a full review... 

Depending on the sizes you could subpackage those 3, or just have one package
with all 3 formats available. 


Comment 17 Marc Wiriadisastra 2008-01-04 11:28:10 UTC
Ok done finally.

http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython-5.4-4.fc8.src.rpm
http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython.spec

I think you will want me to remove a significant amount of stuff.  Rpmlint is
fairly clean.  There are additional html files that shouldn't be there but they
were included in the build so I added them.

Comment 18 Kevin Fenzi 2008-01-05 01:55:32 UTC
Odd... it fails in mock here, but still produces an output file?

text:

postprocess:

BUILD FAILED
file:/builddir/build/BUILD/diveintopython-5.4/xml/common/build_common.xml:295:
Execute failed: java.io.IOException: java.io.IOException: Permission denied

Any ideas?

Comment 19 Marc Wiriadisastra 2008-01-05 04:01:39 UTC
I've got heaps of issues with my Java and I was surprised it works.  Also I have
selinux turned off.

Same thing happens in eclipse so I'm leaning towards an selinux issue.

Comment 21 Kevin Fenzi 2008-01-09 01:16:02 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GFDL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
09247597b21c6253b810f081053e56b5  diveintopython-html-5.4.zip
09247597b21c6253b810f081053e56b5  diveintopython-html-5.4.zip.sav
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
 - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
See below - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have dist tag
OK - Should package latest version

Issues:

1. The html subpackage should own:
/usr/share/diveintopython-html/

The pdf should own: /usr/share/diveintopython-pdf
etc.

ie, they are missing ownership of the top level datadir/name dir.

2. rpmlint says:

diveintopython.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 30)

Suggest: Might use only spaces or only tabs. Not a blocker.

diveintopython-html.noarch: W: wrong-file-end-of-line-encoding
/usr/share/diveintopython-html/diveintopython.css/diveintopython.css
diveintopython-single-html.noarch: W: wrong-file-end-of-line-encoding
/usr/share/diveintopython-single-html/diveintopython.css/diveintopython.css
diveintopython-txt.noarch: W: wrong-file-end-of-line-encoding
/usr/share/diveintopython-single-html/diveintopython.css/diveintopython.css

The css file is CR/LF... I don't think this matters as long as it's properly read.
Suggest: Ignore, unless you see problems loading it.

Issue #1 is pretty minor, provided you fix that before importing this package,
this package is APPROVED.

Comment 23 Kevin Fenzi 2008-01-09 22:02:48 UTC
The package from comment #22 looks good to me... request cvs when you are ready. 

Comment 24 Marc Wiriadisastra 2008-01-10 09:43:34 UTC
New Package CVS Request
=======================
Package Name: diveintopython
Short Description: A python programming guide
Owners: mwiriadi
Branches: F-7 F-8 devel
InitialCC: mwiriadi
Cvsextras Commits: yes

Comment 25 Kevin Fenzi 2008-01-10 16:38:31 UTC
cvs done.


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