Fedora Merge Review: dialog http://cvs.fedora.redhat.com/viewcvs/devel/dialog/ Initial Owner: harald
(!!) MUST: rpmlint output: **** Review message: W: dialog summary-ended-with-dot A utility for creating TTY dialog boxes. W: dialog hardcoded-path-in-buildroot-tag /var/tmp/dialog-root W: dialog buildprereq-use ncurses-devel, gettext ******************** (!!) MUST: Package must meet the Package Naming Guidelines **** Review message: - name: dialog-1.0.20060221-1 should be: dialog-1.0-1.20060221svn - Release tag should match format %{X}.%{alphatag} => 1.20060221svn - Version tag should be 1.0 ******************** (!!) MUST: The package must meet the Packaging Guidelines. **** Review message: - Package should not use the %makeinstall macro. - Uses hardcoded buildroot, should be %{_tmppath}/%{name}-%{version}-%{release}- root-%(%{__id_u} -n) ******************** (!!) MUST: All build depedencies must be listed in BuildRequires. **** Review message: - Package uses BuildPreReq instead of BuildRequires ********************
Fix: Release tag should use the %{?dist} tag Release: 1.20060221svn%{?dist}
so, you want, that I introduce Epoch ... :-/ %define SubVersion 20060221 Version: 1.0 Release: 1.%{SubVersion}svn%{?dist} Epoch: 1
Reopenning since it has not been accepted. The release is still wrong. It should be Release: 0.1.%{SubVersion}svn%{?dist} Please have a look at http://fedoraproject.org/wiki/Packaging/NamingGuidelines In that case I think that it is best not to use an epoch but also avoid incrementing the first release number, and instead use: 1.0.%{SubVersion}svn%{?dist} 1.1.%{SubVersion}svn%{?dist} ....
> It should be Release: 0.1.%{SubVersion}svn%{?dist} From http://fedoraproject.org/wiki/Packaging/NamingGuidelines: Release Tag for Post-Release Snapshot Packages: * %{X}.%{alphatag} Where %{X} is the build number from any previous "stable" package build, incremented by one (if no previous stable package build, use 1), and %{alphatag} is the checkout string, as described above. Example (post-release cvs): kismet-1.0-1 (this is the formal release of kismet 1.0) kismet-1.0-2 (this is a bugfix build to the 1.0 release) kismet-1.0-3.20050515cvs (move to a post-release cvs checkout) kismet-1.0-4.20050515cvs (bugfix to the post-release cvs checkout) kismet-1.0-5.20050517cvs (new cvs checkout, note the increment of %{X})
Ok, sorry, I thought that it really was a pre-release. It is still not completely obvious to me that it isn't a prerelease. In fact it seems that 1.1-20070227 is really a version. and it doesn't seems to be a svn snapshot at all. Maybe in that case it could be 1.1-1.20070227 20070227 being informative?
* I suggest not messing with the original samples directory but instead do something like: rm -rf __distributed_samples mkdir __distributed_samples cp -a samples __distributed_samples fgrep -l -r perl __distributed_samples/samples|xargs rm find __distributed_samples/samples -type f -print0 |xargs -0 chmod a-x and in %files %doc __distributed_samples/samples * More fundamentaly why removing the doc files executable bits? You could remove them on most of the copifuncs/copi.* files (except common.funcs, admin.funcs, copi.wheel, copi.rcnews), since they are concatenated and chmoded after that. In general I think that scripts should keep their exec bits. Another issue is that you remove ./msgbox4-utf8 ./msgbox4-eucjp that happens to have the perl string in them... And also the perl files you remove should certainly be there. * --includedir=/usr/include/dialog should certainly be --includedir=%{_includedir}/dialog * devel should not have COPYING dialog.lsm README since they are already in main package. * suggestion: replace %defattr(-,root,root) with %defattr(-,root,root,-) * devel package certainly requires ncurses-devel * the header files include a lot of ./configure conditionals. Thats very bad and, in general shouldn't be needed. Moreover some are completely unneeded and not even set. This should certainly be worked on with upstream. * there are no shared libs because libtool isn't really used. the configure switch is --with-libtool libtool should then be a BuildRequires.
dialog-1.0-20060221 is the upstream version. "the header files include a lot of ./configure conditionals." yes - they depend on dlg_config.h which is part of the package. During install, the symbols are prefixed to avoid collision with other packages. Some are not needed for applications linking to libdialog, but were used to configure and build the library. All of the header conditionals are required for portability. If you disagree, file a suitable bug report.
The API shouldn't be platform dependent, that's why it is wrong to have configure conditionals in header files, at least platform specific conditionals should be avoided as much as possible. It shows up especially in multilib situations since headers should be the same for both arches. This is a suitable bug report to report such issue, it is suitable to report all the package issues, even those that require talking with upstream. The resolution of those issues may not be a blocker for the merge review, but at least they must be discussed. Regarding the name, as Thomas said 1.1-20070409 (for example) is the full upstream version. I don't know exactly how it should translate in version and release, but in any case there shouldn't be the svn string in the release.
There is conflict in dlg_config.h for DLG__FILE_OFFSET_BITS (bug #341001). Can this #define be safely dropped or is it better to replace the file with a wrapper that will include dlg_config-{32,64}.h?
(In reply to comment #10) > There is conflict in dlg_config.h for DLG__FILE_OFFSET_BITS (bug #341001). Can > this #define be safely dropped or is it better to replace the file with a > wrapper that will include dlg_config-{32,64}.h? Seems that my prediction turned out to be right. Really this should be understood as a problem and worked out with upstream.
dialog-1.1-3.20071028.fc9 should have most of the issues fixed. DLG__FILE_OFFSET_BITS define was removed from installed header as it doesn't seem to be very useful for applications using the library. Thomas, please correct me if I'm wrong.
It's only used in building dialog. The generated header contains both symbols needed for building the program, as well as symbols needed to make applications build with the library.
(In reply to comment #13) > It's only used in building dialog. The generated header > contains both symbols needed for building the program, > as well as symbols needed to make applications build with > the library. Can't those 2 sets be split apart? An API should be platform independent.
formal review is here, see the notes below: OK source files match upstream: 068a46aa1ffbfe96fdbf5cedd480b795a4f6321a dialog-1.1-20080819.tgz OK package meets naming and versioning guidelines. OK specfile is properly named, is cleanly written and uses macros consistently. OK dist tag is present. OK license field matches the actual license. OK license is open source-compatible (LGPLv2). License text included in package. OK latest version is being packaged. OK BuildRequires are proper. OK compiler flags are appropriate. OK %clean is present. OK package builds in mock (Rawhide/x86_64). OK debuginfo package looks complete. OK* rpmlint is silent. OK final provides and requires look sane. N/A %check is present and all tests pass. OK shared libraries are added to the regular linker search paths, correct scriptlets present OK owns the directories it creates. OK doesn't own any directories it shouldn't. OK no duplicates in %files. OK file permissions are appropriate. OK correct scriptlets present. OK code, not content. OK documentation is small, so no -docs subpackage is necessary. OK %docs are not necessary for the proper functioning of the package. OK headers in devel OK no pkgconfig files. OK no libtool .la droppings. OK not a GUI app. - rpmlint compains a bit, but these are OK dialog.src: W: name-repeated-in-summary dialog dialog.x86_64: W: name-repeated-in-summary dialog This package is APPROVED.
What about my comments on the API?
(In reply to comment #16) > What about my comments on the API? It's not a merge review blocker in my opinion and should be tracked in as a separate bug.
API issue are, in my opinion items pertaining to review request since they are a packaging issue and have implications on other packages. Also review request are a more general place for quality control of a package that doesn't have the overhead of doing multiple bugs. Sometimes one bug per isue is the most practical way to handle bugs, sometime a more global assesment is better, and the review is certainly the place for this global assesment. Moreover, review requests are the place where quality issues of any kind can be spotted and where packages with quality issues can be blocked from entering the distro, while bug reports can be ignored by maintainers. This is especially important, in my experience, for Merge reviews since in many package from core are in bad state before the merge review. Doesn't matter much in the end, I am not sure what the quality standard of fedora is these days, but in the old past it seemed to me that there was a preference for quality over quantity (but maybe it was just me).