Bug 226133 - Merge Review: mc
Merge Review: mc
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jindrich Novy
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:39 EST by Nobody's working on this, feel free to take it
Modified: 2013-07-02 19:19 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-01-29 08:48:29 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
dan: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:39:22 EST
Fedora Merge Review: mc

http://cvs.fedora.redhat.com/viewcvs/devel/mc/
Initial Owner: jnovy@redhat.com
Comment 1 Dan Horák 2007-02-03 15:36:07 EST
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 :-)
Comment 2 Jindrich Novy 2007-02-07 11:26:59 EST
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.
Comment 3 Dan Horák 2007-02-09 04:01:19 EST
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.
Comment 4 Jindrich Novy 2007-02-09 07:14:06 EST
Ok, I built the new mc with the latest CVS snapshot.
Comment 5 Dan Horák 2007-02-13 09:39:51 EST
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.
Comment 6 Jindrich Novy 2007-02-15 09:00:50 EST
(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.

Comment 7 Dan Horák 2007-02-16 04:32:44 EST
Now all problems were cleared, so this package is APPROVED.
Comment 8 Jindrich Novy 2007-02-16 04:35:46 EST
Dan, thanks for your comments and review.
Comment 9 Dmitry Butskoy 2007-02-22 08:52:38 EST
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? ;)
Comment 10 Jindrich Novy 2010-01-29 08:48:29 EST
There is only one Fedora-specific mc patch now with 4.7.0.1 release. So I may safely close this one.

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