Bug 193632 - Review Request: tkdnd
Review Request: tkdnd
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Wart
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-05-31 03:12 EDT by Sander Hoentjen
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-14 16:07:47 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 Sander Hoentjen 2006-05-31 03:12:33 EDT
Spec URL: http://amsn.hoentjen.eu/download/tkdnd.spec
SRPM URL: http://amsn.hoentjen.eu/download/tkdnd-1.0a2-1.src.rpm
Description: Tk extension that adds native drag & drop capabilities

I currently have an rpmlint error:
W: tkdnd unstripped-binary-or-object /usr/lib/tkdnd/libtkdnd.so
How can I fix this?
Comment 1 Paul Howarth 2006-05-31 05:24:09 EDT
(In reply to comment #0)
> Spec URL: http://amsn.hoentjen.eu/download/tkdnd.spec
> SRPM URL: http://amsn.hoentjen.eu/download/tkdnd-1.0a2-1.src.rpm
> Description: Tk extension that adds native drag & drop capabilities
> 
> I currently have an rpmlint error:
> W: tkdnd unstripped-binary-or-object /usr/lib/tkdnd/libtkdnd.so

The problem is caused by the .so file being installed without executable
permissions.

> How can I fix this?

Use:
make install INSTALL_DATA="install -p"
instead of just:
make install
which uses a default for INSTALL_DATA of "/usr/bin/install -c -m 644"

BuildReq tcl-devel isn't needed here because tk-devel depends on it by the way.
Comment 2 Sander Hoentjen 2006-05-31 06:59:56 EDT
(In reply to comment #1)

> > How can I fix this?
> 
> Use:
> make install INSTALL_DATA="install -p"
> instead of just:
> make install
> which uses a default for INSTALL_DATA of "/usr/bin/install -c -m 644"
> 
thanks, fixed

> BuildReq tcl-devel isn't needed here because tk-devel depends on it by the way.
> 
fixed, thanks for your quick comments

Spec URL: http://amsn.hoentjen.eu/download/tkdnd.spec
SRPM URL: http://amsn.hoentjen.eu/download/tkdnd-1.0a2-2.src.rpm
Comment 3 Wart 2006-05-31 10:23:54 EDT
Doesn't build on x86_64.  It looks like this will need the same tcl.m4 fix that
was used for tcltls (adding $libdir to the path where tclConfig.sh is found)
Comment 4 Wart 2006-05-31 19:01:52 EDT
The --libdir argument to configure is necessary due to a poorly written
mkIndex.tcl script.  In addition, upstream seems to have decided to include a
prebuilt dll file in the source tarball.  Naughty upstream!

If you remove the lib/tkdnd/*.dll in %prep and use the sed script below in
%build, then you don't need to pass --libdir in %configure.  This addresses the
problem directly instead of indirectly through --libdir.

%{__sed} -i -e 's#libtkdnd.dll ##' \
    -e "s#^set libdir.*#set libdir $RPM_BUILD_ROOT%{_libdir}#" \
    mkIndex.tcl.in

You should also pass DESTDIR to make install:

make install DESTDIR=$RPM_BUILD_ROOT INSTALL_DATA="install -p"
Comment 5 Sander Hoentjen 2006-06-01 09:25:36 EDT
Spec URL: http://amsn.hoentjen.eu/download/tkdnd.spec
SRPM URL: http://amsn.hoentjen.eu/download/tkdnd-1.0a2-3.src.rpm

This has all Wart's fixes. I guess 64bit works now, unfortunatly I can't test that.
Comment 6 Wart 2006-06-01 11:27:01 EDT
(In reply to comment #5)
> I guess 64bit works now, unfortunatly I can't test that.

Almost.  I overlooked the fact that tcl.m4 needs to be modified to look in the
extra directories for tkConfig.sh, too.

Builds in mock look good.  I'll start a formal review shortly.
Comment 7 Sander Hoentjen 2006-06-02 04:47:34 EDT
Spec URL: http://amsn.hoentjen.eu/download/tkdnd.spec
SRPM URL: http://amsn.hoentjen.eu/download/tkdnd-1.0a2-4.src.rpm

I guess this should fix the 64bit issue. Soon I will have enough money for my
own 64bit box :P
Comment 8 Wart 2006-06-02 12:00:15 EDT
Almost there...

It seems that there is a hardcoded "../lib/" path in the Makefile that still
breaks on x86_64 during make install.  Since this package only really installs 3
files, it's probably easier to just drop all of these install-file sed commands
and install everything by hand.  Here's what I did in %install to do this:

%install
cd unix
rm -rf $RPM_BUILD_ROOT
mkdir -p $RPM_BUILD_ROOT/%{_libdir}/%{name}-%{version}
install -p -m 0755 ../lib/tkdnd/*.so $RPM_BUILD_ROOT/%{_libdir}/%{name}-%{version}
install -p -m 0644 ../lib/tkdnd/*.tcl $RPM_BUILD_ROOT/%{_libdir}/%{name}-%{version}

Note that I added %{version} to the installed directory name, just in case you
ever want to have multiple versions installed at the same time.  The %files
section will have to be updated accordingly.  You can also drop the sed command
on mkIndex.tcl, since it won't be needed anymore.
Comment 9 Sander Hoentjen 2006-06-08 06:28:31 EDT
(In reply to comment #8)
Done.
Spec URL: http://amsn.hoentjen.eu/download/tkdnd.spec
SRPM URL: http://amsn.hoentjen.eu/download/tkdnd-1.0a2-5.src.rpm

about %{version}: It might be a good idea since after 4 years of no updates
there is work being done on tkdnd 2
Thanks again for the help
Comment 10 Wart 2006-06-08 11:00:09 EDT
Looks good!  Only two (nitpicking) minor issues:

MUST
====
* Source matches upstream:
  43c91da595aade4978e2e5e820ab0fc9  tkdnd-1.0a2.tar.gz
* rpmlint output clean
* Spec file legible and in Am. English
* No excessive BuildRequires:
* BSD license ok, license file included
* No .desktop file needed
* No -devel subpackage needed
* No need for -docs subpackage
* No duplicate %files
* Permissions look ok

MUSTFIX
=======
* Remove the extra directory in %doc by adding a wildcard:
%doc doc/*

* Move the "rm -rf $RPM_BUILD_ROOT" to the very first line in %install 

SHOULD
======
* You need BuildRequires: xorg-x11-devel if you plan to build on FC4.  You
  can do this by either forking the spec files in CVS, or adding the following
  to the current spec file:
%if "%fedora" <= "4"
BuildRequires:  xorg-x11-devel
%else
BuildRequires:  libXext-devel
%endif

* An alpha release of version 2.0 is available.  Have you considered upgrading,
  or is the alpha version not stable enough?

* Notify upstream about the 64-bit build issues and send them the patch.
Comment 11 Sander Hoentjen 2006-06-08 12:39:59 EDT
(In reply to comment #10)
> Looks good!  Only two (nitpicking) minor issues:
> 

> MUSTFIX
> =======
> * Remove the extra directory in %doc by adding a wildcard:
> %doc doc/*
Yes I saw this myself a while ago but forgot to fix it, fixed now.
> 
> * Move the "rm -rf $RPM_BUILD_ROOT" to the very first line in %install 
whoops, fixed
> 
> SHOULD
> ======
> * You need BuildRequires: xorg-x11-devel if you plan to build on FC4.  You
>   can do this by either forking the spec files in CVS, or adding the following
>   to the current spec file:
> %if "%fedora" <= "4"
> BuildRequires:  xorg-x11-devel
> %else
> BuildRequires:  libXext-devel
> %endif

Should I build on FC4? If yes, I think I will go for a fork.

> 
> * An alpha release of version 2.0 is available.  Have you considered upgrading,
>   or is the alpha version not stable enough?

I considered, but the alpha version is drop only, no drag yet.
> 
> * Notify upstream about the 64-bit build issues and send them the patch.
> 
I think this makes more sence for 2.0 only since i don't think they will release
another 1.x. I will check if 2.0 builds ok for 64-bit after I get my new
computer (tomorrow).

Spec URL: http://amsn.hoentjen.eu/download/tkdnd.spec
SRPM URL: http://amsn.hoentjen.eu/download/tkdnd-1.0a2-6.src.rpm

Comment 12 Wart 2006-06-08 14:44:35 EDT
> > SHOULD
> > ======
> > * You need BuildRequires: xorg-x11-devel if you plan to build on FC4.  You
> >   can do this by either forking the spec files in CVS, or adding the following
> >   to the current spec file:
> > %if "%fedora" <= "4"
> > BuildRequires:  xorg-x11-devel
> > %else
> > BuildRequires:  libXext-devel
> > %endif
> 
> Should I build on FC4? If yes, I think I will go for a fork.

It's up to you either way.  There's no requirement for you to build on FC4, and
you can always decide to do it later if you wish.
 
> > * An alpha release of version 2.0 is available.  Have you considered upgrading,
> >   or is the alpha version not stable enough?
> 
> I considered, but the alpha version is drop only, no drag yet.

Ok.

> > * Notify upstream about the 64-bit build issues and send them the patch.
> > 
> I think this makes more sence for 2.0 only since i don't think they will release
> another 1.x. I will check if 2.0 builds ok for 64-bit after I get my new
> computer (tomorrow).

That's understandable.  If you want to support both 1.0 and 2.0 being installed
simultaneously then you'll probably want to name the new package "tkdnd2",
otherwise yum will only let you have one installed at a time.

> Spec URL: http://amsn.hoentjen.eu/download/tkdnd.spec
> SRPM URL: http://amsn.hoentjen.eu/download/tkdnd-1.0a2-6.src.rpm

All MUSTFIX items have been fixed.

APPROVED
Comment 13 Wart 2006-06-14 16:07:47 EDT
Re-closing due to recent Bugzilla data loss.

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