This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 225693 - Merge Review: dialog
Merge Review: dialog
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dan Horák
Fedora Package Reviews List
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:26 EST by Nobody's working on this, feel free to take it
Modified: 2009-12-04 07:51 EST (History)
5 users (show)

See Also:
Fixed In Version: dialog-1.1-1.20070227svn.fc7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-02 06:22:04 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
dan: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:26:54 EST
Fedora Merge Review: dialog

http://cvs.fedora.redhat.com/viewcvs/devel/dialog/
Initial Owner: harald@redhat.com
Comment 1 Daniel Kopeček 2007-02-25 16:12:39 EST
(!!) 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
********************
Comment 2 Daniel Kopeček 2007-02-26 07:21:35 EST
Fix: Release tag should use the %{?dist} tag
Release: 1.20060221svn%{?dist}
Comment 3 Harald Hoyer 2007-02-27 07:30:02 EST
so, you want, that I introduce Epoch ... :-/

%define SubVersion 20060221
Version: 1.0
Release: 1.%{SubVersion}svn%{?dist}
Epoch: 1
Comment 4 Patrice Dumas 2007-03-21 16:56:34 EDT
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}
....
Comment 5 Harald Hoyer 2007-03-23 06:31:12 EDT
> 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})
Comment 6 Patrice Dumas 2007-03-23 16:56:59 EDT
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?
Comment 7 Patrice Dumas 2007-03-23 19:32:19 EDT
* 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.
Comment 8 Thomas E. Dickey 2007-04-01 19:02:34 EDT
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.
Comment 9 Patrice Dumas 2007-06-01 04:26:09 EDT
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.
Comment 10 Miroslav Lichvar 2007-10-30 13:08:23 EDT
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?
Comment 11 Patrice Dumas 2007-10-30 16:40:44 EDT
(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.
Comment 12 Miroslav Lichvar 2007-11-05 08:27:31 EST
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.
Comment 13 Thomas E. Dickey 2007-11-05 10:24:29 EST
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.
Comment 14 Patrice Dumas 2008-04-13 05:04:57 EDT
(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.
Comment 15 Dan Horák 2009-12-02 06:22:04 EST
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.
Comment 16 Patrice Dumas 2009-12-02 11:57:51 EST
What about my comments on the API?
Comment 17 Dan Horák 2009-12-03 06:08:08 EST
(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.
Comment 18 Patrice Dumas 2009-12-04 07:51:32 EST
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).

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