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)

-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

Created attachment 126831 [details]
protocoll from make test

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.

- 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.

Add the -g flag during compiliation.

Don't stript the executables.

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 :)

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)

Now where does that g come from? I don't understand.

(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).

(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.

(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@

(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.

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.

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.

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.

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).

(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.

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

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.

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.

God:
+ Mock build works fine.

Bad:
- Source tls1.5.0-src.tar.gz is different from upstream

ok now i have one with correct MD5SUM

http://amsn.hoentjen.eu/download/tcltls-1.5.0-7.src.rpm

This latest rpm works great with tclhttpd.  I can verify that this package is
functional.

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

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".

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..

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)

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

(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?

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.

(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.

Thanks a lot the both of you, I will now add the FE-NEEDSPONSOR blocker.

Unassigning bug since the reviewer can't sponsor.  This will let sponsors know
that it's up for grabs.

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.

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

Assigning this to myself now that I've been granted sponsor status.  Have you
filled out the CLA in the accounts system yet?

I now have filled it out, and I applied for a cvsextras account.

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.

(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

(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.

ok, name change reverted.
http://amsn.hoentjen.eu/download/tcltls.spec
http://amsn.hoentjen.eu/download/tcltls-1.5.0-10.src.rpm

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.

Package has been built for devel now, and I will request build for FC4 and 5 as
soon as the branches are created

Reassigning to me as part of some bugzilla cleanup.