Bug 177658 - Review Request: mediawiki - The PHP-based wiki software behind Wikipedia
Review Request: mediawiki - The PHP-based wiki software behind Wikipedia
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mike McGrath
David Lawrence
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-01-12 13:24 EST by Roozbeh Pournader
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
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 12:11:58 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Build log that resulted in an unstripped binary (4.69 KB, text/plain)
2006-01-14 16:05 EST, Mike McGrath
no flags Details

  None (edit)
Description Roozbeh Pournader 2006-01-12 13:24:37 EST
Spec Name or Url: http://guava.farsiweb.info/~roozbeh/mediawiki.spec
SRPM Name or Url: http://guava.farsiweb.info/~roozbeh/mediawiki-1.5.5-1.src.rpm
Description:
MediaWiki is the software used for Wikipedia (http://www.wikipedia.org) and
the other Wikimedia Foundation websites. Compared to other wikis, it has an
wide range of features and support for high-traffic websites using
multiple servers.
Comment 1 Mike McGrath 2006-01-13 10:33:22 EST
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?
Comment 2 Paul Howarth 2006-01-13 10:40:54 EST
(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.
Comment 3 Mike McGrath 2006-01-13 10:49:58 EST
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)
Comment 4 Ville Skyttä 2006-01-13 10:59:52 EST
(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).
Comment 5 Roozbeh Pournader 2006-01-14 11:24:32 EST
(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?
Comment 6 Mike McGrath 2006-01-14 16:05:14 EST
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.
Comment 7 Roozbeh Pournader 2006-01-15 07:04:41 EST
(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?
Comment 8 Roozbeh Pournader 2006-01-15 07:43:56 EST
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
Comment 9 Roozbeh Pournader 2006-01-31 09:54:54 EST
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@farsiweb.info> - 1.5.6-1
- Update to upstream 1.5.6
Comment 10 Mike McGrath 2006-01-31 11:34:34 EST
-%files math needs a %defattr
-%description change - "it has an wide" to "it has a wide"
Comment 11 Roozbeh Pournader 2006-01-31 12:02:11 EST
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@farsiweb.info> - 1.5.6-2
- Add %%defattr for -math subpackage
- Fixed typo in description
Comment 12 Mike McGrath 2006-01-31 22:49:02 EST
-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.  
Comment 13 Mike McGrath 2006-02-01 10:36:12 EST
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
Comment 14 Roozbeh Pournader 2006-02-02 08:56:53 EST
(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.
Comment 15 Roozbeh Pournader 2006-02-02 11:08:29 EST
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
Comment 16 Mike McGrath 2006-02-02 13:45:45 EST
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.
Comment 17 Roozbeh Pournader 2006-02-04 06:44:10 EST
(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)

Comment 18 Mike McGrath 2006-02-04 11:48:03 EST
-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
Comment 19 Roozbeh Pournader 2006-02-04 12:12:47 EST
(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.
Comment 20 Alex Lancaster 2006-02-06 11:53:34 EST
Since this has now successfully been built in devel, it should be closed.  Note
also, however, bug #180177, which I just opened against mediawiki.

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