Fedora Merge Review: tclx http://cvs.fedora.redhat.com/viewcvs/devel/tclx/ Initial Owner: mmaslano
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}?
> 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.
Look right now.
* 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?
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.
(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?
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.
From tclx-8.4.0-7.fc7 are removed ldconfig. You was right tclx8.4.so don't seem to be shared library.
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.
The paths aren't nice. I'll fix all paths in tcl and related packages after the tcl8.5 will be out.
One more think, bcond_with/without should certainly be used instead of _without_check.
Some issues are still not addressed, reopening. Close when you think everything is done.
bcond_with is feature, that's not a bug of spec file.
Please have a look at: http://fedoraproject.org/wiki/Packaging/Tcl
(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.
Many packages don't have bcond macro. It's only eye candy for spec.
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.
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
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.
> 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
Looks good now. I'll attach a diff for the spec file with additional minor fixes, feel free to take what you want.
Created attachment 317970 [details] minor fixes
Thanks I like all changes. I rebuilt tclx in cvs: http://koji.fedoraproject.org/koji/taskinfo?taskID=849660
Thanks, reclosing bug.