Bug 226415

Package Change Request
======================
Package Name: sgml-common
Updated Fedora Owners: ovasik

* BuildRoot is not among the preferred ones
* the following requires seems wrong to me, they are old:
Requires: sh-utils fileutils textutils
  what are they for? And the grep requires?
* where comes sgml-common-automake.tar.gz from?
  shouldn't it better to take those files from the automake package?
* url is lacking. Maybe
http://www.math.ias.edu/doc/sgml-common-0.6.3/html/index.html
* if some files come from jadetex, they should come from the
  upstream package
* timestamps should be kept
* rpm macros should used, and used consistently
* where comes the CHANGES file from?
* %makeinstall should certainly not be used 
* I don't think that the with-docdir patch is for fedora, it is 
  for upstream. In fedora there are other ways to do it, maybe
  along
make install.... htmldir=....
  This would remove the need for autoconf/automake
* the %clean section could be simplified

There are certainly more issues, just try to fix as much as 
possible.

Suggestion: change %defattr (-,root,root) to %defattr (-,root,root,-)

CCing maintainer.

- build root will be changed
- requires you mentioned are in exception list so they could and will be removed
- URL you provided is not ok, much better is http://www.w3.org/2003/entities/
which I used for xml subpackage in May when I got the package - now will be used
generally as substitute of no longer available ISO 8879 page. Or I can use
http://www.linuxfromscratch.org/blfs/view/svn/pst/sgml-common.html - but this is
not upstream URL, it is only small howto/description for current sgml-common.
- timestamps kept by usage of -p 
- rpm macros like %{_bindir} and %{_datadir} used
- files xml.dcl, xml.soc and sgml.dcl, sgml.soc are from openjade package,
pubtext subdir, but it is a bit overkill to have whole 8.8M openjade package as
sources because of 10 kbytes of files not modified since 1999 (maybe longer)...
- that Changes file is outside the tarball(and included) since RHL 7.1, by Tim
Waugh - added because of changed location of files. I think it could be removed
because it is quiet a long time since the change of names and locations occured.
- %defattr(-, root, root, -) will be used

Additionally: fixed rpmlint warnings/issues - License tag, nonconfig file marked
as config, summary ended with dot.

So the points which I want to discuss are:
1) Do you really think that including openjade tarball into sources to digout a
few files from it is necessary? Those files didn't changed for years and I'm
maintainer of Openjade package - and it is mentioned that the files are from
OpenJade/pubtext dir.
2) About that usage of automake package: This would require to rewrite some
things inside the tarball and without adding any value - except more clean package. 
3) suggestion about with-docdir.patch for upstream - there is no upstream
page/bugzilla afaik, last package version is made 3 years ago - so I don't know
how can I get it to upstream and remove from fedora.

Will hold the build with changes until those three points will be clarified.

(In reply to comment #4)

> So the points which I want to discuss are:
> 1) Do you really think that including openjade tarball into sources to digout a
> few files from it is necessary? Those files didn't changed for years and I'm
> maintainer of Openjade package - and it is mentioned that the files are from
> OpenJade/pubtext dir.

Ok, but then add to the comment that you are the openjade maintainer
and verify that the timestamps are right such that it is easy to 
check the file age and compare with the files from openjade. If the 
timestamps are not right and cannot be changes (it is often the case
when the files are already in cvs), please note the original timestamps
in the spec file.

> 2) About that usage of automake package: This would require to rewrite some
> things inside the tarball and without adding any value - except more clean
package.

I don't really understand why. What it the technical issue here?
In any case having clean package is an explicit goal of 
fedora packaging and package review.

> 3) suggestion about with-docdir.patch for upstream - there is no upstream
> page/bugzilla afaik, last package version is made 3 years ago - so I don't know
> how can I get it to upstream and remove from fedora.

You can keep it in the srpm if you like but you shouldn't apply
it nor rerun the autotools, there are less intrusive ways to
achieve the same result. I can do some spec file patch if you like.

ad 1) Ok, I'll mention that I'm OpenJade maintainer and that the files with the
sgml-common are up2date
ad 2) there was one problem which denied usage of latest autotools, found a way
how to solve this, so they will be no longer shipped with package(and
BuildRequires: AutoMake and AutoConf added ...
ad 3) ok, kept in SRPM but not applied in prep of spec file

Built as sgml-common-0.6.3-22.fc9 with changes you recommended. 

Created attachment 266561 [details]
spec patch to avoid rerunning autotools and use %doc 

There are 4 changes in the patch:
- copy automake-1.4 files instead of rerunning autotools
- preserve timestamps
- use mkdir -p for %{_datadir}/sgml/docbook even though it
  is not strictly required
- remove installed documentation and install it with %doc
  along with more files
- in files use %{_mandir}/man8/install-catalog.8*

feel free to use only what you want. The part about avoiding
rerunning the autotools is a must fix, however.

Another issue is that the xml-common description doesn't seems to me 
to describe what is in the package.

Ok, thanks for objections/hints/patch, will use the changes from your spec patch
and add/improve description of xml-common subpackage. Will be built as
sgml-common-0.6.3-23.fc9 ...

Just to prevent issues - what description you would like to have?
I changed that to - it is better for you?:
The xml-common is subpackage of sgml-common package which contains 
a collection XML catalogs that are useful for processing XML, but 
that don't need to be included in main package.

(In reply to comment #9)

> Just to prevent issues - what description you would like to have?
> I changed that to - it is better for you?:
> The xml-common is subpackage of sgml-common package which contains 
> a collection XML catalogs that are useful for processing XML, but 
> that don't need to be included in main package.

Looks good.

Just one question - when I'm trying to build the package in mock with BuildReq:
automake-1.4 , it fails - it seems that last time that package was build is
RHL-7.3 ... will this build in koji? Or it has to be changed somehow? 

aaah, sorry , it seems that BuildRequires: automake is enough because it
includes automake-1.4 subdir ...

heh, ok, no, BuildRequires:automake14 is neccessary. Anyway, fixed and building
in koji as sgml-common-0.6.3-23.fc9

Yep, I used a bad BR name sorry.

I don't remember why I didn't continued the review...

Anyway looks good now, except that the xml-common url doesn't
correspond at all with the xml-common package. This url, however
links to some explanations, and ultimately to files in
http://www.w3.org/2003/entities/2007/
Sone of these files are in docbook-dtds. I don't know about others.
Should they be shipped somehow?

(In reply to comment #15)
> Anyway looks good now, except that the xml-common url doesn't
> correspond at all with the xml-common package. This url, however
> links to some explanations, and ultimately to files in
> http://www.w3.org/2003/entities/2007/

Actually I think there is no free completely corresponding url for sgml-common
and xml-common. Old swiss page with iso 8879 was not 100% correct too, because
in sgml-common and xml-common is only subset of full set of entities. Anyway - I
think the best way is to keep it as it is done now - with link to w3.org entities.

> Sone of these files are in docbook-dtds. I don't know about others.
> Should they be shipped somehow?

Which files do you mean? I agree that docbook-dtds corresponds somehow with
sgml-common/xml-common entities - as both are subsets docbook entities.

(In reply to comment #16)
> (In reply to comment #15)
> > Anyway looks good now, except that the xml-common url doesn't
> > correspond at all with the xml-common package. This url, however
> > links to some explanations, and ultimately to files in
> > http://www.w3.org/2003/entities/2007/
> 
> Actually I think there is no free completely corresponding url for sgml-common
> and xml-common. Old swiss page with iso 8879 was not 100% correct too, because
> in sgml-common and xml-common is only subset of full set of entities. Anyway - I
> think the best way is to keep it as it is done now - with link to w3.org entities.

For sgml-common, sure, but for xml-common, I think that the best
is not to provide any URL, since there is no url corresponding with
that package. It will inherit the sgml-common url fine in any case.
 
> > Sone of these files are in docbook-dtds. I don't know about others.
> > Should they be shipped somehow?
>
> Which files do you mean? I agree that docbook-dtds corresponds somehow with
> sgml-common/xml-common entities - as both are subsets docbook entities.

All the .ent files at:
http://www.w3.org/2003/entities/2007/

nobody was assigned, I'll take this review now

OK source files match upstream:
103c9828f24820df86e55e7862e28974  sgml-common-0.6.3.tgz
OK source contains full URL
OK package meets naming and versioning guidelines.
OK specfile is properly named, is cleanly written and uses macros consistently.
OK dist tag is present.
OK build root is correct.
OK license field matches the actual license (GPL+).
OK license is open source-compatible. License text included in package.
OK latest version is being packaged.

OK* BuildRequires are proper.
- there's a require on older version of automake, from the previous
review text I see the package needs it
BuildRequires: libxml2 >= 2.4.8-2 also looks quite suspicious
- isn't the version number superfluous? I see there is 
version 2.7.6 of this library in F10 and no older versions are available

OK compiler flags are appropriate. - no compilation necessary
OK %clean is present.
OK package builds in mock.
OK debuginfo package looks complete. - no debuginfo necessary

BAD rpmlint is silent.
sgml-common.spec: W: patch-not-applied Patch3: sgml-common-automake.patch
sgml-common.spec: W: patch-not-applied Patch4: sgml-common-0.6.3-docdir.patch
- unused patches ought to be commented out, I see there is
a comment in the spec you want to keep this patches in the SRPM
but not apply them: does this make sense? why?
xml-common.noarch: W: no-documentation - this is OK I guess, the
description of the package seems to be enough

OK final provides and requires look sane.
N/A %check is present and all tests pass.
OK no shared libraries are added to the regular linker search paths.
OK owns the directories it creates.
OK doesn't own any directories it shouldn't.
OK no duplicates in %files.
OK file permissions are appropriate.
OK no scriptlets present.
OK code vs content - can be seen as both, but that's ok
OK documentation is small, so no -docs subpackage is necessary.
OK %docs are not necessary for the proper functioning of the package.
OK no headers.
OK no pkgconfig files.
OK no libtool .la droppings.
OK not a GUI app.

seems OK overall, just need to clarify the unused patches and maybe the BR on libxml2 should be without version

Sorry, I forgot to update that bugzilla. Unused patches removed, BR now unversioned. Built as sgml-common-0.6.3-32.fc13 .

OK, thanks, review+