Bug 481564 - (bind-to-tinydns) Review Request: bind-to-tinydns - Convert DNS zone files in BIND format to tinydns format
Review Request: bind-to-tinydns - Convert DNS zone files in BIND format to ti...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Itamar Reis Peixoto
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-26 07:11 EST by Tim Jackson
Modified: 2009-03-05 11:33 EST (History)
4 users (show)

See Also:
Fixed In Version: 0.4.3-4.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-05 11:33:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
change makefile to accept cflags. (540 bytes, patch)
2009-01-27 10:08 EST, Itamar Reis Peixoto
no flags Details | Diff

  None (edit)
Description Tim Jackson 2009-01-26 07:11:30 EST
Spec URL: http://fedorapeople.org/~timj/packaging/bind-to-tinydns/bind-to-tinydns.spec
SRPM URL: http://fedorapeople.org/~timj/packaging/bind-to-tinydns/bind-to-tinydns-0.4.3-1.el5.src.rpm
Description:
This is a program that parses zone files used by the BIND DNS server and
converts them to the native format of the tinydns component of Dan Bernstein's
djbdns DNS server.
Comment 1 Itamar Reis Peixoto 2009-01-26 08:00:04 EST
Can you follow the packaging source url guidelines ?

https://fedoraproject.org/wiki/Packaging/SourceURL

Can you merge this 2 lines in only one ?

mkdir -p $RPM_BUILD_ROOT%{_bindir}
install -m 755 bind-to-tinydns $RPM_BUILD_ROOT%{_bindir}/

something like this
install -Dp -m 755 bind-to-tinydns $RPM_BUILD_ROOT%{_bindir}/bind-to-tinydns
Comment 2 Tim Jackson 2009-01-27 04:19:24 EST
(In reply to comment #1)
> Can you follow the packaging source url guidelines ?

Sure, I can't imagine why that's not there - must have been having a bad day or something when I originally wrote the spec. Updated.

> Can you merge this 2 lines in only one ?
> 
> mkdir -p $RPM_BUILD_ROOT%{_bindir}
> install -m 755 bind-to-tinydns $RPM_BUILD_ROOT%{_bindir}/
> 
> something like this
> install -Dp -m 755 bind-to-tinydns $RPM_BUILD_ROOT%{_bindir}/bind-to-tinydns

Yes, but it's unnecessary. However I've changed it to avoid any debate.

Spec URL:
http://fedorapeople.org/~timj/packaging/bind-to-tinydns/bind-to-tinydns.spec
SRPM URL:
http://fedorapeople.org/~timj/packaging/bind-to-tinydns/bind-to-tinydns-0.4.3-2.el5.src.rpm
Comment 3 Itamar Reis Peixoto 2009-01-27 08:49:45 EST
I can't access your src.rpm files (2009-01-27 11:37:09 ERROR 404: Not Found)

but here are the review.

GOOD -> 

- no rpmlint's message.

rpmlint /home/itamar/rpmbuild/SRPMS/bind-to-tinydns-0.4.3-2.fc10.src.rpm \
/home/itamar/rpmbuild/RPMS/x86_64/bind-to-tinydns-0.4.3-2.fc10.x86_64.rpm \
/home/itamar/rpmbuild/RPMS/x86_64/bind-to-tinydns-debuginfo-0.4.3-2.fc10.x86_64.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

- package builds fine in koji (dist-F11)
http://koji.fedoraproject.org/koji/taskinfo?taskID=1085805

- package meets naming and versioning guidelines.
- specfile is properly named, is cleanly written and uses macros consistently.
- dist tag is present.
- latest version is being packaged.
- BuildRequires are proper.
- %clean is present.
- package installs properly.
- debuginfo package looks complete.
- no scriptlets present.
- license is open source-compatible.
- package works as expected

SHOULD VERIFY ->
license sounds like GPLv2 for me instead GPLv2+  

I see no  blocker issue

------------------------------------------------------------
   This package (bind-to-tinydns) is APPROVED by itamarjp
------------------------------------------------------------
Comment 4 manuel wolfshant 2009-01-27 09:00:17 EST
The build step does not respect the mandatory compiling flags:

cc -Wall -g -o bind-to-tinydns bind-to-tinydns.c
bind-to-tinydns.c: In function 'handle_entry':
bind-to-tinydns.c:610: warning: pointer targets in passing argument 1 of 'str_to_uint' differ in signedness
bind-to-tinydns.c: In function 'main':
bind-to-tinydns.c:969: warning: pointer targets in passing argument 5 of 'handle_entry' differ in signedness
+ exit 0


this must be fixed before approval.
Comment 5 manuel wolfshant 2009-01-27 09:01:43 EST
 Itamar, please assign the bug to yourself and change the status from NEW to ASSIGNED when you perform a review. Thanks.
Comment 6 Tim Jackson 2009-01-27 09:24:41 EST
Sorry, the SRPM was:
http://fedorapeople.org/~timj/packaging/bind-to-tinydns/bind-to-tinydns-
0.4.3-2.src.rpm

Since no specific license version is explicitly specified by the author, it
should actually be marked as "GPL+" as per
https://fedoraproject.org/wiki/Licensing#Good_Licenses

This is corrected in
http://fedorapeople.org/~timj/packaging/bind-to-tinydns/bind-to-tinydns-0.4.3-3.src.rpm
and the corresponding spec.

Manuel, those are warnings only in the compiler output; the package builds successfully on all architectures and whilst it would be better if they weren't there, as far as I can see there is nothing in the Packaging Guidelines stating that all compiler warnings must be patched out.

Final point: I am intending to only import this on the EL-5 branch so if anyone wants to co-maintain in devel/F-10/F-9 then let me know.
Comment 7 Itamar Reis Peixoto 2009-01-27 10:08:45 EST
Created attachment 330097 [details]
change makefile to accept cflags.

change makefile to accept cflags.
Comment 8 Itamar Reis Peixoto 2009-01-27 10:11:43 EST
> Manuel, those are warnings only in the compiler output; the package builds
> successfully on all architectures and whilst it would be better if they weren't
> there, as far as I can see there is nothing in the Packaging Guidelines stating
> that all compiler warnings must be patched out.

Manuel is talking about CFLAGS, 

include the patch in Comment #7 and change your spec file to pass the correct CFLAGS parameters.

make %{?_smp_mflags} CFLAGS="%{optflags}"
Comment 10 Itamar Reis Peixoto 2009-01-28 07:45:43 EST
> this must be fixed before approval.

Manuel

please look at Comment #9, and answer if sounds good now.
Comment 11 manuel wolfshant 2009-01-28 08:43:33 EST
I've tested it yesterday :)
With your patch it looks OK.
Comment 12 Itamar Reis Peixoto 2009-01-28 08:59:51 EST
(In reply to comment #11)
> I've tested it yesterday :)
> With your patch it looks OK.

then please add the review flag again "+", so Tim Jackson will able to request cvs access.
Comment 13 manuel wolfshant 2009-01-28 11:44:51 EST
Right, here we go.
Comment 14 Tim Jackson 2009-01-28 15:28:20 EST
New Package CVS Request
=======================
Package Name: bind-to-tinydns
Short Description: Convert DNS zone files in BIND format to tinydns format
Owners: timj
Branches: EL-5
InitialCC:
Comment 15 Kevin Fenzi 2009-01-28 19:13:02 EST
Note: the devel branch is always created. 

If you really don't want to maintain this in non EL-5 branches, you might post to the devel list asking if anyone else would like to?

cvs done.
Comment 16 Itamar Reis Peixoto 2009-01-28 19:24:59 EST
> Final point: I am intending to only import this on the EL-5 branch so if anyone
> wants to co-maintain in devel/F-10/F-9 then let me know.

Can I do this ?
Comment 17 Tim Jackson 2009-01-29 12:08:06 EST
(In reply to comment #16)
> > Final point: I am intending to only import this on the EL-5 branch so if 
> > anyone wants to co-maintain in devel/F-10/F-9 then let me know.
> Can I do this ?

Sure, that would be great - thanks. I don't seem to be able to change the owner from the pkgdb, but just drop the CVS admin request in here.
Comment 18 Itamar Reis Peixoto 2009-01-31 18:54:52 EST
New Package CVS Request
=======================
Package Name: bind-to-tinydns
Short Description: Convert DNS zone files in BIND format to tinydns format
Owners: itamarjp timj
Branches: F-10
InitialCC:
Comment 19 Kevin Fenzi 2009-02-01 13:36:36 EST
cvs done.
Comment 20 Fedora Update System 2009-02-04 21:20:34 EST
bind-to-tinydns-0.4.3-4.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update bind-to-tinydns'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-1336
Comment 21 Fedora Update System 2009-03-05 11:33:38 EST
bind-to-tinydns-0.4.3-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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