Bug 397211 - Review Request: redet - Regular expression development and execution tool
Review Request: redet - Regular expression development and execution tool
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 398261
  Show dependency treegraph
 
Reported: 2007-11-23 14:45 EST by Debarshi Ray
Modified: 2007-11-30 17:12 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-11-29 12:27:48 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
pertusus: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Debarshi Ray 2007-11-23 14:45:20 EST
Spec URL: http://rishi.fedorapeople.org/redet.spec
SRPM URL: http://rishi.fedorapeople.org/redet-8.23-1.fc8.src.rpm


Description:

Redet is a tool for developing and executing regular expressions using any of
more than 50 search programs, editors, and programming languages, intended
both for developing regular expressions for use elsewhere and as a search tool
in its own right. 

Additional features include persistent history, extensive help, a variety of
character entry tools, and the ability to change locale while running.
Comment 1 Debarshi Ray 2007-11-23 14:46:16 EST
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=255950

I inherited this package and since there has been a new upstream release since
it was last updated, I would like to pass this through a review.
Comment 2 Patrice Dumas 2007-11-23 16:25:36 EST
I think that in the make install line, INSTALL="%{__install} -p"
is not useful, and that you should add 
BINDIR=%{_bindir}

Also it seems to me that in the sed that sets the /usr/share path
it would be better to use the rpm macro, like

join %{_datadir} Redet

Strangely, nothing is installed in %{_datadir}/Redet?

Also since you try hard to keep timestamps, I think it would be better
to keep them despite the sed.


Also I would personally suggest to harcode the version in 
Patch0, such that it means the version since the patch 
was added or last changed.

Are the files Sample* useful? And FeatureList-groovy-en_us?
Maybe as %doc?

I think that xdg-open should be used instead of dillo as default 
browser, and added as a dependency.

It would be nice to provide the manual. It is even 
called from the program, from %{_datadir}/Redet/Manual/Manual.html
This would certainly mean using the redet-full-8.23.tar.bz2
tarball.

Comment 3 Debarshi Ray 2007-11-24 14:49:05 EST
(In reply to comment #2)

> I think that in the make install line, INSTALL="%{__install} -p"
> is not useful, and that you should add 
> BINDIR=%{_bindir}

BINDIR is not necessary since the Makefile sets it correctly using PREFIX. But
it does not do that with MANDIR, and hence it had to be set explicitly.

> Also it seems to me that in the sed that sets the /usr/share path
> it would be better to use the rpm macro, like

Ok.

> Strangely, nothing is installed in %{_datadir}/Redet?

Look below.

> Also since you try hard to keep timestamps, I think it would be better
> to keep them despite the sed.

How do I do that with sed? I am not very familiar with it. 

> I think that xdg-open should be used instead of dillo as default 
> browser, and added as a dependency.

Ok. I will fix this.

> It would be nice to provide the manual. It is even 
> called from the program, from %{_datadir}/Redet/Manual/Manual.html
> This would certainly mean using the redet-full-8.23.tar.bz2
> tarball.

The manual is 4.9M in size and was historically provided by the redet-doc
package. I have inherited it too, and will soon submit a review. Since the
manual is so big in size, I do not want to put it in the main package and burden
users with such big downloads.
Comment 4 Debarshi Ray 2007-11-24 15:45:47 EST
(In reply to comment #2)

> Are the files Sample* useful? And FeatureList-groovy-en_us?
> Maybe as %doc?

While the Sample* files are documentation files, FeatureList-groovy-en_us
contains the results of a set of tests used to determine the exact Groovy
features supported by Redet. So shall we still include it as %doc?
Comment 6 Patrice Dumas 2007-11-25 08:11:37 EST
(In reply to comment #3)
> (In reply to comment #2)
> 
> > I think that in the make install line, INSTALL="%{__install} -p"
> > is not useful, and that you should add 
> > BINDIR=%{_bindir}
> 
> BINDIR is not necessary since the Makefile sets it correctly using PREFIX. But
> it does not do that with MANDIR, and hence it had to be set explicitly.

I disagree, here. With the default fedora macros, %{_bindir}
is %{_prefix}/bin, redet uses PREFIX=%{_prefix} and appends bin,
so it works.

However, the macro that should be used for BINDIR is %{_bindir},
it is not %{_prefix}/bin. So BINDIR should be set to %{_bindir},
to cope with redefinitions of %{_bindir}.

> > Also since you try hard to keep timestamps, I think it would be better
> > to keep them despite the sed.
> 
> How do I do that with sed? I am not very familiar with it. 

I suggest something along:
sed --expression \
  's|set NonBinPath \[file join /usr local share Redet\];|set NonBinPath \[file
join /usr share Redet\];|' redet.tcl > redet.tcl.tmp
touch -r redet.tcl redet.tcl.tmp
mv redet.tcl.tmp redet.tcl


> > It would be nice to provide the manual. It is even 
> > called from the program, from %{_datadir}/Redet/Manual/Manual.html
> > This would certainly mean using the redet-full-8.23.tar.bz2
> > tarball.
> 
> The manual is 4.9M in size and was historically provided by the redet-doc
> package. I have inherited it too, and will soon submit a review. Since the
> manual is so big in size, I do not want to put it in the main package and burden
> users with such big downloads.

You are right. However, since it is quite hard to understand the tool
without the manual, I suggest mentionning the redet-doc package
in the %description, like

The documentation for this package is in %{name}-doc.


Also I suggest hardcoding the version in Patch0 to show that this
was the version the patch was introduced in.
Comment 8 Patrice Dumas 2007-11-25 17:42:38 EST
%{_datadir} is still not used 
I suggest:
  's|set NonBinPath \[file join /usr local share Redet\];|set NonBinPath \[file
join %{_datadir} Redet\];|' redet.tcl > redet.tcl.tmp

I also suggest using
%{_mandir}/man1/%{name}.1*

In any case, 

* follow guidelines
* upstream source
09812846ab653dacbb0adf2a61927fbf  redet-8.23.tar.gz
* use rpm macros
* %files section right

APPROVED
Comment 9 Debarshi Ray 2007-11-26 02:51:17 EST
(In reply to comment #8)

> %{_datadir} is still not used 
> I suggest:
>   's|set NonBinPath \[file join /usr local share Redet\];|set NonBinPath \[file
> join %{_datadir} Redet\];|' redet.tcl > redet.tcl.tmp

Oops. Sorry, I missed that. Will fix it.
Comment 10 Debarshi Ray 2007-11-26 12:00:26 EST
New Package CVS Request
=======================
Package Name: redet
Short Description: Regular expression development and execution tool
Owners: rishi
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: no
Comment 11 Kevin Fenzi 2007-11-26 12:27:02 EST
Not sure that there is any cvsadmin work that needs doing here. 
You are already listed as owner on those branches. 

Feel free to reset the flag if you need anything else. 

Additionally, would you also consider maintaining the EL-4/EL-5 EPEL branches of
this package (which are listed as orphaned still)?

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