Bug 226480 - Merge Review: tclx
Merge Review: tclx
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ivana Varekova
Fedora Package Reviews List
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:08 EST by Nobody's working on this, feel free to take it
Modified: 2008-09-29 10:49 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-09-29 10:49:41 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
varekova: fedora‑review+


Attachments (Terms of Use)
minor fixes (1.76 KB, patch)
2008-09-29 08:50 EDT, Patrice Dumas
no flags Details | Diff

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

http://cvs.fedora.redhat.com/viewcvs/devel/tclx/
Initial Owner: mmaslano@redhat.com
Comment 1 Ivana Varekova 2007-03-05 06:15:11 EST
rpmlint output:
W: tclx make-check-outside-check-section    make test

W: tclx no-documentation
W: tclx-devel no-documentation
E: tclx-doc standard-dir-owned-by-package /usr/share/man/mann

- is the groff BuildRequire necessary?
- change the BuildRoot to
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
- I'm not sure whether the present structure of tclx package is right - it is
divided into 4 packages 2 of them contains 1 and 3 files.
- in dir /usr/lib/debug/usr/lib/tclx8.4/ should be owned by tclx
- is it possible to add  %{?_smp_mflags}?
Comment 2 Marcela Mašláňová 2007-03-07 11:41:47 EST
> make test
fixed
> W: tclx no-documentation
> W: tclx-devel no-documentation
removed doc package and add doc into tclx package.

Packages are updated (time to time) and their structure and amount of files
should be dependent on flow from tclx to tcl. I removed only the doc package.

Comment 3 Ivana Varekova 2007-03-08 07:59:11 EST
Look right now.
Comment 4 Patrice Dumas 2007-03-13 20:42:30 EDT
* The call to autoconf shouldn't be needed. 
If you want to restore the configure timestamp, you can do
touch -r configure.2.relid configure
but I don't think that it is needed.

* The RPM_OPT_FLAGS are overwritten. I suggest using
make all CFLAGS_DEFAULT= CFLAGS_WARNING=
but maybe there are cleaner ways.

* could you please expand on:
# utf-8 locale needed to avoid truncating help files
LANG=en_US.UTF-8 make install DESTDIR=$RPM_BUILD_ROOT

* the ldconfig call seems completly unuseful to me: there
is no library that can be linked against.

* the documentation on using tclx is lacking. There is a README
with instructions, but it refers to a man page that isn't bundled
(TclX_Init.3). Looking at that man page it seems to refer to 
a library that isn't bundled??

* Related issue is should the man pages in doc/ be installed? 

* is the header file of any use without lib to link against?

* why is tix mentioned in the -devel description?


* Did you have a look at the tcl draft guideline?

http://fedoraproject.org/wiki/PackagingDrafts/Tcl

It may not be final, but maybe there are already things to use?
Comment 5 Marcela Mašláňová 2007-03-20 11:42:50 EDT
ldconfig - tclx8.4.so is shared library -> Packaging guidelines

tclx-devel - there is only header file, because library is in tclx and
tclx-devel couldn't be install without tclx.

> guideline
No, they didn't.
Comment 6 Patrice Dumas 2007-03-20 18:26:33 EDT
(In reply to comment #5)
> ldconfig - tclx8.4.so is shared library -> Packaging guidelines

Doesn't seems so to me. tclx8.4.so seems to be a dlopened object
file, not a shared library. It is in /usr/lib/tclx8.4/ so ldconfig
won't find it anyway.

> tclx-devel - there is only header file, because library is in tclx and
> tclx-devel couldn't be install without tclx.

Since there is no shared lib, maybe the tclx-devel package isn't
useful.

I am not sure whether it is right or not not to have a shared library.
What is your opinion on that subject?

> > guideline
> No, they didn't.

Maybe you should? 

Comment 7 Patrice Dumas 2007-03-20 18:28:17 EDT
As a side note, I don't think that this package should be
considered to be clean given the issue of the dlopened 
versus shared libs issue.
Comment 8 Marcela Mašláňová 2007-03-26 12:01:15 EDT
From tclx-8.4.0-7.fc7 are removed ldconfig. You was right tclx8.4.so don't seem
to be shared library.
Comment 9 Patrice Dumas 2007-08-27 10:51:33 EDT
Please have a look at
http://fedoraproject.org/wiki/PackagingDrafts/Tcl
The advocated paths seem better to me than what it used
currently. 

In this draft there is something about the name, but I think that it 
shouldn't be changed for now.

There are comments above that I think should be addressed.
Comment 10 Marcela Mašláňová 2007-08-28 03:13:37 EDT
The paths aren't nice. I'll fix all paths in tcl and related packages after the
tcl8.5 will be out.
Comment 11 Patrice Dumas 2007-11-22 12:51:53 EST
One more think, bcond_with/without should certainly be used instead
of _without_check.
Comment 12 Patrice Dumas 2008-01-27 17:54:46 EST
Some issues are still not addressed, reopening. Close when
you think everything is done.
Comment 13 Marcela Mašláňová 2008-03-26 11:24:47 EDT
bcond_with is feature, that's not a bug of spec file.
Comment 14 Patrice Dumas 2008-04-05 12:09:15 EDT
Please have a look at:
http://fedoraproject.org/wiki/Packaging/Tcl
Comment 15 Patrice Dumas 2008-04-05 12:10:43 EDT
(In reply to comment #13)
> bcond_with is feature, that's not a bug of spec file.

Can you explain why you don't want to use it? It is practical and 
consistent with other packages. I even remember having seen it in
some guidelines, but there shouldn't be a need for guidelines.

The review is here to ensure maximal quality, the conditionals are
part of it.
Comment 16 Marcela Mašláňová 2008-04-07 02:45:46 EDT
Many packages don't have bcond macro. It's only eye candy for spec.
Comment 17 Patrice Dumas 2008-04-07 15:34:17 EDT
All the packages I have reviewed have the bcond macro now. It
is not only eye-candy. The use of such conditional is much simpler
than one that needs a define.

I can do the patch if you want to. 
Comment 18 Marcela Mašláňová 2008-09-23 07:11:45 EDT
You persuaded me :) Here is package with bcond

http://mmaslano.fedorapeople.org/tclx/tclx-8.4.0-11.fc10.src.rpm
http://mmaslano.fedorapeople.org/tclx/tclx.spec
Comment 19 Patrice Dumas 2008-09-28 10:16:40 EDT
Remaining issues:

Still no library installed in ld paths, so the following is unneeded:
%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig


Among the man pages, CmdWrite, Handles and CmdWrite seems to be
part of tcl documentation and not tclx.

It seems to me that TclXInit.3 and Keylist.3 should be in the devel
package, since they are associated with the C interface.


Also TclXInit.3 is partly wrong because doing -ltclx won't work,
one needs to use -L /usr/lib/tcl8.5/tclx8.4/ -ltclx8.4. However
one could imagine that users wanting to use tclx like that
know that the object is in general dlopened and so that they have to find
the library in the tcl paths.


It may be possible to have in devel
%{_librir}/libtcl.so -> /usr/lib/tcl8.5/tclx8.4/libtclx8.4.so
and ship an /etc/ld.so.conf.d/ file to add the path to ldconfig.
However I think that it would be wrong since there is no soname.


There is also a rather innocuous rpmlint warning:
tclx.src: W: mixed-use-of-spaces-and-tabs (spaces: line 60, tab: line 74)


I'd still like to know more precisely why utf-8 locale needed 
to avoid truncating help files.
Comment 20 Marcela Mašláňová 2008-09-29 07:42:28 EDT
> Among the man pages, CmdWrite, Handles and CmdWrite seems to be
> part of tcl documentation and not tclx.
True. These pages were excluded.

> It seems to me that TclXInit.3 and Keylist.3 should be in the devel
> package, since they are associated with the C interface.
Moved into devel package.

> Also TclXInit.3 is partly wrong because doing -ltclx won't work,
> one needs to use -L /usr/lib/tcl8.5/tclx8.4/ -ltclx8.4. However
> one could imagine that users wanting to use tclx like that
> know that the object is in general dlopened and so that they have to find
> the library in the tcl paths.
> It may be possible to have in devel
> %{_librir}/libtcl.so -> /usr/lib/tcl8.5/tclx8.4/libtclx8.4.so
> and ship an /etc/ld.so.conf.d/ file to add the path to ldconfig.
> However I think that it would be wrong since there is no soname.
This package hadn't new release for long time and I don't think it's needed to do some bigger changes.

> I'd still like to know more precisely why utf-8 locale needed 
> to avoid truncating help files.
I'm also curious why. I removed it, 'cause I didn't find the reason.

http://mmaslano.fedorapeople.org/tclx/tclx-8.4.0-12.fc10.src.rpm
http://mmaslano.fedorapeople.org/tclx/tclx.spec
Comment 21 Patrice Dumas 2008-09-29 08:48:53 EDT
Looks good now. I'll attach a diff for the spec file with 
additional minor fixes, feel free to take what you want.
Comment 22 Patrice Dumas 2008-09-29 08:50:01 EDT
Created attachment 317970 [details]
minor fixes
Comment 23 Marcela Mašláňová 2008-09-29 09:00:45 EDT
Thanks I like all changes. I rebuilt tclx in cvs:
http://koji.fedoraproject.org/koji/taskinfo?taskID=849660
Comment 24 Patrice Dumas 2008-09-29 10:49:41 EDT
Thanks, reclosing bug.

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