Bug 389971
Summary: | Review Request: diveintopython - The html book | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Marc Wiriadisastra <marc> |
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | fedora-package-review, loganjerry, notting |
Target Milestone: | --- | Flags: | kevin:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2008-01-12 09:07:42 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Marc Wiriadisastra
2007-11-19 05:21:29 UTC
Lots of rpmlint errors that I need help with. I think the spec file can be improved and would love suggestions on improvements. 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 I've fixed those errors. http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython-5.4-2.fc8.src.rpm http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython.spec Appreciate helping me out :) Marc, do you need a sponsor? I can review your package, but I can't sponsor you. Would love a review and yes I need a sponsor. I would be happy to review this and look at sponsoring you... expect a full review in a bit here. 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? 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. 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/ 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. 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 (Removing NEEDSPONSOR: bug 426733) 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? The choices are htmlflat, html and pdf I have tried doc, txt both of those fail as a build option. 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. 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. 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. 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? 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. My bad a few more build requires. http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython.spec http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython-5.4-5.fc8.src.rpm 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. Fixed. The CSS file is not an issue it still functions fine. http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython.spec http://mwiriadi.fedorapeople.org/packages/diveintopython/diveintopython-5.4-6.fc8.src.rpm The package from comment #22 looks good to me... request cvs when you are ready. 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 cvs done. |