Bug 427482 - Review Request: publican-fedora - Fedora Publishing Theme
Review Request: publican-fedora - Fedora Publishing Theme
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jens Petersen
Fedora Extras Quality Assurance
:
Depends On: publican
Blocks: 427483 427484
  Show dependency treegraph
 
Reported: 2008-01-03 22:47 EST by Jeff Fearn
Modified: 2013-06-07 08:45 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-13 19:49:40 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
petersen: fedora‑review+


Attachments (Terms of Use)
publican-fedora.spec-1.patch (1.18 KB, patch)
2008-02-13 03:53 EST, Jens Petersen
no flags Details | Diff

  None (edit)
Comment 1 Jens Petersen 2008-01-03 23:37:31 EST
Some initial comments:

You can use %{_datadir} instead of %{_usr}/share.

I don't think "BuildRequires: documentation-devel" is necessary.

Personally I would prefer lowercase for the branch suffix:
ie documentation-devel-fedora.  That way it would be easier to find
with globbing etc.
Comment 2 Jens Petersen 2008-01-03 23:46:44 EST
err, brand suffix, not branch.
Comment 3 Jeff Fearn 2008-01-03 23:50:59 EST
BuildRequires: documentation-devel:

po2entity from documentation-devel is used to create translated entity files at
build time, so this BuildRequires is correct.

Cheers, Jeff.
Comment 4 Jens Petersen 2008-01-08 23:06:25 EST
Would you consider using "-fedora" for the suffix instead of "-Fedora"?
Comment 5 Jeff Fearn 2008-01-16 22:12:34 EST
(In reply to comment #4)
> Would you consider using "-fedora" for the suffix instead of "-Fedora"?

Sure, Fedora demoted from propern noun to plain old noun.

I'll wait until the main package is accepted before supplying new spec and srpm,
since I need to set the correct dependencies and rename this package to match.
Comment 7 Jens Petersen 2008-02-07 08:40:05 EST
Requires(post): coreutils
Requires(postun): coreutils

are not needed.

Minor: "-n %{name}-%{version}" is superfluous for %setup.
Comment 8 Jeff Fearn 2008-02-12 00:51:58 EST
http://svn.fedorahosted.org/svn/documentation-devel/trunk/Files/publican-fedora-0.8-0.fc9.src.rpm
http://svn.fedorahosted.org/svn/documentation-devel/trunk/Files/publican-fedora.spec

- Setup per Brand Book_Templates
- Fix soure and URL paths
- Use release in source path
- add OPL text as COPYING
- removed coreutils reqq
Comment 10 Jens Petersen 2008-02-12 21:12:54 EST
rpmlint says:

 publican-fedora.src:16: W: unversioned-explicit-obsoletes
documentation-devel-Fedora

This is ok, but probably it can be dropped assuming there aren't many people
using the old package.

 publican-fedora.src: W: invalid-license Open Publication License

The Fedora short license name is "Open Publication": see
http://fedoraproject.org/wiki/Licensing#head-7c329132fff48be993272795da69b49c7812e8a9

A few comments:

publican-0.29/Book_Template/en-US/ and publican-fedora-0.8/Book_Template/en-US
are the same, but presumably they are duplicated because they are under
different licenses, right?

The README file says "for building Red_Hat_Enterprise_Linux documentation".

It would be valuable to get comments on this package from the Fedora Docs project
since I am not really qualified to evaluate the templates, etc.

Otherwise the package basically looks ok to me.
Comment 11 Jeff Fearn 2008-02-12 21:52:31 EST
(In reply to comment #10)
> rpmlint says:
> 
>  publican-fedora.src:16: W: unversioned-explicit-obsoletes
> documentation-devel-Fedora
> 
> This is ok, but probably it can be dropped assuming there aren't many people
> using the old package.
> 
>  publican-fedora.src: W: invalid-license Open Publication License
> 
> The Fedora short license name is "Open Publication": see
>
http://fedoraproject.org/wiki/Licensing#head-7c329132fff48be993272795da69b49c7812e8a9

Fixed.

> A few comments:
> 
> publican-0.29/Book_Template/en-US/ and publican-fedora-0.8/Book_Template/en-US
> are the same, but presumably they are duplicated because they are under
> different licenses, right?

This is correct.

> The README file says "for building Red_Hat_Enterprise_Linux documentation".

Changed to Fedora.
 
> It would be valuable to get comments on this package from the Fedora Docs project
> since I am not really qualified to evaluate the templates, etc.

It's pretty hard for the Fedora docs team to comment on this since they can't
actually use the system until it's in Fedora.

I'm sure once it is in there and they can play with it a bit, they will have a
bunch of changes to the look and feel, and the common content. In fact I'm
hoping they will want to take over this package :)

I don't think the content needs to be exact since no Fedora documentation uses
this system yet.

> Otherwise the package basically looks ok to me.

http://svn.fedorahosted.org/svn/publican/trunk/Files/publican-fedora-0.8-2.fc9.src.rpm
http://svn.fedorahosted.org/svn/publican/trunk/Files/publican-fedora.spec
Comment 12 Jens Petersen 2008-02-13 02:21:30 EST
I think it would be better to change the package Group
from "Applications/Text" to something else like "Development/Libraries".
I am not sure if there is a more appropriate group.
Comment 13 Karsten Wade 2008-02-13 03:24:38 EST
(In reply to comment #11)
 
> It's pretty hard for the Fedora docs team to comment on this since they can't
> actually use the system until it's in Fedora.
> 
> I'm sure once it is in there and they can play with it a bit, they will have a
> bunch of changes to the look and feel, and the common content. In fact I'm
> hoping they will want to take over this package :)

+1

That was the plan; no need to slow the packaging process down for looking down
at that level.  Once it's a package, then we can commence to arguing stuff with
the upstream, like, "Can you switch everything to uses dashes '-' instead of
underscores '_'?"  I kid!!

Actually, I've looked a bit at the source and have seen this system on-and-off
over the years, and haven't seen any surprises.  Using it for Fedora Docs is
going to mean some potential modifications to the source XML (file names, other
conventions), but that is expected.  I reckon stuff will build with just a
little fiddling.
Comment 14 Karsten Wade 2008-02-13 03:34:39 EST
(In reply to comment #11)
> (In reply to comment #10)
> > rpmlint says:

> >  publican-fedora.src: W: invalid-license Open Publication License
> > 
> > The Fedora short license name is "Open Publication": see
> >
>
http://fedoraproject.org/wiki/Licensing#head-7c329132fff48be993272795da69b49c7812e8a9
> 
> Fixed.

The legal notice specifies " ... Open Publication License, V1.0 or
later ..."  My understanding from Red Hat Legal (discussion with Mark
Webbink) is that we don't want to use the "or later" language because we
don't want to commit to using something that doesn't even exist yet.

I'm not sure if this matters.  That can exist in the toolchain and not
be the same in the doc itself, right?  As upstream you have a range of
licensing decisions you can make, but since it's actually RHT as the
upstream, I recommend this more conservative language.

If you like, you can swipe the lingo directly from the Fedora Docs
legal notice:

http://docs.fedoraproject.org/release-notes/f8/en_US/legalnotice.html
http://docs.fedoraproject.org/release-notes/f8/en_US/sn-legalnotice.html

The first is a pointer with set-up, so the set-up lingo matches what
the main legal notice page has.  The wording on the
sn-legalnotice.html is directly from the OPL; the rest is a modified
grandchild of the legal notice that used to be in RHEL docs, inherited
through the original Fedora days.
Comment 15 Jens Petersen 2008-02-13 03:50:00 EST
Here is the review:

 +:ok, =:needs attention

MUST Items:
[=] MUST: rpmlint must be run on every package.

 publican-fedora.src:17: W: unversioned-explicit-obsoletes
documentation-devel-Fedora
 publican-fedora.noarch: W: obsolete-not-provided documentation-devel-Fedora

These are ok.

 publican-fedora.noarch: W: incoherent-version-in-changelog 0.8-1 0.8-2

Please take care of this one.

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}, in the format
%{name}.spec unless your package has an exemption on Package Naming Guidelines.

[=] MUST: The package must meet the Packaging Guidelines.

Please remove the redundant Requires(post) and Requires(postun).

[+] MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.
[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package must be included in %doc.

(But please consider Karsten's comments above.)

[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source

 f0d61bc6d241c57e03c91f40120c0e87 
package-review/publican-fedora/publican-fedora-0.8.tgz

[+] MUST: The package must successfully compile and build into binary rpms on at
least one supported architecture.
[+] MUST: All build dependencies must be listed in BuildRequires
[+] MUST: A package must own all directories that it creates.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros, as described in the macros
section of Packaging Guidelines.
[+] MUST: The package must contain code, or permissable content.
[+] MUST: If a package includes something as %doc, it must not affect the
runtime of the application.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT). See Prepping BuildRoot For %install for details.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[+] SHOULD: The reviewer should test that the package functions as described. A
package should not segfault instead of running, for example.

I tested "create_book --brand fedora --name ..." and a few obvious make targets,
and it seems to work ok.
Comment 16 Jens Petersen 2008-02-13 03:53:46 EST
Created attachment 294760 [details]
publican-fedora.spec-1.patch
Comment 17 Jens Petersen 2008-02-13 04:00:40 EST
I don't see any other problems with the package, but reviewers, feel free to post
additional comments if you should spot anything I might have overlooked.

Package is APPROVED, but please address the minor points raised above.
Comment 18 Jeff Fearn 2008-02-13 18:15:43 EST
New Package CVS Request
=======================
Package Name: publican-fedora
Short Description:  Publican documentation template files for fedora
Owners: jfearn
Branches: devel, F-8, EL-5, EL-4
InitialCC: mdious
Cvsextras Commits: yes
Comment 19 Jens Petersen 2008-02-13 18:56:00 EST
cvs admin is done
Comment 20 Jeff Fearn 2008-02-13 19:49:40 EST
built in koji

http://koji.fedoraproject.org/koji/buildinfo?buildID=36125
Comment 21 Ruediger Landmann 2013-06-06 20:41:42 EDT
Package Change Request
======================
Package Name: publican-fedora
New Branches: el6-docs
Owners: rlandmann
Comment 22 Gwyn Ciesla 2013-06-07 08:45:33 EDT
Invalid branch EL-6-docs requested

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