Bug 227075
Summary: | Review Request: jtidy-1.0-0.20000804r7dev.6jpp - HTML syntax checker and pretty printer | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Rafael H. Schloming <rafaels> | ||||||||||
Component: | Package Review | Assignee: | Nuno Santos <nsantos> | ||||||||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||||||||
Severity: | medium | Docs Contact: | |||||||||||
Priority: | medium | ||||||||||||
Version: | rawhide | CC: | aortega, overholt, tross | ||||||||||
Target Milestone: | --- | Flags: | dbhole:
fedora-review+
wtogami: 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: | 2007-04-12 15:24:49 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: | |||||||||||||
Bug Depends On: | |||||||||||||
Bug Blocks: | 227059 | ||||||||||||
Attachments: |
|
Description
Rafael H. Schloming
2007-02-02 17:42:17 UTC
MUST: X rpmlint on jtidy srpm gives no output W: jtidy non-standard-group Text Processing/Markup/HTML . ignore this one E: jtidy unknown-key GPG#c431416d . I don't where this is coming from. Perhaps the SRPM just needs to be rebuilt on Fedora? E: jtidy tag-not-utf8 %changelog E: jtidy non-utf8-spec-file jtidy.spec . I think this *might* be the accent in Ville's last name * package is named appropriately X specfile name matches %{name} . the specfile should just be jtidy.spec X package meets packaging guidelines. . BuildRoot incorrect. As per this: http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot it should be: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) . remove "section free" . remove BuildArch . why have the scripts sub-package at all? I think we should just put jtidy.script into the main jtidy package. This should be done at JPackage, though, I guess, so don't worry about it here. * license field matches the actual license. * license is open source-compatible. * license text included in package and marked with %doc * specfile written in American English * specfile is legible . I think the script should be renamed to just %{name}.script ... but this is . why use %__rm and not just rm? . same for %__chmod, %ant, %__sed, and %__ln_s -> just a nit-pick and not something that will hold up the review * source files match upstream (md5sum checked) X package successfully compiles and builds on at least x86 . I get a whole bunch of these errors using the latest gcj 4.1 branch (with the generics backport): [javac] 26. ERROR in /home/andrew/rpmbuild/BUILD/jtidy-04aug2000r7-dev/src/org/w3c/tidy/DOMElementImpl.java (at line 31) [javac] public class DOMElementImpl extends DOMNodeImpl [javac] ^^^^^^^^^^^^^^ [javac] The type DOMElementImpl must implement the inherited abstract method Element.setIdAttribute(String, boolean) X BuildRequires are proper . one of the duplicate "Requires: xml-commons-apis" should become a BuildRequires * no locale data so no find_lang necessary * package is not relocatable * package owns all directories and files X no %files duplicates . I don't think the %ghost is necessary for the last entry in %files javadoc * file permissions are fine; %defattrs present * %clean present * macro usage is consistent * package contains code * no large docs so no -doc subpackage . javadoc package present * %doc files don't affect runtime * shared libraries are present, but no ldconfig required. * no pkgconfig or header files * no -devel package * no .la files * no desktop file * not a web app. * file ownership fine * final provides and requires are sane Note: we should try to gcj-ify this package while we're at it. SHOULD: * package includes license text X package builds on i386 . see above X package functions . I don't know how to test this package X package builds in mock my mock setup doesn't seem to be working but I'll try to test on Monday Created attachment 147907 [details]
Spec file with AOT added
I am passing the rest upstream. So I hope to get a new version from them soon. Humm, Bugzilla loses the comment when one also adds an attachment. Here is the last SRPM with AOT: http://people.redhat.com/fnasser/jtidy-1.0-0.20000804r7dev.7jpp.src.rpm Will try and get one without the duplicate requires and perhaps without the script subpackage if possible. Created attachment 147929 [details]
Spec file with double Requires/missing BR fixed
Created attachment 147930 [details]
Spec file with double Requires/missing BR fixed
Comment on attachment 147929 [details]
Spec file with double Requires/missing BR fixed
duplicate
http://people.redhat.com/fnasser/jtidy-1.0-0.20000804r7dev.8jpp.src.rpm has the double Requires/missing BR fixed The spec that is attached still has the double Requires/missing BR problem. Here is the review for the attached specfile and http://people.redhat.com/fnasser/jtidy-1.0-0.20000804r7dev.8jpp.src.rpm : Still remaining (things requiring fixes being with X): X rpmlint on jtidy srpm gives no output W: jtidy non-standard-group Text Processing/Markup/HTML . ignore this one E: jtidy unknown-key GPG#c431416d . ignore this. it's just the JPackage GPG key. E: jtidy tag-not-utf8 %changelog E: jtidy non-utf8-spec-file jtidy.spec . I think this *might* be the accent in Ville's last name . run: iconv -f iso-8859-1 -t utf-8 -o * package is named appropriately * specfile name matches %{name} X BuildRoot incorrect. Should be: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) X remove "section free" ? why have the scripts sub-package at all? I think we should just put jtidy.script into the main jtidy package. This should be done at JPackage, though, I guess, so don't worry about it here. * license field matches the actual license. * license is open source-compatible. * license text included in package and marked with %doc * specfile written in American English * specfile is legible ? I think the script should be renamed to just %{name}.script ? why use %__rm and not just rm? ? same for %__chmod, %ant, %__sed, and %__ln_s -> just nit-picks and not something that will hold up the review * source files match upstream (md5sum checked) X package successfully compiles and builds on at least x86 . I get a whole bunch of these errors using the latest gcj 4.1 branch (with the generics backport): [javac] 26. ERROR in /home/andrew/rpmbuild/BUILD/jtidy-04aug2000r7-dev/src/org/w3c/tidy/DOMElementImpl.java (at line 31) [javac] public class DOMElementImpl extends DOMNodeImpl [javac] ^^^^^^^^^^^^^^ [javac] The type DOMElementImpl must implement the inherited abstract method Element.setIdAttribute(String, boolean) X BuildRequires are proper . one of the duplicate "Requires: xml-commons-apis" should become a BuildRequires * no locale data so no find_lang necessary * package is not relocatable * package owns all directories and files * no %files duplicates * file permissions are fine; %defattrs present * %clean present * macro usage is consistent * package contains code * no large docs so no -doc subpackage . javadoc package present * %doc files don't affect runtime * shared libraries are present, but no ldconfig required. * no pkgconfig or header files * no -devel package * no .la files * no desktop file * not a web app. * file ownership fine * final provides and requires are sane SHOULD: * package includes license text X package builds on i386 . see above X package functions . I don't know how to test this package X package builds in mock my mock setup doesn't seem to be working but I'll try to test on Monday Created attachment 147941 [details]
Fixed spec file
http://people.redhat.com/fnasser/jtidy-1.0-0.20000804r7dev.8jpp.1.src.rpm iconv run (thanks for the hint) section removed R->BR both in attached spec and SRPM now (got the wrong spec file attached before) added .1%{?dist} (In reply to comment #9) > The spec that is attached still has the double Requires/missing BR problem. Verified. One problem I see is that I don't think its release is proper. I'm under the impression that it should be of the form: 0.Z.tag.Xjpp.Y%{?dist} I don't see a Z in this case. > E: jtidy tag-not-utf8 %changelog > E: jtidy non-utf8-spec-file jtidy.spec Verified. > X BuildRoot incorrect. Should be: > > %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) I think this should be fixed regardless of the current discussion. The *current* review guidelines specify the buildroot so please use it. > X remove "section free" Verified. > X package successfully compiles and builds on at least x86 I still get lots of: 7. ERROR in /home/andrew/rpmbuild/BUILD/jtidy-04aug2000r7-dev/src/org/w3c/tidy/DOMAttrImpl.java (at line 31) public class DOMAttrImpl extends DOMNodeImpl implements org.w3c.dom.Attr { ^^^^^^^^^^^ The type DOMAttrImpl must implement the inherited abstract method Attr.getSchemaTypeInfo() This needs to be fixed. > X BuildRequires are proper > . one of the duplicate "Requires: xml-commons-apis" should become a BuildRequires Verified. > X package builds on i386 > . see above > X package functions > . I don't know how to test this package These are still present (obviously :). I am not sure what is the better course of action w.r.t. the pre-release for these dated packages that were in the previous format (all others get the right rpm ordering, but the YYYYMMDD is a really big number, and for some reason Fedora decided that svn and cvs come _after_ the number). We have basically two choices: 1) Change the format now to the new one and... raise Epoch! 2) Let it be a little longer with the current date in the hopes a release will be issued. Also, I have my doubts about the way the source tar ball is named. This date seems to be the indication of a branch creation date, the release being actually the characters that come after it. I am sending an e-mail to the authors to get that straighten up before we have to do two Epoch bumpings. Updated spec and SRPM: http://overholt.ca/fedora/jtidy.spec http://overholt.ca/fedora/jtidy-1.0-0.1.r7dev.1jpp.1.src.rpm MUST: * package is named appropriately - match upstream tarball or project name OK - try to match previous incarnations in other distributions/packagers for consistency OK - specfile should be %{name}.spec OK - non-numeric characters should only be used in Release (ie. cvs or something) OK - for non-numerics (pre-release, CVS snapshots, etc.), see http://fedoraproject.org/wiki/Packaging/NamingGuidelines#PackageRelease OK - if case sensitivity is requested by upstream or you feel it should be not just lowercase, do so; otherwise, use all lower case for the name OK * is it legal for Fedora to distribute this? - OSI-approved OK - not a kernel module OK - not shareware OK - is it covered by patents? OK - it *probably* shouldn't be an emulator OK - no binary firmware OK * license field matches the actual license. OK * license is open source-compatible. - use acronyms for licences where common OK * specfile name matches %{name} OK * verify source and patches (md5sum matches upstream, know what the patches do) - if upstream doesn't release source drops, put *clear* instructions on how to generate the the source drop; ie. # svn export blah/tag blah # tar cjf blah-version-src.tar.bz2 blah OK * skim the summary and description for typos, etc. OK * correct buildroot - should be: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) OK * if %{?dist} is used, it should be in that form (note the ? and % locations) OK * license text included in package and marked with %doc OK * keep old changelog entries; use judgement when removing (too old? useless?) OK * packages meets FHS (http://www.pathname.com/fhs/) OK X * rpmlint on <this package>.srpm gives no output - justify warnings if you think they shouldn't be there Perhaps change group for javadoc to "Documentation".. ? I will not block on this though * changelog should be in one of these formats: * Fri Jun 23 2006 Jesse Keating <jkeating> - 0.6-4 - And fix the link syntax. * Fri Jun 23 2006 Jesse Keating <jkeating> 0.6-4 - And fix the link syntax. * Fri Jun 23 2006 Jesse Keating <jkeating> - 0.6-4 - And fix the link syntax. * Packager tag should not be used OK X * Vendor tag should not be used Tag is still there X * Distribution tag should not be used Tag is still there * use License and not Copyright OK * Summary tag should not end in a period OK * if possible, replace PreReq with Requires(pre) and/or Requires(post) OK * specfile is legible - this is largely subjective; use your judgement OK * package successfully compiles and builds on at least x86 OK * BuildRequires are proper - builds in mock will flush out problems here - the following packages don't need to be listed in BuildRequires: bash bzip2 coreutils cpio diffutils fedora-release (and/or redhat-release) gcc gcc-c++ gzip make patch perl redhat-rpm-config rpm-build sed tar unzip which OK * summary should be a short and concise description of the package OK * description expands upon summary (don't include installation instructions) OK * make sure lines are <= 80 characters OK * specfile written in American English OK * make a -doc sub-package if necessary - see http://fedoraproject.org/wiki/Packaging/Guidelines#head-9bbfa57478f0460c6160947a6bf795249488182b OK * packages including libraries should exclude static libraries if possible OK * don't use rpath OK * config files should usually be marked with %config(noreplace) OK * GUI apps should contain .desktop files OK * should the package contain a -devel sub-package? * use macros appropriately and consistently - ie. %{buildroot} and %{optflags} vs. $RPM_BUILD_ROOT and $RPM_OPT_FLAGS OK * don't use %makeinstall OK * locale data handling correct (find_lang) - if translations included, add BR: gettext and use %find_lang %{name} at the end of %install OK X * consider using cp -p to preserve timestamps Lines in %install use cp -a .. consider using cp -ap * split Requires(pre,post) into two separate lines OK * package should probably not be relocatable OK * package contains code - see http://fedoraproject.org/wiki/Packaging/Guidelines#CodeVsContent - in general, there should be no offensive content OK X * package should own all directories and files /usr/share/java is owned by jpackage-utils and it should be a requirement * there should be no %files duplicates OK * file permissions should be okay; %defattrs should be present OK * %clean should be present Ok * %doc files should not affect runtime OK * if it is a web apps, it should be in /usr/share/%{name} and *not* /var/www OK X * verify the final provides and requires of the binary RPMs Missing a "Requires: xml-commoms-apis" ? * run rpmlint on the binary RPMs OK SHOULD: * package should include license text in the package and mark it with %doc OK * package should build on i386 OK * package should build in mock Additionally: - Need to put %define gcj_support 1 at the top of the spec - 's' after the '-' in BSD-style should be capital Updated spec and SRPM: http://overholt.ca/fedora/jtidy.spec http://overholt.ca/fedora/jtidy-1.0-0.1.r7dev.1jpp.1.src.rpm (In reply to comment #15) > X * rpmlint on <this package>.srpm gives no output > - justify warnings if you think they shouldn't be there > Perhaps change group for javadoc to "Documentation".. ? I will not block on > this though Done. > X * Vendor tag should not be used > Tag is still there > > X * Distribution tag should not be used > Tag is still there Fixed, fixed. > X * consider using cp -p to preserve timestamps > Lines in %install use cp -a .. consider using cp -ap Done. > X * package should own all directories and files > /usr/share/java is owned by jpackage-utils and it should be a requirement Fixed. > X * verify the final provides and requires of the binary RPMs > Missing a "Requires: xml-commoms-apis" ? There is a Requires: xml-commons-apis already. > - Need to put %define gcj_support 1 at the top of the spec I've changed the crazy conditional gcj_support line to just be gcj_support 1 > - 's' after the '-' in BSD-style should be capital Fixed. APPROVED. Reassigning to component owner. New Package CVS Request ======================= Package Name: jtidy-1.0-0.20000804r7dev.6jpp Short Description: HTML syntax checker and pretty printer Owners: nsantos Branches: FC-7 InitialCC: rafaels,dbhole |