This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 226479 - Merge Review: tcl
Merge Review: tcl
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Marcela Mašláňová
Fedora Package Reviews List
:
Depends On:
Blocks: 228782
  Show dependency treegraph
 
Reported: 2007-01-31 16:08 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-23 08:20:38 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
wart: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:08:43 EST
Fedora Merge Review: tcl

http://cvs.fedora.redhat.com/viewcvs/devel/tcl/
Initial Owner: mmaslano@redhat.com
Comment 1 Wart 2007-02-08 23:06:09 EST
Since Tcl is currently flip-flopping between 8.5a5 and 8.4, I'll start
with some of the low-hanging fruit in the spec file, and return to more
version-specific issues once it settles down.

* Source1 should no longer be necessary now that tk has been split out
  into a separate package.  This will greatly reduce the size of the
  src rpm.

* Removing Source1 will let you collapse the build directory by one
  level, since the tk sources no longer get unpacked alongside the tcl
  sources.  Make sure to adjust the setup and patch commands to reflect
  this new structure.

* Source2 contains the html sources.  Aren't these already built from the
  Tcl sources, or do they have to come from a separate tarball?  If
  they must come from a separate tarball, it would be nice to include
  a pointer to the original url, or commands used to generate the html
  files.

* BuildRequires: sed not necessary.  Likewise, I suspect that
  BuildRequires: man isn't needed either.

* The Version: and URL: tags for the subpackages are redundant.  They
  are copied from the main package if not found.

* In %build, ls %{_tmppath} is pointless.  Delete it.

* Move the 'make test' conditionals to a separate '%check' section instead of
  putting it in %build.

* %install contains a 'make html' command that appears to be building the
  html documentation.  This should be in %build, unless it requires
  Tcl to be installed before running.  Even if that's the case, it
  might be possible to run Tcl from the build directory in order to generate
  the html docs.  Either move 'make html' to %build, or document in the
  spec file why it needs to be in %install.

* The backwards compatible symlink might be better addressed by creating
  explicit directories. As it is now, it creates a link from /usr/lib to
  /usr/share/tcl8.5 on 64-bit systems, and doesn't create and own
  /usr/lib/tcl8.5 at all.
  See bug #227200 for one possible workaround.

* The %post and %postun sections should be replaced by one-liners:
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig
Comment 2 Marcela Mašláňová 2007-02-12 12:40:39 EST
Review will be needed for tcl8.4.

Source tk is needed for generating html.
BuildRequires are needed (sed is in configure).
What do you mean by redundant version?

Links for backward compatibily stay in tcl 8.4. I'll fix it for new version.
Comment 3 Sander Hoentjen 2007-02-12 13:26:15 EST
Is there a chance you will remove the epoch? Rawhide upgrade path breakage seems
to be viewed as the lesser evil compared to introducing an epoch. 
Comment 4 Wart 2007-02-12 14:51:11 EST
(In reply to comment #2)
> Review will be needed for tcl8.4.

Ok, then I'll start a full review based on the current state of CVS.

> Source tk is needed for generating html.

I checked upstream, and they publish the html pages:

http://superb-east.dl.sourceforge.net/sourceforge/tcl/tcl8.4.13-html.tar.gz

This will let you separate out the -html subpackage into a standalone package
and drop the included tk source tarball.  The -html subpackage can also be made
"BuildArch: noarch" since it contains only html and a few images.

> BuildRequires are needed (sed is in configure).

Separating the html documentation into a separate package removes the need for
'BuildRequires: man'.  sed is in the minimal build environment, and is
explicitly disallowed by the packaging guidelines:

http://fedoraproject.org/wiki/Packaging/Guidelines#head-4cadce5e79d38a63cad3941de1dadc9d25d67d30

> What do you mean by redundant version?

In the main package you specify:
URL: http://tcl.sourceforge.net/

And again in the -devel (and other) subpackage, you specify the url tag again. 
If you leave off the URL: tag in the -devel subpackage, then it will inherit the
value from the main package.  The same is true for the Version: tag; it's not
necessary to specify it again in the subpackage.

> Links for backward compatibily stay in tcl 8.4. I'll fix it for new version.

Ok, but it would still be nice to address the second component of bug #227200,
adding %{_libdir}/tcl%{majorver} to auto_path.  This would let us proceed with
updating the extensions in preparation for bug #226893.  But this is really an
orthogonal issue to the merge review, so I won't block the review just for this.
Comment 5 Marcela Mašláňová 2007-02-14 03:37:30 EST
I rebuilt it, some minor problems stay. Please see and let me know your opinion.
Comment 6 Wart 2007-02-14 17:14:35 EST
Now for the full review:

rpmlint output:
E: tcl invalid-soname /usr/lib/libtcl8.4.so libtcl8.4.so
  - I'm inclined to ignore this as this is how Tcl has always named/versioned
    its shared libraries.  Yes, it's awkward, but there are 10 years of
    Tcl history pressuring it to remain the same.
W: tcl-debuginfo spurious-executable-perm
/usr/src/debug/tcl-8.4.13/tcl8.4.13/generic/tclThreadAlloc.c
  - See "MUSTFIX" below for a simple fix.

GOOD
====
* Package and spec file named appropriately
* BSD license ok, license file included
* Spec file legible and in Am. English
* No locales
* ldconfig called appropriately
* Not relocatable
* build root cleaned in %clean
* Headers and unversioned .so shared libs in -devel subpackage
* %doc does not affect runtime
* No .desktop file needed

MUSTFIX
=======
* Mixed use of $RPM_BUILD_ROOT and %{buildroot}.  Please decide on one
  or the other and use it consistently in the spec file.

* Use %{_libdir} instead of %{_prefix}/%{_lib}.  If you really mean
  %{_prefix}/lib, then be aware that %{_lib} evaluates to lib64 on
  x86_64 arches.

* The package installs a recursive symlink:
  /usr/lib/tcl8.4 -> ./tcl8.4
  This used to be the symlink from /usr/share/tcl8.4 -> /usr/lib/tcl8.4

* Package does not own all directories that it creates.  In order to fix,
  and simplify the %files section, you can remove all of the %{_datadir}/...
  lines and replace them with a single:
  %{_datadir}/%{name}%{majorver}

* rpmlint debuginfo warning is harmless, but easily fixed by adding
  to %prep:
  chmod -x generic/tclThreadAlloc.c

* The %define epoch 1 is unnecessary.  If you set the Epoch: tag to '1',
  then rpm will implicitly define the %{epoch} variable.

* Source does not match upstream.  It appears that the fedora source
  tarball has 3 extra files:
  $ diff -r tcl8.4.13.upstream tcl8.4.13.fedora
  Only in tcl8.4.13.fedora/library/tcltest: constraints.tcl
  Only in tcl8.4.13.fedora/library/tcltest: files.tcl
  Only in tcl8.4.13.fedora/library/tcltest: testresults.tcl

  If these files were added to upstream's tarball after downloading,
  then I would recommend adding them as extra SourceX: files in the
  spec and committing them to CVS, instead of modifying the upstream
  tarball.

  If, however, upstream removed these files and replaced the upstream
  tarball without telling anyone, then you should just replace the
  fedora tarball with the current upstream version.

* %{_mandir}/mann/* should really be part of the main package, since it
  contains man pages for all of the script-level commands.  %{_mandir}/man3/*
  is correctly located in the -devel subpackage.

* Is the -html patch really necessary?  It doesn't seem to have any effect
  now that the html docs are installed from the upstream tarball and not
  generated at build time.

* Don't bother installing/packaging the ldAix shell script.  It's a wrapper
  for ld on AIX systems, making it pointless on Fedora.

SHOULD
======
* Consider using the recommended BuildRoot:
  %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

* Create a new package + spec file for tcl-html.  There are no more
  source dependencies between tcl and tcl-html, and splitting them
  into separate spec files will allow you to tag tcl-html as
  BuildArch: noarch
Comment 7 Dennis Gilmore 2007-02-14 17:18:42 EST
The old suggested buildroot is now mandatory  so that is a must fix item
Comment 8 Wart 2007-02-14 17:48:40 EST
(In reply to comment #6)
> SHOULD
> ======
...
> 
> * Create a new package + spec file for tcl-html.  There are no more
>   source dependencies between tcl and tcl-html, and splitting them
>   into separate spec files will allow you to tag tcl-html as
>   BuildArch: noarch

To move forward on this point, I created a new tcl-html package for review:  bug
#228782
Comment 9 Marcela Mašláňová 2007-02-15 10:09:33 EST
> E: tcl invalid-soname /usr/lib/libtcl8.4.so libtcl8.4.so
It must stay for backward compatibility.

> Use %{_libdir} instead of %{_prefix}/%{_lib}
Please see if it's everything allright.

> html
Patch decline to generate useless html source. I think it's better than spend
time with generating. I don't want split tcl into two packages, maybe in new
version. It's small source for html.

> Source does not match upstream.
Could you give me http of your source? Because I check it and they are the same.
Comment 10 Wart 2007-02-15 14:44:39 EST
(In reply to comment #9)
> > E: tcl invalid-soname /usr/lib/libtcl8.4.so libtcl8.4.so
> It must stay for backward compatibility.

Right.  That's why I said the error could be ignored.

> > html
> Patch decline to generate useless html source. I think it's better than spend
> time with generating. I don't want split tcl into two packages, maybe in new
> version. It's small source for html.

The html: target in the Makefile isn't run by either 'make' or 'make install',
so won't get executed by the spec file anymore.  You have to explitly run
'make html' to trigger it.  Since 'make html' was removed from the spec file,
the -html patch isn't needed.

Splitting the -html package into a separate spec file isn't a 
requirement, but it is strongly encouraged because it will:
* simplify the tcl spec file
* make the tcl src rpm smaller
* prevent needless updates to the -html package whenever tcl is rebuilt
* shorten the build time for the tcl package since the -html subpackage
  won't be rebuilt
* allow the -html files to be properly marked as 'noarch'

> > Source does not match upstream.
> Could you give me http of your source? Because I check it and they are the same.

# curl -O http://puzzle.dl.sourceforge.net/sourceforge/tcl/tcl8.4.13-src.tar.gz
# md5sum tcl8.4.13-src.tar.gz
c6b655ad5db095ee73227113220c0523  tcl8.4.13-src.tar.gz

...but this doesn't match the md5sum in the 'sources' file, nor does it
match the md5sum from the tcl source tarball that I get when running
'make srpm'.
Comment 11 Wart 2007-02-15 15:04:10 EST
(In reply to comment #9)
> > E: tcl invalid-soname /usr/lib/libtcl8.4.so libtcl8.4.so
> It must stay for backward compatibility.
> 
> > Use %{_libdir} instead of %{_prefix}/%{_lib}
> Please see if it's everything allright.

The use of %{_libdir} looks ok, but the backwards-compatible link is still wrong:

cd %{_datadir}
ln -s %{name}%{majorver} $RPM_BUILD_ROOT/%{_libdir}/%{name}%{majorver}

This creates the same recursive link:
lrwxrwxrwx 1 root root 6 2007-02-15 11:47 /usr/lib/tcl8.4 -> tcl8.4

I think you want either:
ln -s %{_datadir}/%{name}%{majorver} $RPM_BUILD_ROOT/%{_libdir}/%{name}%{majorver}

or 

ln -s %{_datadir}/%{name}%{majorver}
$RPM_BUILD_ROOT/%{_prefix}/lib/%{name}%{majorver}

It's unclear to me from the comment in the spec file if this should be
%{_prefix}/lib or %{_libdir} for backwards compatibility.  Do you know which
other packages need this backwards compatible link?
Comment 12 Wart 2007-02-15 16:33:41 EST
The -devel package for the -10 release is missing the private header files.  It
looks like the mkdir command to create the tcl-private directory got lost in the
spec file:

mkdir -p $RPM_BUILD_ROOT/%{_includedir}/%{name}-private/{generic,unix}
Comment 13 Marcela Mašláňová 2007-02-16 06:07:40 EST
> comment #11
I think correct will be ln -s {_datadir}/%{name}%{majorver}
$RPM_BUILD_ROOT/%{_prefix}/lib/%{name}%{majorver}
The prefix was used for packages for lib or lib64. I don't know, which packages
need this backward compatibility (not sure if they still needed, or if it's
something what was forgoten in spec).

> comment #12
yes, you're right
Comment 14 Wart 2007-02-19 21:01:37 EST
(In reply to comment #11)
> The use of %{_libdir} looks ok, but the backwards-compatible link is still wrong:
> 
> cd %{_datadir}
> ln -s %{name}%{majorver} $RPM_BUILD_ROOT/%{_libdir}/%{name}%{majorver}
> 
> This creates the same recursive link:
> lrwxrwxrwx 1 root root 6 2007-02-15 11:47 /usr/lib/tcl8.4 -> tcl8.4

Additionally, the 'cd %{_datadir}' changes the current working directory.  The
sed command shortly after makes use of $PWD when modifying the tclConfig.sh file:

sed -i -e "s|$RPM_BUILD_ROOT/unix|%{_libdir}|;
s|$RPM_BUILD_ROOT|%{_includedir}/%{name}-private|"
$RPM_BUILD_ROOT/%{_libdir}/%{name}Config.sh

This results in an unusable tclConfig.sh file because the paths to the header
files still use the build directory on the build server.
Comment 15 Marcela Mašláňová 2007-02-21 13:40:06 EST
Finally I built tcl with 
ln -s %{_datadir}/%{name}%{majorver} %{buildroot}%{_prefix}/lib/%{name}%{majorver}
When I change it, I couldn't built it on x86_64 so I ended with this solution
for backward compatibility.

Tcl-html wasn't be separated, because:
- many packages have documentation in one package. 
- the documentation isn't huge.
Comment 16 Wart 2007-02-26 23:55:21 EST
Regression:  %{buildroot} and $RPM_BUILD_ROOT are both used.

tclConfig.sh looks good now; Tk and others build fine with it.

Fix the buildroot macro and we should be done here.

Once this review is finished, I hope we can start discussing a plan on
addressing some of the other non-blocking issues that will further clean up the
way Tcl and the various extensions are handled in Fedora.

(Setting the fedora-review flag back to '?' per the revised review guidelines)
Comment 17 Marcela Mašláňová 2007-02-27 05:30:46 EST
Ok, I changed it in version tcl-8.4.13-12. We can discuss it by email.
Comment 18 Wart 2007-02-27 12:58:27 EST
I recently discovered that bz #227200 is really a multilib problem with Tcl. 
That is, it's not possible to install both x86_64 and i386 versions of Tcl
extensions simultaneously due to the symlink and missing %{_libdir}/tcl8.4 from
the package path.  I'll try to get some guidance from the mailing lists if this
multilib problem needs to be fixed as part of the merge review or not.
Comment 19 Wart 2007-06-04 00:03:22 EDT
(In reply to comment #18)
> I recently discovered that bz #227200 is really a multilib problem with Tcl. 
> That is, it's not possible to install both x86_64 and i386 versions of Tcl
> extensions simultaneously due to the symlink and missing %{_libdir}/tcl8.4 from
> the package path.  I'll try to get some guidance from the mailing lists if this
> multilib problem needs to be fixed as part of the merge review or not.

After a bit of a break, I've come back to this.  I take back what I said in the
previous comment.  Tcl currently does _not_ have multilib issues, because
arch-dependent extensions are installed outside of Tcl's install directory, and
into /usr/lib or /usr/lib64 as appropriate.  Bug #227200 is a metakit packaging
bug, not a Tcl bug.

Once the Tcl packaging guidelines are accepted (Tcl 8.5 timeframe?) then
extensions will be installed in Tcl's /usr/%{_lib}/tcl8.x directory, but this
will be an arch-specific directory and there still shouldn't be any multilib issues.

I'm calling this merge review done and hope that we can address some of the
package loading optimizations for Fedora 8, even if it's before Tcl 8.5 is released.

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