Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 177483
Summary: | Review Request: subversion-api-docs | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Bojan Smojver <bojan> |
Component: | Package Review | Assignee: | Toshio Kuratomi <toshio> |
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-extras-list, jorton, toshio |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-04-10 19:25:58 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: | 163779 |
Description
Bojan Smojver
2006-01-11 01:00:19 UTC
This is in relation to: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=177063 It would perhaps be simpler to just generate the docs from /usr/include/subversion-1 to avoid having to run configure and package whole tarball again; would just need a copy of the doxygen.conf from upstream and the doxygen invocation from the Makefile. Thanks for the idea - I quite like it. The whole thing with full svn source RPM being in both core and extras was weird, but I didn't think of doing this. I'll rework and report back when done. I uploaded the new version of the spec file to the same location. Lots of hacks, but it does work on my Rawhide box. There is also the source RPM there (ftp://ftp.rexursive.com/pub/fedora-extras/subversion-api-docs-1.3.0-1.src.rpm) - only 17kB :-) Quite an interesting package. Only a cursory look so far, but the one thing I noticed is your packaging the files into %{_docdir}/subversion-%{version}/api-docs. Why did you choose to put them there instead of allowing them to make their own %{_docdir} area? Answer to comment #5: Because they effectively belong to subversion package, but they cannot be distributed with Fedora Core because of the size constraints. So, they end up in the correct place using the Fedora Extras package. Okay -- I'll do a review tonight. Good: * Naming guidelines: the name of the source is subversion. This package is intended as an addon to the Core subversion package using the Core subversion headers (in subversion-devel) to generate its content. Thus the name looks like a subpackage of subversion. * spec file name matches package name * OSI approved license (BSD) * LICENSE text is included in the subversion package which is required by this package. As it's intended as an addon to the Core package extracting its content from the actual Core headers this seems acceptable to me. * Package is legible and in American English * Builds successfully on FC4 using version 1.2.3 instead of 1.3.0 * No excluded BuildRequires * All necessary BuildRequires provided. * No locales * No shared libraries * Not relocatable * Owns all directories it creates * No duplicate files * Correct permissions * %clean's the buildroot * Uses consistent macros * Permissible content: API documentation for a packaged library. * Does not own directories owned by other packages. Needswork: * Does the subversion-api-docs-doxygen.conf file come from upstream somewhere? Is it in their distributed tarball? Having a URL to the file would be good -- perhaps a link into their subversion repository for the %{version} tag? Needs addressing: * If we do treat this as a pseudo-subpackage of Core's subversion, then we need to make these changes: - Requires should be on subversion rather than subversion-devel - BuildRequires subversion-devel and the Requires should require the full EVR: BuildRequires: subversion-devel = %{version}-%{release} Requires: subversion = %{version}-%{release} This leads into my concern, however: - How are you going to keep this package synced up with Core's subversion? To do this right, you need to create a new version whenever Core releases an updated subversion package. Otherwise there will be unresolved dependencies between Core and Extras that prevent people from upgrading. This isn't a hard thing to do, but it is time critical. Let's say there's a security bug in neon that requires apps to be recompiled against the new neon library. This affects subversion, ethereal, and openoffice.org among others. These packages are recompiled aainst the new library and released. But because people have the subversion-api-docs package installed which hasn't been upgraded yet, yum refuses to upgrade them and people are left vulnerable until we rebuild subversion-api-docs against the new subversion. - "Large documentation files should go in a -docs subpackage." If these files were created as a subpackage of the Core subversion package they would likely just be included as %doc in a subpackage. They wouldn't be specially installed into the main subversion package's doc directory. So it seems much more graceful to install this into it's own documentation directory and not have a Requires: on subversion or subversion-devel. The API documentation can get out of sync with the installed subversion, then, but it seems preferable to blocking upgrading in case of security releases or the like. - We would want to include a copy of subversion's LICENSE in this case. > Does the subversion-api-docs-doxygen.conf file come from upstream somewhere? Yes. We can link into the correct branch/tag of SVN repository of Subversion. > Requires should be on subversion rather than subversion-devel People that use this package are developers that use Subversion APIs. It is unlikely they will be developing software that uses Subversion APIs without wanting to have subversion-devel installed. It make sense that this package pulls in the -devel package. > BuildRequires subversion-devel and the Requires should require the full EVR Documentation goes into packagename-%{version} subdirectory. APIs of Subversion don't change mid major release. The documentation in the newer version/release may be a bit better (i.e. more accurate) but that's not critical for the API or functioning of the package. That's why release info isn't necessary. > How are you going to keep this package synced up with Core's subversion? Well, that is a good question. AFAIK, FE packages are rebuilt by hand. The maintaner just needs to update the package when required - a situation that is no different to many other packages from FE that may depend on FC packages that get bumped up, therefore creating an upgrade problem. This can be fixed like this: Requires: subversion-devel >= %{version} Then we can also own: %doc %dir %{_docdir}/subversion-%{version} To make sure this directory gets whacked when the old version of subversion-api-docs finally gets upgraded. > We would want to include a copy of subversion's LICENSE in this case. Sure. Again, we can point to the relevant file in Subversion SVN repository. I don't think that'll work:: $ repoquery --requires --repoid=development subversion-devel apr-devel apr-util-devel subversion = 1.3.0-4.2 So whether we depend on subversion or subversion-devel we're creating a situation where we cannot update subversion-core because the documentation is out of sync. OTOH, not including the release portion of EVR in the package dependencies makes it less likely that the scenario I mentioned where a neon security flaw is blocked by out-of-sync subversion/subversion-api-docs could occur but the example would still be valid for security flaws in subversion itself. You argue yourself, that exact EVR isn't necessary for the package dependencies because an upgrade will just add accuracy to what is already there. My argument is just an extension of this: it's better that the subversion-api-docs are not made dependent on subversion and are placed in their own %{_docdir}/subversion-api-docs directory because it eliminates a potential security problem in trade for a similar accuracy of the documentation. In comment #10, did you take into account this (I mentioned it in my reply): Requires: subversion-devel >= %{version} With the above, the upgrade of subversion, subversion-devel and all its dependencies will simply go through as normal, as subversion-devel (the requirement for subversion-api-docs) will always be greater or equal to that of subversion-api-docs. So, unless there is some weird downgrade (e.g. FC starts shipping Subversion 1.2.9 instead of 1.3.0, 1.3.1 etc.), I don't see a problem with that. No security upgrade should be affected by this package. Ah -- missed the ">=". Hmmm... That sounds fine then. Roll another package with the URLs filled in, the updated Requires:, and docdir ownership and I'll take a look. Sorry about the delay. Just been very busy lately. I'll get to it probably some time next week. Hopefully, I addressed to concerns. The spec file and source RPM are at the same URLs as before: ftp://ftp.rexursive.com/pub/fedora-extras/subversion-api-docs.spec ftp://ftp.rexursive.com/pub/fedora-extras/subversion-api-docs-1.3.0-1.src.rpm 8888a30767fe277cd193bee2cfe0af95 subversion-api-docs-1.3.0-1.src.rpm All issues have been addressed. ACCEPTED The one thing I noted is that you should always bump the release when you make a new version, even while it is in review. Please remember this for next time. Thanks Toshio. I'll try to remember the release number rule for next time - version numbers are cheap after all :-) Again, sorry about the delay. I'm finding it hard these days to be on the right machine at the right time to finish this. Hopefully in the next week or so I'll move to next steps... |