Fedora Merge Review: mc http://cvs.fedora.redhat.com/viewcvs/devel/mc/ Initial Owner: jnovy
first shots: - package release tag - because your are using CVS snapshot, so you must comply with http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-cfd71146dbb6f00cec9fe3623ea619f843394837 - perhaps wrong usage of %{makeinstall} - details at http://fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002 - unowned dir - %{_libexecdir}/mc - mixed usage of spaces and tabs in the spec file, old entries in the changelog don't have escaped '%' in macros - explicit setting of --foodir=%{_foodir} when running the %configure macro, it is its function to set them :-)
Fixed. Thanks for the review. The only remaining rpmlint warning is about the mixed tab/spaces. For some reason rpmlint considers four spaces a tab, but two spaces are not. I consider this a bogus warning, as the iconv calls are better readable without tabs.
Could you, please, use the latest snapshot (http://www.ibiblio.org/pub/Linux/utils/file/managers/mc/snapshots/mc-2007-01-24-03.tar.gz) so I can check the checksum of the source file? The packaged version was deleted from the upstream location. There are still some warnings and errors from rpmlint, so we will have to check their relevancy yet.
Ok, I built the new mc with the latest CVS snapshot.
The full review is here and see the output of rpmlint at the end OK source files match upstream: 740d8b17463002c5bb3915841eb9abf936377c50375b410cb5d0640900ede8f3 mc-2007-01-24-03.tar.gz 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. OK license is open source-compatible. License text included in package. OK latest version is being packaged. OK BuildRequires are proper. OK compiler flags are appropriate. OK %clean is present. OK package builds in mock (i386). OK debuginfo package looks complete. OK final provides and requires looks sane 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, not content. 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. BAD rpmlint is not silent. I: mc-debuginfo checking I: mc checking W: mc incoherent-version-in-changelog 4.6.1a-42 1:4.6.1a-42.20070124cvs.fc7 minor :-) W: mc conffile-without-noreplace-flag /etc/mc/cedit.menu W: mc conffile-without-noreplace-flag /etc/mc/edit.indent.rc W: mc conffile-without-noreplace-flag /etc/mc/edit.spell.rc W: mc conffile-without-noreplace-flag /etc/mc/extfs/extfs.ini W: mc conffile-without-noreplace-flag /etc/mc/extfs/sfs.ini W: mc conffile-without-noreplace-flag /etc/mc/mc.charsets W: mc conffile-without-noreplace-flag /etc/mc/mc.ext W: mc conffile-without-noreplace-flag /etc/mc/mc.lib W: mc conffile-without-noreplace-flag /etc/mc/mc.menu W: mc conffile-without-noreplace-flag /etc/mc/syntax/Syntax at least mc.ext, mc.menu should be "noreplace", maybe all W: mc conffile-without-noreplace-flag /etc/profile.d/mc.csh W: mc conffile-without-noreplace-flag /etc/profile.d/mc.sh E: mc script-without-shebang /usr/share/mc/bin/mc-wrapper.sh E: mc script-without-shebang /usr/share/mc/bin/mc-wrapper.csh E: mc executable-marked-as-config-file /etc/profile.d/mc.sh E: mc executable-sourced-script /etc/profile.d/mc.sh 0755 E: mc non-executable-script /etc/mc/edit.spell.rc 0644 E: mc script-without-shebang /usr/share/mc/bin/mc.sh E: mc script-without-shebang /usr/share/mc/bin/mc.csh E: mc non-standard-uid /usr/libexec/mc/cons.saver vcsa E: mc setuid-binary /usr/libexec/mc/cons.saver vcsa 04711 E: mc non-standard-executable-perm /usr/libexec/mc/cons.saver 04711 E: mc non-standard-executable-perm /usr/libexec/mc/cons.saver 04711 E: mc executable-marked-as-config-file /etc/profile.d/mc.csh E: mc executable-sourced-script /etc/profile.d/mc.csh 0755 E: mc non-executable-script /etc/mc/edit.indent.rc 0644 Can you give some explanation for the issues above? I don't think they are real blockers, but would like to read your opinion.
(In reply to comment #5) > BAD rpmlint is not silent. > > W: mc conffile-without-noreplace-flag /etc/mc/cedit.menu > W: mc conffile-without-noreplace-flag /etc/mc/edit.indent.rc > W: mc conffile-without-noreplace-flag /etc/mc/edit.spell.rc > W: mc conffile-without-noreplace-flag /etc/mc/extfs/extfs.ini > W: mc conffile-without-noreplace-flag /etc/mc/extfs/sfs.ini > W: mc conffile-without-noreplace-flag /etc/mc/mc.charsets > W: mc conffile-without-noreplace-flag /etc/mc/mc.ext > W: mc conffile-without-noreplace-flag /etc/mc/mc.lib > W: mc conffile-without-noreplace-flag /etc/mc/mc.menu > W: mc conffile-without-noreplace-flag /etc/mc/syntax/Syntax > at least mc.ext, mc.menu should be "noreplace", maybe all Ok all of them are now noreplace except the mc.charsets and mc.lib -> I left those as config only. It's unlikely that anyone wants to change it. > W: mc conffile-without-noreplace-flag /etc/profile.d/mc.csh > W: mc conffile-without-noreplace-flag /etc/profile.d/mc.sh I removed the config flag from these. > E: mc script-without-shebang /usr/share/mc/bin/mc-wrapper.sh > E: mc script-without-shebang /usr/share/mc/bin/mc-wrapper.csh It's ok, because they are called by mc.sh and mc.csh via source. > E: mc executable-marked-as-config-file /etc/profile.d/mc.sh > E: mc executable-sourced-script /etc/profile.d/mc.sh 0755 > E: mc non-executable-script /etc/mc/edit.spell.rc 0644 > E: mc script-without-shebang /usr/share/mc/bin/mc.sh > E: mc script-without-shebang /usr/share/mc/bin/mc.csh > > E: mc non-standard-uid /usr/libexec/mc/cons.saver vcsa > E: mc setuid-binary /usr/libexec/mc/cons.saver vcsa 04711 > E: mc non-standard-executable-perm /usr/libexec/mc/cons.saver 04711 > E: mc non-standard-executable-perm /usr/libexec/mc/cons.saver 04711 The above is required to save console contents.
Now all problems were cleared, so this package is APPROVED.
Dan, thanks for your comments and review.
There are a couple of patches which alter the default upstream source. AFAIK the Fedora's policy against "new feature" patches is "send it upstream". Certainly, the patches for compatibility (i.e. big utf8 patch, which allows MC to be looked under UTF-8 locale the same way as upstream's MC is looked) or for scalability (new locales, new optional modules etc.) are allowed. But there are at least "mc-showfree" and "mc-utf8-look-and-feel" patches (and maybe even more) which do not match that criteria. Such kind of patches should first be sent upstream. It is exactly the upstream's decision whether apply them or not. Else you should fork the project with another name etc... Well, sorry for the words above, actually I'm not so formal. Since these two patches already present for a while, let's it be. But to be correct, some things are needed: - Config option to switch new "look-and-feel" on/off must be added - This option and already existing "showfree" option should not be enabled by default. In other words, the fresh installed Fedora's MC should be looked closely to vanilla upstream version. MC is one of "every day and every hour" applications for me. When the habitual "look and feel" suddenly changes, and there is no way to switch off, it is painful enough for users... P.S. The names of patches should normaly have form "mc-version-name.patch", where "version" points the first upstream version when the patch was appeared. If the upstream is actually cvs, then use cvs date for it. P.P.S. I hope it is not needed to re-open this bugzilla ticket? ;)
There is only one Fedora-specific mc patch now with 4.7.0.1 release. So I may safely close this one.