Bug 189374 - Re-Review Request: jed: an editor
Re-Review Request: jed: an editor
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-19 11:51 EDT by Bill Nottingham
Modified: 2014-03-16 22:59 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-06 11:03:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Bill Nottingham 2006-04-19 11:51:16 EDT
Spec URL: http://cvs.fedora.redhat.com/viewcvs/*checkout*/rpms/jed/devel/jed.spec?root=extras<spec SRPM URL: Check it out, do 'make srpm'
Description: the Jed editor

So, when this was first imported into Extras, it got a free pass as it was moved from Core. I'd like, if possible, to get it reviewed to avoid future problems.

Issues will most likely just be fixed directly in CVS.
Comment 1 Michael Schwendt 2006-04-24 05:24:10 EDT
 From man jed:

| FILES
|       JED_ROOT/lib/*.sl
|              these are the default runtime jed slang files
|       JED_ROOT/lib/site.sl
|              This is the default startup file.

Here JED_ROOT could be substituted with the expanded %{_datadir}/jed
since the value of $JED_ROOT is not explained in the manual page.

|       /etc/jed.rc
|              The system wide configuration file.
|       ~/.jedrc
|              Per user configuration file.

Can you get the system wide configuration file to work? I believe
this has never worked before in Red Hat Linux and later (with a
corresponding ticket in bugzilla). Steps to reproduce:

  $ sudo cp /usr/share/jed/lib/jed.conf /etc/jed.rc
  $ rm -f $HOME/.jedrc
  $ jed

gives different results compared with

  $ sudo rm /etc/jed.rc
  $ cp -f /usr/share/jed/lib/jed.conf $HOME/.jedrc
  $ jed

because the system wide file from /etc/jed.rc is not loaded at all.
The code loads 'Default_Jedrc_Startup_File' = $JED_ROOT/lib/jed.rc.

Copying jed.conf (not jed.rc) as the new system wide default should
give a different (blue) theme. It doesn't.
Comment 2 Bill Nottingham 2006-05-09 23:12:19 EDT
/etc/jed.rc and JED_ROOT in man page fixed in 0.99.18-2, in CVS.
Comment 3 Jason Tibbitts 2006-06-02 13:57:13 EDT
Just did a quick mock build (development, x86_64).  rpmlint is unhappy:

W: jed prereq-use /sbin/install-info

I see the Prereq: but I don't see any calls to install-info.  The scriptlets at
http://fedoraproject.org/wiki/ScriptletSnippets?highlight=%28scriptlets%29#head-117e9450bc166ceb4251bf8d87a9dd4e862442a4
should be used to install info pages; Prereq: should not be used in any case;
Requires(pre): is preferred.

W: jed buildprereq-use slang-devel >= 2.0, autoconf, libselinux-devel

BuildRequires: should be used instead.

E: jed broken-syntax-in-scriptlet-requires BuildPrereq: slang-devel >= 2.0,
autoconf, libselinux-devel

Not sure what's up here; it will probably go away when you switch to BuildRequires:

W: jed hardcoded-path-in-buildroot-tag /var/tmp/%{name}-%{version}-%{release}-root

Please use %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
as the buildroot.

W: jed patch-not-applied Patch3: jed-multilib.patch

I assume you have reasons for not applying patch3.

E: jed obsolete-not-provided jed-common
E: jed obsolete-not-provided jed-xjed

You should provide these so that upgrades work and so that any packages which
might depend on them don't break.

E: jed binary-or-shlib-defines-rpath /usr/bin/jed ['/usr/lib64']

rpath is bad and should be removed if at all possible.  I'll dig into this
further if necessary.

W: jed doc-file-dependency /usr/share/doc/jed-0.99.18/doc/tm/rtl/tm-sort.sl
/usr/bin/env
W: jed doc-file-dependency /usr/share/doc/jed-0.99.18/doc/tm/rtl/whatelse.sl
/usr/bin/env

Documentation should not be executable.
Comment 4 Bill Nottingham 2006-06-02 21:53:20 EDT
(In reply to comment #3)
> Just did a quick mock build (development, x86_64).  rpmlint is unhappy:
> 
> W: jed prereq-use /sbin/install-info

* Tue May  4 2004 Bill Nottingham <notting@redhat.com> 0.99.16-4
- remove info page (#115826)

Oops.

> W: jed buildprereq-use slang-devel >= 2.0, autoconf, libselinux-devel
> 
> BuildRequires: should be used instead.

Syntax changed.

> W: jed hardcoded-path-in-buildroot-tag /var/tmp/%{name}-%{version}-%{release}-root
> 
> Please use %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> as the buildroot.

Fixed.

> W: jed patch-not-applied Patch3: jed-multilib.patch
> 
> I assume you have reasons for not applying patch3.

Didn't apply, forgot to remove it from the spec.

> E: jed obsolete-not-provided jed-common
> E: jed obsolete-not-provided jed-xjed
> 
> You should provide these so that upgrades work and so that any packages which
> might depend on them don't break.

These were obsoleted in 2002. Nothing should require them. (And, technically,
it doens't really obsolete xjed - we just stopped building it because it's
 a bad idea.)


> E: jed binary-or-shlib-defines-rpath /usr/bin/jed ['/usr/lib64']
> 
> rpath is bad and should be removed if at all possible.  I'll dig into this
> further if necessary.

Nah, I'll dig it out. Probably bad autoconf somewhere.

> W: jed doc-file-dependency /usr/share/doc/jed-0.99.18/doc/tm/rtl/tm-sort.sl
> /usr/bin/env
> W: jed doc-file-dependency /usr/share/doc/jed-0.99.18/doc/tm/rtl/whatelse.sl
> /usr/bin/env
> 
> Documentation should not be executable.

Fixed.

Comment 5 Jason Tibbitts 2006-06-03 21:23:37 EDT
(In reply to comment #4)
> > W: jed prereq-use /sbin/install-info

Still seems to be there in -3.

[obsolete-not-provided]
> These were obsoleted in 2002. Nothing should require them. (And, technically,
> it doens't really obsolete xjed - we just stopped building it because it's
>  a bad idea.)

Maybe they should just disappear.

> Nah, I'll dig it out. Probably bad autoconf somewhere.

I did check that the usual make LIBTOOL=/usr/bin/libtool bit doesn't work.

By the way, could you add %{?_smp_mflags} to the make line?  I love my parallel
make.
Comment 6 Bill Nottingham 2006-06-05 17:13:51 EDT
All fixed.
Comment 7 Jason Tibbitts 2006-06-05 22:01:32 EDT
Everything looks good, execpt that you don't use the dist tag.  (Sorry I didn't
notice it earlier; that's why I always run through my checklist.)  It's not an
absolute requirement but it's very strongly recommended.

If you don't mind my asking, how did you get rid of rpath?  I assume it's in the
multilib patch; is it the third hunk?

Since the only issue is the dist tag I'll go ahead and approve.  Normally I'd
say you can fix it when you check in, but it's already checked in, so I suppose
you can fix it at your leisure.

Review:
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
X dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* source files match upstream:
   5378c8e7805854018d9ec5c3cfadf637  jed-0.99-18.tar.bz2
   5378c8e7805854018d9ec5c3cfadf637  jed-0.99-18.tar.bz2-srpm
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock (development, x86_64).
* rpmlint is silent.
* final provides and requires are sane:
   jed-common
   jed-xjed
   jed = 0.99.18-4
  =
   libselinux.so.1()(64bit)
   libslang.so.2()(64bit)
   libslang.so.2(SLANG2)(64bit)
   libutil.so.1()(64bit)
   libutil.so.1(GLIBC_2.2.5)(64bit)
* no shared libraries are present.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* %clean is present.
* %check is not present; no test suite upstream
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.
* not a GUI app.

APPROVED
Comment 8 Bill Nottingham 2006-06-06 10:19:10 EDT
Re: dist tag, I've just not used it in the past because I'm not a
build-every-release-for-every-reelase person. I suppose it could be added; the
package itself is only really backportable to FC5 (due to slang2 requirements).

As to the rpath, there is stuff in the aclocal.m4 that it includes in configure
that adds rpath if slang is in (what it determines to be) a nonstandard directory.
The bits were only checking against /lib | /usr/lib | /usr/local/lib ; by
replacing /lib with /lib64 on arches that use that, it allows the check to
succeed. (Arguably, this shouldn't be checked in the package at all, but doing
this sort of patch is the path of least resistance, since I think upstream is
doing this check & rpathing for some Solaris stuff.)
Comment 9 Bill Nottingham 2006-06-06 11:03:28 EDT
Built.

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