[Bug 682666] Review Request: DeTex - A program to remove TeX constructs from a text file

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=682666

Mohamed El Morabity <pikachu.2014 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |pikachu.2014 at gmail.com

--- Comment #2 from Mohamed El Morabity <pikachu.2014 at gmail.com> 2011-04-14 21:19:40 EDT ---

Since I *really* need this tool, I'd be glad to review it.


Some comments:

* your patch could be *really* simplified:
   - the flex package provides also lex, which is a symbolic link to flex ;
   - in the same way, libl.a is provided by the flex-static package and is a
symbolic link to libfl.a
   As a result, modifying the LEX and LEXLIB variables in your patch is
useless.
  You could even skip the « make install ... » instruction in %install in your
.spec, and manually install the binary and the man page, like below:
  %install
  rm -rf $RPM_BUILD_ROOT
  install -Dpm 755 detex $RPM_BUILD_ROOT%{_bindir}/detex
  install -Dpm 644 detex.1l $RPM_BUILD_ROOT%{_mandir}/man1/detex.1l
(notice the -p option of install to preserve timestamp during installation).

* detex is not compiled using the Fedora standard flags ($RPM_OPT_FLAGS). It
appears clearly in compilation logs. Moreover the generated *-debug package is
unusable. You must set the CFLAGS when calling make:
   %build
   make CFLAGS="$RPM_OPT_FLAGS"
You can even add to CFLAGS the « -DNO_MALLOC_DECL » option you enabled on your
patch; by this way, the patch is useless and can be removed from your package:
   %build
   make CFLAGS="$RPM_OPT_FLAGS -DNO_MALLOC_DECL"

* The 1l section for man pages is sometimes intended for programs installed in
/usr/local. This may be the reason for the man page to be suffixed « 1l ».
Whatever the explanation, it's not correct. I suggest you to rename the man
page to « detex.1 »: in %install, simply:
  %install
  rm -rf $RPM_BUILD_ROOT
  install -Dpm 755 detex $RPM_BUILD_ROOT%{_bindir}/detex
  install -Dpm 644 detex.1l $RPM_BUILD_ROOT%{_mandir}/man1/detex.1
(dont forget to fix it also in %files section of your .spec).
As a result, you should also patch the man page and replace each occurence of «
1L » to « 1 ».

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.