Bug 186327 - Review Request: tcltls
Review Request: tcltls
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
: Reopened
Depends On:
Blocks: FE-ACCEPT 185951
  Show dependency treegraph
 
Reported: 2006-03-22 17:27 EST by Sander Hoentjen
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-29 10:43:54 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)
protocoll from make test (6.89 KB, text/plain)
2006-03-27 11:28 EST, Jochen Schmitt
no flags Details
Added patch for configure script (1.06 KB, patch)
2006-03-28 15:25 EST, Wart
no flags Details | Diff
look for tclConfig.sh in %{_libdir} (1.16 KB, patch)
2006-03-28 15:27 EST, Wart
no flags Details | Diff
Add $RPM_OPT_FLAGS to compile line (403 bytes, patch)
2006-03-28 15:30 EST, Wart
no flags Details | Diff

  None (edit)
Description Sander Hoentjen 2006-03-22 17:27:45 EST
Spec Name or Url: http://amsn.sourceforge.net/tjikkun/tcltls.spec
SRPM Name or Url: http://amsn.sourceforge.net/tjikkun/tcltls-1.5.0-1.src.rpm
Description: The TLS OpenSSL extension to Tcl
Comment 1 Jochen Schmitt 2006-03-23 11:48:23 EST
Good:

+ Local build worked fine.
+ Mock build worked fine.

Bad:
- fedora-qa complaints:
   tls1.5.0-src.tar.gz                                              11 kB 00:00
   Source tls1.5.0-src.tar.gz is different from upstream
- Package doesn't contains any documentation.
- Package doesn't contain verbarin license text.
- rpmlint compiaints:
   E: tcltls invalid-soname /usr/lib/libtls1.50.so libtls1.50.so
- fedora-qa complaints.
   Each %files section should have a %defattr line
   (wiki: Packaging/ReviewGuidelines)
Comment 2 Sander Hoentjen 2006-03-24 03:56:40 EST
-Source tls1.5.0-src.tar.gz is different from upstream
i checked and both are 113600 in size, and have MD5SUM of:
9eeab472475773b3810acc808ebec759
but sometimes the sf.net has a failed-mirror and the the download is not correct.
Or maybe I am doing something wrong, I am quite new at packaging.
The rest I have fixed.
Thank you for taking the time to review.

Spec Name or Url: http://amsn.sourceforge.net/tjikkun/tcltls.spec
SRPM Name or Url: http://amsn.sourceforge.net/tjikkun/tcltls-1.5.0-2.src.rpm


Comment 3 Jochen Schmitt 2006-03-27 11:28:29 EST
Created attachment 126831 [details]
protocoll from make test
Comment 4 Jochen Schmitt 2006-03-27 11:29:27 EST
Good:
* RPM name is OK
* Source tls1.5.0-src.tar.gz is the same as upstream
* Builds fine in mock
* rpmlint of tcltls looks OK
* rpmlint of tcltls-devel looks OK
* File list of tcltls looks OK
* File list of tcltls-devel looks OK
* INSERT RESULT OF RUN TEST

Bad:
- debuginfo RPM contains no source files.
- make test fails please see attached file.
Comment 5 Sander Hoentjen 2006-03-27 14:11:12 EST
- debuginfo RPM contains no source files.
How can I make it contain them? I searched for information but could not find
anything. Is it automatically generated with /usr/lib/rpm/find-debuginfo.sh ?
If so what can I do about it?
- make test fails please see attached file.
I fixed this but will upload when I know about the other problem.

Once again thank you very much for taking the time for reviewing.
Comment 6 Jochen Schmitt 2006-03-27 14:35:05 EST
Add the -g flag during compiliation.

Don't stript the executables.
Comment 7 Sander Hoentjen 2006-03-28 01:52:27 EST
Spec Name or Url: http://amsn.sourceforge.net/tjikkun/tcltls.spec
SRPM Name or Url: http://amsn.sourceforge.net/tjikkun/tcltls-1.5.0-3.src.rpm

I *think* everything is ok now, but you'll probably find something :)
Comment 8 Jochen Schmitt 2006-03-28 10:27:51 EST
Sorry:

Local build coused:
_BLKSIZE=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGNED_CHAR=1 -DHAVE_LANGINFO=1
-DHAVE_SYS_IOCTL_H=1       -I"/usr/include" -I"/usr/include"    -g -O2  -fPIC -c
`echo ./fixstrtod.c` -o fixstrtod.o
rm -f libtls-1.50g.so
cc -shared -Wl,-soname,libtls-1.50g.so -o libtls-1.50g.so tls.o tlsIO.o tlsBIO.o
tlsX509.o fixstrtod.o  -L/usr/lib -ltclstub8.4g -L/usr/lib -lssl -L/usr/lib -lcrypto
/usr/bin/ld: cannot find -ltclstub8.4g
collect2: ld returned 1 exit status
make: *** [libtls-1.50g.so] Error 1
Fehler: Bad exit status from /var/tmp/rpm-tmp.63113 (%build)
Comment 9 Sander Hoentjen 2006-03-28 10:39:21 EST
Now where does that g come from? I don't understand.
Comment 10 Wart 2006-03-28 10:55:44 EST
(In reply to comment #9)
> Now where does that g come from? I don't understand.

It comes from the --enable-symbols flag that you pass to configure.  You might
want to consider updating the tcl.m4 in this package with the latest one from
the tclconfig module in Tcl's cvs repository.  That will also fix the build
errors on x86_64 (it doesn't look for tclConfig.sh in /usr/lib64).
Comment 11 Ralf Corsepius 2006-03-28 11:09:06 EST
(In reply to comment #8)
> _BLKSIZE=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGNED_CHAR=1 -DHAVE_LANGINFO=1
> -DHAVE_SYS_IOCTL_H=1       -I"/usr/include" -I"/usr/include"    -g -O2  -fPIC -c
> `echo ./fixstrtod.c` -o fixstrtod.o
> rm -f libtls-1.50g.so
> cc -shared -Wl,-soname,libtls-1.50g.so -o libtls-1.50g.so tls.o tlsIO.o tlsBIO.o
> tlsX509.o fixstrtod.o  -L/usr/lib -ltclstub8.4g -L/usr/lib -lssl -L/usr/lib
-lcrypto

This log exposes further issues:
* -I/usr/include and -L/usr/lib almost never are correct
* The compiler calls above don't honor RPM_OPT_FLAGS.
Comment 12 Wart 2006-03-28 12:19:26 EST
(In reply to comment #11)
> This log exposes further issues:
> * -I/usr/include and -L/usr/lib almost never are correct

These are picked up from the tclConfig.sh build file in the Core Tcl package. 
It should be fixed in Core.

> * The compiler calls above don't honor RPM_OPT_FLAGS.

It seems that the extension is ignoring TCL_EXTRA_CFLAGS from tclConfig.sh,
which is where RPM_OPT_FLAGS is stored from the build of Tcl.  This is supposed
to be picked up automatically, but isn't here.  You can work around it by adding
it to CFLAGS_DEFAULT in the Makefile:
CFLAGS_DEFAULT = @CFLAGS_DEFAULT@ @TCL_EXTRA_CFLAGS@
Comment 13 Wart 2006-03-28 15:07:06 EST
(In reply to comment #1)
> - rpmlint compiaints:
>    E: tcltls invalid-soname /usr/lib/libtls1.50.so libtls1.50.so

This is a dynamically loaded extension to Tcl, not a shared library for linking
against other applications.  IMHO, it should be moved out of %{_libdir} and into
its own %{_libdir}/tls1.50 directory, with the pkgIndex.tcl file updated
accordingly, as is done with some other Tcl extensions in FE.

Let me know if you want some help with this and the other build issues.  I've
packaged quite a number of Tcl extensions for FE and have come across most of
these problems before.
Comment 14 Wart 2006-03-28 15:25:34 EST
Created attachment 126929 [details]
Added patch for configure script

Added the diff for the configure script so that you don't need to run autoconf
in the spec file.
Comment 15 Wart 2006-03-28 15:27:38 EST
Created attachment 126930 [details]
look for tclConfig.sh in %{_libdir}

Modified tcl.m4 and configure script to look for tclConfig.sh in '--libdir' and
 /usr/lib64 on x86_64.	This fixes the build error on x86_64.
Comment 16 Wart 2006-03-28 15:30:28 EST
Created attachment 126931 [details]
Add $RPM_OPT_FLAGS to compile line

Updates Makefile.in to add $TCL_EXTRA_CFLAGS from tclConfig.sh to the compile
line.  TCL_EXTRA_CFLAGS contains the $RPM_OPT_FLAGS value from the build of Tcl
in Fedora Core.  This removes the need for the tcltls-1.5-tclm4-debugoptimize
patch.
Comment 17 Sander Hoentjen 2006-03-30 02:13:59 EST
Wart, thank you very much for all your posts! At the moment I am really busy at
work but I will try to use the patches as soon as possible. This is (together
with amsn) the first rpm i am building so I have a lot to learn (and thanks to
everybody posting I have learned a lot).
Comment 18 Wart 2006-03-30 12:18:51 EST
(In reply to comment #17)
> Wart, thank you very much for all your posts! At the moment I am really busy at
> work but I will try to use the patches as soon as possible. This is (together
> with amsn) the first rpm i am building so I have a lot to learn (and thanks to
> everybody posting I have learned a lot).

No problem.  I had actually considered submitting this one myself for tclhttpd
(to get https support), but got distracted by other packages.
Comment 19 Sander Hoentjen 2006-04-24 10:19:25 EDT
Finally I had some time again, so here they are:
http://amsn.hoentjen.eu/download/tcltls.spec
http://amsn.hoentjen.eu/download/tcltls-1.5.0-5.src.rpm

I put in all Wart's changes and moved the libtls so into the tls1.5 subdir. I
hope I got it right now,

Sander
Comment 20 Wart 2006-04-24 12:15:30 EDT
You still need to remove '--enable-symbols' from the %configure statement so
that it doesn't try to add the 'g' in -ltclstub8.4g.

It would also be good, as Jochen said, to include the tests as part of the
build.  Just add the following to the spec file:

%check
make test

And as noted, there are three failing tests.  The first two look like they might
be due to the use of a different openssl version from when the tests were
written.  The third should be posted upstream to see if it's a real problem or not.
Comment 21 Sander Hoentjen 2006-04-24 13:30:48 EDT
Hum, i must have messed something up because it did have that before, i had the
patch ready to go:
http://amsn.hoentjen.eu/download/tcltls.spec
http://amsn.hoentjen.eu/download/tcltls-1.5.0-6.src.rpm

it is ok on my machine, now i am also trying it with mock, but that will take a
while.
Comment 22 Jochen Schmitt 2006-04-24 15:04:58 EDT
God:
+ Mock build works fine.

Bad:
- Source tls1.5.0-src.tar.gz is different from upstream
Comment 23 Sander Hoentjen 2006-04-24 15:21:52 EDT
ok now i have one with correct MD5SUM

http://amsn.hoentjen.eu/download/tcltls-1.5.0-7.src.rpm
Comment 24 Wart 2006-04-24 19:02:17 EDT
This latest rpm works great with tclhttpd.  I can verify that this package is
functional.
Comment 25 Jochen Schmitt 2006-04-25 10:04:56 EDT
sorry, but the tar.gz file in the source RPM doesn't match with the upstream
version.

fedora-qa tcltls-1.5.0-7.src.rpm
Starting QA for local srpm tcltls-1.5.0-7.src.rpm
Checking tcltls, version 1.5.0, release 7...
Are the name, version and release correct according to Fedora's package naming
guidelines ([y]/n)
y
View the last spec diff ? ([y]/n)
n
Checking source 0
Source tls1.5.0-src.tar.gz is different from upstream
  (wiki: QAChecklist item 2)
Patches found: tcltls-configurein.pkgname.patch, tcltls-1.5-rpmoptflags.patch,
tcltls-1.5-64bit.patch, tcltls-makefilein-soname.patch,
tcltls-tclm4-soname.patch, tcltls-1.5-relpath.patch,
tcltls-1.5-pkgindex-version.patch, tcltls-1.5-ciphertest-KRB5.patch.
Look at the patches ? ([y]/n)
Interrupted by user
Comment 26 Wart 2006-04-25 11:01:56 EDT
I have the following md5sum from both upstream and the latest src rpm:

9eeab472475773b3810acc808ebec759  tls1.5.0-src.tar.gz

However, the Source0: tag can't be used directly to get the sources due to the
sourceforge download redirection.  Try using "dl.sourceforge.net" instead of
"download.sourceforge.net".
Comment 27 Sander Hoentjen 2006-04-25 12:45:35 EDT
ok, as you said now with dl.sourceforge.net:
http://amsn.hoentjen.eu/download/tcltls-1.5.0-8.src.rpm

Hope this is finally the right one, I must be the worst packager ever..
Comment 28 Jochen Schmitt 2006-04-25 13:48:01 EDT
Sorry, you may have a strange problem.

I got

fedora-qa tcltls-1.5.0-8.src.rpm
Starting QA for local srpm tcltls-1.5.0-8.src.rpm
Checking tcltls, version 1.5.0, release 8...
Are the name, version and release correct according to Fedora's package naming
guidelines ([y]/n)
y
Diff the spec files ? ([y]/n)
n
Checking source 0
tls1.5.0-src.tar.gz                                              12 kB 00:00
Source tls1.5.0-src.tar.gz is different from upstream
  (wiki: QAChecklist item 2)
Patches found: tcltls-configurein.pkgname.patch, tcltls-1.5-rpmoptflags.patch,
tcltls-1.5-64bit.patch, tcltls-makefilein-soname.patch,
tcltls-tclm4-soname.patch, tcltls-1.5-relpath.patch,
tcltls-1.5-pkgindex-version.patch, tcltls-1.5-ciphertest-KRB5.patch.
Look at the patches ? ([y]/n)


Comment 29 Jochen Schmitt 2006-04-25 13:50:36 EDT
for Information the md5sum from the downloaded file is:

2e39bbc3b1cc8f837fe7a0abec9bdaad  tls1.5.0-src.tar.gz

The source in in the rpm have the following md5sum:

9eeab472475773b3810acc808ebec759  tls1.5.0-src.tar.gz
Comment 30 Wart 2006-04-25 13:56:43 EDT
(In reply to comment #29)
> 2e39bbc3b1cc8f837fe7a0abec9bdaad  tls1.5.0-src.tar.gz
> 
> 9eeab472475773b3810acc808ebec759  tls1.5.0-src.tar.gz

Are the file sizes the same?

Can you untar the downloaded file and the tarball in the src rpm and run "diff
-r" to check the differences?
Comment 31 Jochen Schmitt 2006-04-25 16:12:22 EDT
It's seems to be a access problem to dl.sourceforge.com

When I download the .tar.gz file with firefox, I get a proper file.

Comment 32 Wart 2006-04-25 17:36:47 EDT
(In reply to comment #27)
> Hope this is finally the right one, I must be the worst packager ever..

Nah, I've seen (and written!) much worse.  :)

This and #185951 look like your first Extras packages.  If this is true then you
need to add the FE-NEEDSPONSOR blocker bug to both so that potential sponsors
can review and approve this package.
Comment 33 Sander Hoentjen 2006-04-26 00:03:22 EDT
Thanks a lot the both of you, I will now add the FE-NEEDSPONSOR blocker.
Comment 34 Wart 2006-05-04 14:11:46 EDT
Unassigning bug since the reviewer can't sponsor.  This will let sponsors know
that it's up for grabs.
Comment 35 Wart 2006-05-10 12:56:10 EDT
If you're having trouble finding a sponsor then there are a couple of things you
can do:

* Comment on other packages to show that you have a good understanding of the
packaging guidelines.  This is the most important thing you can do.

* Post to fedora-extras-list that you need a sponsor and refer back to these two
bugs.

Potential sponsors will then take a look at these bugs, as well as others that
you've commented on to see if you've earned your sponsorship.
Comment 36 Sander Hoentjen 2006-05-11 14:44:45 EDT
Wart,

thank you for your comments and for teaching me stuff. I knew about those 2
things to get sponsored. It just seems i picked a really bad time to start this
packaging stuff, since at the moment I am really busy with work/school and my
computer is not playing nicely (time for replacement, but no money). Work is
calming down a bit (I hope) so I think I will be able to do some reviews (making
comments) soon.

Sander
Comment 37 Wart 2006-05-25 14:31:22 EDT
Assigning this to myself now that I've been granted sponsor status.  Have you
filled out the CLA in the accounts system yet?
Comment 38 Sander Hoentjen 2006-05-26 04:52:12 EDT
I now have filled it out, and I applied for a cvsextras account.
Comment 39 Wart 2006-05-26 13:26:05 EDT
I finally got around to a formal review.  Most of the issues have already been
addreessed above, but there's still a few remaining nits.

MUST
====
* rpmlint output:
  W: tcltls-devel no-documentation
  ...which can be ignored because upstream doesn't provide development
  documentation.
* Source matches upstream
  9eeab472475773b3810acc808ebec759  SOURCES/tls1.5.0-src.tar.gz
* BSD license ok, license file included
* Spec file legible and in Am. English
* No excessive BR:
* No locales
* No shared libraries in the default linker path
* Not relocatable
* Owns the directory that it creates
* No duplicate %files
* buildroot cleaned where needed
* No -docs subpackage needed
* header file included in -devel package
* No .desktop file needed
* Package runs without crashing (tested with tclhttpd)
* Builds in mock on:
  core4-i386, core4-x86_64, core5-i386, core5-x86_64, devel-i386
* File permissions look ok

ANOMOLIES
=========
* Package name does not match upstream (tcltls vs tls).  However, the
  general convention outside of Fedora for naming Tcl packages is "tcl<foo>",
  which is already quite similar to Fedora's naming convention for
  language subpackages "tcl-<foo>".  In this case I feel it's better to
  use either tcl<foo> or tcl-<foo> for the package name as 'tls' is too
  generic.

* The version number is 1.5.0, yet upstream puts files in %{_libdir}/tls1.50.
  You should probably tell upstream to fix their directory name.  You can
  also do it yourself in this package, but I won't consider it a blocker.
  Fixing the directory name would also remove the need for the majorver and
  minorver %defines in the spec file.

MUSTFIX
=======
* A more succinct summary would be "OpenSSL extension for Tcl"
* Summary for the -devel package reads better as:
  "Header files for the OpenSSL extension for Tcl"
* Minor nit: Remove the commented-out entry in %files and in %prep
* Use a macro in %configure:
  %configure --with-ssl-dir=%{_prefix}
* Group should be "Development/Libraries"
* The URL, Version, and Group tags in the -devel subpackage aren't necessary.
  They are inherited from the base package.
Comment 40 Sander Hoentjen 2006-05-26 14:44:45 EDT
(In reply to comment #39)
I fixed all, except for "Group tag in the -devel subpackage", since rpmbuild
complains without it.
I decided to change the tcltls name to tcl-tls because i was just thinking about
that today, and i wanted to ask what was right, since in most other distros it
is tcltls (that's why i chose it initially) but for fedora's naming tcl-tls
seems most appropriate.

http://amsn.hoentjen.eu/download/tcl-tls.spec
http://amsn.hoentjen.eu/download/tcl-tls-1.5.0-9.src.rpm
Comment 41 Wart 2006-05-26 16:30:57 EDT
(In reply to comment #40)
> (In reply to comment #39)
> I fixed all, except for "Group tag in the -devel subpackage", since rpmbuild
> complains without it.
> I decided to change the tcltls name to tcl-tls because i was just thinking about
> that today, and i wanted to ask what was right, since in most other distros it
> is tcltls (that's why i chose it initially) but for fedora's naming tcl-tls
> seems most appropriate.

In this case I think following the name used by other distros would be more
appropriate.  Please use tcltls instead of tcl-tls.  After you make that change
I'll APPROVE the package and we can move on to the next step.
Comment 43 Wart 2006-05-26 17:06:43 EDT
I double checked that the package still builds in mock.

APPROVED

I've updated your sponsored status so that you can now continue with the cvs
import process.  Don't hesitate to email me if you have any questions.
Comment 44 Sander Hoentjen 2006-05-29 01:15:48 EDT
Package has been built for devel now, and I will request build for FC4 and 5 as
soon as the branches are created
Comment 45 Wart 2006-08-29 10:43:54 EDT
Reassigning to me as part of some bugzilla cleanup.

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