Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226415
Summary: Merge Review: sgml-common Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: nobody@fedoraproject.org QAContact: fedora-package-review@redhat.com CC: twaugh@redhat.com
Fedora Merge Review: sgml-common
http://cvs.fedora.redhat.com/viewcvs/devel/sgml-common/ Initial Owner: twaugh@redhat.com
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226415
twaugh@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226415
ruben@rubenkerkhof.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226415
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |medium Priority|normal |medium Product|Fedora Extras |Fedora
ovasik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
------- Additional Comments From ovasik@redhat.com 2007-07-27 04:50 EST ------- Package Change Request ====================== Package Name: sgml-common Updated Fedora Owners: ovasik@redhat.com
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226415
wtogami@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
pertusus@free.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |pertusus@free.fr
------- Additional Comments From pertusus@free.fr 2007-11-14 15:45 EST ------- * 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,-)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
twaugh@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ovasik@redhat.com
------- Additional Comments From twaugh@redhat.com 2007-11-15 04:57 EST ------- CCing maintainer.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From ovasik@redhat.com 2007-11-15 09:43 EST ------- - 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From pertusus@free.fr 2007-11-17 09:49 EST ------- (In reply to comment #4)
So the points which I want to discuss are:
- 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.
- 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.
- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From ovasik@redhat.com 2007-11-21 09:09 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From pertusus@free.fr 2007-11-22 04:11 EST ------- Created an attachment (id=266561) --> (https://bugzilla.redhat.com/attachment.cgi?id=266561&action=view) 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From pertusus@free.fr 2007-11-22 04:12 EST ------- Another issue is that the xml-common description doesn't seems to me to describe what is in the package.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From ovasik@redhat.com 2007-11-22 05:04 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From pertusus@free.fr 2007-11-22 06:39 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From ovasik@redhat.com 2007-11-22 07:20 EST ------- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From ovasik@redhat.com 2007-11-22 07:23 EST ------- aaah, sorry , it seems that BuildRequires: automake is enough because it includes automake-1.4 subdir ...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From ovasik@redhat.com 2007-11-22 07:35 EST ------- heh, ok, no, BuildRequires:automake14 is neccessary. Anyway, fixed and building in koji as sgml-common-0.6.3-23.fc9
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From pertusus@free.fr 2007-11-22 08:04 EST ------- Yep, I used a bad BR name sorry.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
pertusus@free.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |pertusus@free.fr Status|NEW |ASSIGNED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Version|devel |rawhide
------- Additional Comments From pertusus@free.fr 2007-12-17 11:45 EST ------- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From ovasik@redhat.com 2007-12-19 08:17 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
------- Additional Comments From pertusus@free.fr 2007-12-19 09:36 EST ------- (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/
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: sgml-common
https://bugzilla.redhat.com/show_bug.cgi?id=226415
pertusus@free.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEEDINFO Flag| |needinfo?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226415
Patrice Dumas pertusus@free.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|pertusus@free.fr |nobody@fedoraproject.org Flag|needinfo? |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226415
Daniel Novotny dnovotny@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dnovotny@redhat.com AssignedTo|nobody@fedoraproject.org |dnovotny@redhat.com
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226415
--- Comment #18 from Daniel Novotny dnovotny@redhat.com 2010-01-15 07:53:46 EST --- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226415
--- Comment #19 from Ondrej Vasik ovasik@redhat.com 2010-02-03 04:44:29 EST --- Sorry, I forgot to update that bugzilla. Unused patches removed, BR now unversioned. Built as sgml-common-0.6.3-32.fc13 .
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226415
Daniel Novotny dnovotny@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |RAWHIDE Flag| |fedora-review+
--- Comment #20 from Daniel Novotny dnovotny@redhat.com 2010-02-03 07:03:21 EST --- OK, thanks, review+
package-review@lists.fedoraproject.org