Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 227075 - Review Request: jtidy-1.0-0.20000804r7dev.6jpp - HTML syntax checker and pretty printer
Summary: Review Request: jtidy-1.0-0.20000804r7dev.6jpp - HTML syntax checker and pret...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nuno Santos
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: 227059
TreeView+ depends on / blocked
 
Reported: 2007-02-02 17:42 UTC by Rafael H. Schloming
Modified: 2014-12-01 23:13 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-04-12 15:24:49 UTC
dbhole: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)
Spec file with AOT added (deleted)
2007-02-12 16:01 UTC, Fernando Nasser
no flags Details
Spec file with double Requires/missing BR fixed (deleted)
2007-02-12 19:27 UTC, Fernando Nasser
no flags Details
Spec file with double Requires/missing BR fixed (deleted)
2007-02-12 19:27 UTC, Fernando Nasser
no flags Details
Fixed spec file (deleted)
2007-02-12 22:51 UTC, Fernando Nasser
no flags Details

Description Rafael H. Schloming 2007-02-02 17:42:17 UTC
Spec URL: http://people.redhat.com/rafaels/specs/jtidy-1.0-0.20000804r7dev.6jpp.spec
SRPM URL: ftp://jpackage.hmdc.harvard.edu/JPackage/1.7/generic/SRPMS.free/jtidy-1.0-0.20000804r7dev.6jpp.src.rpm
Description: JTidy is a Java port of HTML Tidy, a HTML syntax checker and pretty
printer. Like its non-Java cousin, JTidy can be used as a tool for
cleaning up malformed and faulty HTML. In addition, JTidy provides a DOM
parser for real-world HTML.

Javadoc for jtidy.

Utility scripts for jtidy.

Comment 1 Andrew Overholt 2007-02-11 00:13:18 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

Comment 2 Fernando Nasser 2007-02-12 16:01:52 UTC
Created attachment 147907 [details]
Spec file with AOT added

Comment 3 Fernando Nasser 2007-02-12 16:20:40 UTC
I am passing the rest upstream.  So I hope to get a new version from them soon.

Comment 4 Fernando Nasser 2007-02-12 19:19:01 UTC
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.





Comment 5 Fernando Nasser 2007-02-12 19:27:32 UTC
Created attachment 147929 [details]
Spec file with double Requires/missing BR fixed

Comment 6 Fernando Nasser 2007-02-12 19:27:50 UTC
Created attachment 147930 [details]
Spec file with double Requires/missing BR fixed

Comment 7 Fernando Nasser 2007-02-12 19:29:04 UTC
Comment on attachment 147929 [details]
Spec file with double Requires/missing BR fixed

duplicate

Comment 8 Fernando Nasser 2007-02-12 19:30:00 UTC
http://people.redhat.com/fnasser/jtidy-1.0-0.20000804r7dev.8jpp.src.rpm

has the double Requires/missing BR fixed

Comment 9 Andrew Overholt 2007-02-12 22:08:31 UTC
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


Comment 10 Fernando Nasser 2007-02-12 22:51:13 UTC
Created attachment 147941 [details]
Fixed spec file

Comment 11 Fernando Nasser 2007-02-12 22:54:12 UTC
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}



Comment 12 Andrew Overholt 2007-02-13 16:03:53 UTC
(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 :).

Comment 13 Fernando Nasser 2007-02-13 16:22:07 UTC
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.


Comment 15 Deepak Bhole 2007-02-16 00:53:54 UTC
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@redhat.com> - 0.6-4
  - And fix the link syntax.

  * Fri Jun 23 2006 Jesse Keating <jkeating@redhat.com> 0.6-4
  - And fix the link syntax.

  * Fri Jun 23 2006 Jesse Keating <jkeating@redhat.com>
  - 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


Comment 16 Andrew Overholt 2007-02-16 15:56:51 UTC
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.

Comment 17 Deepak Bhole 2007-02-16 20:10:54 UTC
APPROVED. Reassigning to component owner.

Comment 18 Nuno Santos 2007-02-21 21:17:47 UTC
New Package CVS Request
=======================
Package Name: jtidy-1.0-0.20000804r7dev.6jpp
Short Description: HTML syntax checker and pretty printer
Owners: nsantos@redhat.com
Branches: FC-7
InitialCC: rafaels@redhat.com,dbhole@redhat.com


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