Bug 177658
Summary: | Review Request: mediawiki - The PHP-based wiki software behind Wikipedia | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Roozbeh Pournader <roozbeh> | ||||
Component: | Package Review | Assignee: | Mike McGrath <imlinux> | ||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | David Lawrence <dkl> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | alex, fedora-extras-list | ||||
Target Milestone: | --- | ||||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | mediawiki-1.5.6-4.fc5 | Doc Type: | Bug Fix | ||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2006-02-06 17:11: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 | ||||||
Attachments: |
|
Description
Roozbeh Pournader
2006-01-12 18:24:37 UTC
Hello Roozbeh, I cannot approve this package but I have some suggestions. * Strip the math executable with: install -s math/texvc $RPM_BUILD_ROOT%{wikidir}/math * go ahead and make the math portion a different package * I noticed mediawiki-install.txt is included as a source but is not included in the RPM after it gets built, perhaps that should go under %docs? (In reply to comment #1) > Hello Roozbeh, I cannot approve this package but I have some suggestions. > > * Strip the math executable with: > install -s math/texvc $RPM_BUILD_ROOT%{wikidir}/math Executables should not be stripped like this because it results in useless debuginfo packages. The rpmbuild process should automatically strip out the debug symbols and put them in the debuginfo package. So this rpmlint message should be ignored? W: mediawiki unstripped-binary-or-object /var/www/mediawiki/math/texvc (I am fairly new at reviewing packages) (In reply to comment #3) > So this rpmlint message should be ignored? > > W: mediawiki unstripped-binary-or-object /var/www/mediawiki/math/texvc Most likely no; it should be fixed instead so that rpmbuild can strip it. Some cases where rpmbuild is (was) not able to strip binaries include files installed without write permissions for the package builder, and setuid/setgid binaries (bug 117858). (In reply to comment #1) > * go ahead and make the math portion a different package Apparently it's not possible to use a single SPEC file to build a noarch package and a platform-specific in a single run, so the main package which is actually noarch will become platform-specific. Anyway, I did it for the sake of the exercise: Spec Url: http://guava.farsiweb.info/~roozbeh/mediawiki.spec SRPM Url: http://guava.farsiweb.info/~roozbeh/mediawiki-1.5.5-2.src.rpm > * I noticed mediawiki-install.txt is included as a source but is not included in > the RPM after it gets built, perhaps that should go under %docs? It already goes in the %docs. It gets named "INSTALL.fedora" in the final RPM. > W: mediawiki unstripped-binary-or-object /var/www/mediawiki/math/texvc That is weird. rpmlint doesn't give me any such warning on my own machines, and a proper *-debuginfo RPM is also created. Would you please attach a build log? Created attachment 123206 [details]
Build log that resulted in an unstripped binary
I've tested this on two machines, both FC4 and both of them gave me the
un-stripped binary warning with rpmlint.
(In reply to comment #6) > Created an attachment (id=123206) [edit] > Build log that resulted in an unstripped binary > > I've tested this on two machines, both FC4 and both of them gave me the > un-stripped binary warning with rpmlint. Apparently rpm-build's find-debuginfo.sh is never run during your build process. It's very probably your rpm settings. May I suggest that you use the build environment created using fedora-rpmdevtools's fedora-buildrpmtree in a clean user account, or use mock? New version: Spec Url: http://guava.farsiweb.info/~roozbeh/mediawiki.spec SRPM Url: http://guava.farsiweb.info/~roozbeh/mediawiki-1.5.5-3.src.rpm %changelog - Add PreReq for httpd, since we use the apache user - Make mediawiki-math dependencies more specific - Package documentation for mediawiki-math New version: Spec Url: http://guava.farsiweb.info/~roozbeh/mediawiki.spec SRPM Url: http://guava.farsiweb.info/~roozbeh/mediawiki-1.5.6-1.src.rpm %changelog * Tue Jan 31 2006 Roozbeh Pournader <roozbeh> - 1.5.6-1 - Update to upstream 1.5.6 -%files math needs a %defattr -%description change - "it has an wide" to "it has a wide" New version: Spec Url: http://guava.farsiweb.info/~roozbeh/mediawiki.spec SRPM Url: http://guava.farsiweb.info/~roozbeh/mediawiki-1.5.6-2.src.rpm %changelog * Tue Jan 31 2006 Roozbeh Pournader <roozbeh> - 1.5.6-2 - Add %%defattr for -math subpackage - Fixed typo in description -You could condense your %files section into: %config(noreplace) %{_sysconfdir}/httpd/conf.d/mediawiki.conf %attr(770,root,apache) %dir %{wikidir}/config %{wikidir}/config/index.php %dir %{wikidir} %{wikidir}/[^c]*[^h] with a chmod for %{wikidir}/maintenance/*.pl in %install. -Replace instances of "etc" with %{_sysconfdir} -Also, including doc's in the main package wouldn't increase the package size much. Just don't delete doc's, install them, use %doc to mark them and change [^c] to ^[cd] I don't know 100% what best practice is on consolidating %files other than I was asked to do it for Nagios (#176613), If no one else comments on it I'll ask IRC. Forgot something, replace PreReq with plain old Requires, soon the difference between Requires and PreReq will be phased out: http://fedoraproject.org/wiki/Packaging/Guidelines#tags (In reply to comment #12) > -You could condense your %files section into [...] Makes real sense. Thanks a lot. > -Replace instances of "etc" with %{_sysconfdir} Done. > -Also, including doc's in the main package wouldn't increase the package size > much. Just don't delete doc's, install them [...] OK, but I put them under /usr/share/doc/mediawiki-x.y.z/internals, since they are internal documents. (In reply to comment #12) > replace PreReq with plain old Requires, soon the difference > between Requires and PreReq will be phased out I somehow need to make sure httpd is installed before mediawiki, or RPM won't recognize the user "apache". Any other way to do this? (Not fixing this for the moment.) Will post new version soon. New version: Spec Url: http://guava.farsiweb.info/~roozbeh/mediawiki.spec SRPM Url: http://guava.farsiweb.info/~roozbeh/mediawiki-1.5.6-3.src.rpm %changelog - Refactor the %%files section (Mike McGrath) - Replace "/etc" with macro - Package docs under %%{_defaultdocdir}/internals - Minor change in description I believe you can use Requires(pre) instead of PreReq to achieve the same affect. I was looking at some that are already in Extras and some of them do them different ways. (In reply to comment #16) > I believe you can use Requires(pre) instead of PreReq to achieve the same > affect. Yes, that would work, and the package will be forward-compatible that way. New version: Spec Url: http://guava.farsiweb.info/~roozbeh/mediawiki.spec SRPM Url: http://guava.farsiweb.info/~roozbeh/mediawiki-1.5.6-4.src.rpm %changelog - Use Requires(pre) instead of PreReq (Mike McGrath) -rpmlint (mediawiki): E: mediawiki no-binary E: mediawiki non-standard-gid /var/www/mediawiki/config apache E: mediawiki non-standard-dir-perm /var/www/mediawiki/config 0770 These are safe to ignore -rpmlint (mediawiki-math): No output -Matches naming requirements -is %{name}.spec -Meets packaging guidelines -License is GPL -Spec file is written in English and is easy to follow -Source matches upstream -Successfully builds on i386 -All build requires listed, no duplicates -Locale not used -Package ownes all directories it creates -Contains no duplicate file listings -Both files sections contain proper %defattr -Cleans $RPM_BUILD_ROOT -%macro's used consistantly -Builds in mock Everything looks good, I look forward to using this package. One thing before you upload it, take my name out of the comments :-D APPROVED (In reply to comment #18) > Everything looks good, I look forward to using this package. Thanks a lot for the review. I really appreciated it, and I believe your comments have resulted in an easier time in maintaining it for the long term. > One thing before you upload it, take my name out of the comments :-D Well, usually someone who provides a patch (some of your comments were really a patch) is credited. I will take your name out if you don't wish to be credited, but these were really your patches, you were really helpful, and I'd really like to credit you for them. Since this has now successfully been built in devel, it should be closed. Note also, however, bug #180177, which I just opened against mediawiki. |