Bug 495411 - Review Request: dnsjava - Java DNS implementation
Review Request: dnsjava - Java DNS implementation
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks: RabbIT
  Show dependency treegraph
 
Reported: 2009-04-12 16:41 EDT by Pavel Alexeev
Modified: 2009-05-09 00:12 EDT (History)
4 users (show)

See Also:
Fixed In Version: 2.0.6-6.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-19 12:15:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Pavel Alexeev 2009-04-12 16:41:40 EDT
Spec URL: http://hubbitus.net.ru/rpm/Fedora9/dnsjava/dnsjava.spec
SRPM URL: http://hubbitus.net.ru/rpm/Fedora9/dnsjava/dnsjava-2.0.6-1.fc9.src.rpm
Description: dnsjava is an implementation of DNS in Java. It supports all of the common record types and the DNSSEC types. It can be used for queries, zone transfers, and dynamic updates. It includes a cache which can be used by clients, and a minimal implementation of a server. It supports TSIG authenticated messages, partial DNSSEC verification, and EDNS0.
dnsjava provides functionality above and beyond that of the InetAddress class.
Since it is written in pure Java, dnsjava is fully threadable, and in many cases is faster than using InetAddress.
dnsjava provides both high and low level access to DNS. The high level functions perform queries for records of a given name, type, and class, and return an array of records. There is also a clone of InetAddress, which is even simpler. A cache is used to reduce the number of DNS queries sent. The low level functions allow direct manipulation of dns messages and records, as well as allowing additional resolver properties to be set.
A 'dig' clone and a dynamic update program are included, as well as a primary-only server.
Comment 1 Orcan Ogetbil 2009-04-13 04:06:23 EDT
Quick comments: 

* Package does not obey
    - the "BuildRequires and Requires" section,
    - the GCJ section,
    - the Javadoc scriptlets section of
  http://fedoraproject.org/wiki/Packaging/Java

Other than this,

* Macros must start with %% in the comments and in the changelog.

* We prefer %defattr(-,root,root,-)

? The %section macro is not used. Why is it defined?
Comment 2 Pavel Alexeev 2009-04-13 14:27:01 EDT
(In reply to comment #1)
> * Package does not obey
>     - the "BuildRequires and Requires" section,
[Build]Requires added.

>     - the GCJ section,
I add it. It is new for me, please check wht I do it properly.

>     - the Javadoc scriptlets section of
Ok. ghost deleted.
>   http://fedoraproject.org/wiki/Packaging/Java
> 
> Other than this,
> 
> * Macros must start with %% in the comments and in the changelog.
Off course! Where I do not double it???

> * We prefer %defattr(-,root,root,-)
Ok.

> ? The %section macro is not used. Why is it defined?
It come from JPackage imported rpm. Deleted.


http://hubbitus.net.ru/rpm/Fedora9/dnsjava/dnsjava-2.0.6-2.fc9.src.rpm
Comment 3 Orcan Ogetbil 2009-04-14 02:06:35 EDT
Okay, here is a more thorough review:

* rpmlint says:
   dnsjava.src:106: W: libdir-macro-in-noarch-package (main package)
      This one is a false warning and can be ignored
   dnsjava.x86_64: W: file-not-utf8 /usr/share/doc/dnsjava-2.0.6/Changelog
      We need to fix this. "iconv" will help.

! There are some example .java files in the root of the tarball. Their usage are explained in the USAGE file. I think these .java files need to go to %doc (of the main package). Alternatively, you can build them and put them in %{_datadir}/%{name} or so. (You mention about these files in the %description too)

* There is a tests directory. The README file mentions about building and running these compile tests. We should make a %check section and run these tests, if possible.

? Shouldn't the group tag be "System Environment/Libraries"?

! Since you are building the javadoc from source, you can remove the existing doc/ directory in %prep

* README file says: 
   "dnsjava is placed under the BSD license.  Several files are also under
   additional licenses; see the individual files for details."
I found that the files org/xbill/DNS/Tokenizer.java, org/xbill/DNS/ZoneTransferIn.java are licensed under MIT
This makes the license BSD and MIT

* This comment contains single % macro
   #ant -Dj2se.javadoc=%{_javadocdir}/java clean docsclean dnssec jar docs
Do we need this comment?

! Also these comments are not needed. They can be removed:
   #Epoch:         0
   #Vendor:                JPackage Project
   #Distribution:  JPackage

* This changelog entry contains single % macro
   - In Source0 tag inject %%{name} and %{version} macroses.

(Also macroses->macros)

* "%attr(-,root,root)" is redundant in the line
   %attr(-,root,root) %{_libdir}/gcj/%{name}
I reported this to java folks a while ago. They still didn't fix this guideline.

! In the description, please separate the paragraphs with blanks lines. It'll look better.

* These BR's seem unnecessary: jce, java-javadoc

* BR: jpackage-utils is listed twice.

* You don't want to write "specific_version" in Requires. If you need to pull openjdk-devel instead of gcj-devel, you can use something like >=1.7 or >=1:1.6.0

* Also use the same number (>=1.7 or >=1:1.6.0) for Requires: java
Comment 4 Pavel Alexeev 2009-04-14 10:55:20 EDT
(In reply to comment #3)

> * rpmlint says:
>    dnsjava.src:106: W: libdir-macro-in-noarch-package (main package)
>       This one is a false warning and can be ignored
I also think it is wrong. Is there bug for that on rpmlint?

>    dnsjava.x86_64: W: file-not-utf8 /usr/share/doc/dnsjava-2.0.6/Changelog
>       We need to fix this. "iconv" will help.
Off course I seen this. But I do not know from *what* encoding it should be recoded. Enca also do not help me:
$ enca Changelog
Unrecognized encoding

I think it is not very big problem in any case.

> ! There are some example .java files in the root of the tarball. Their usage
> are explained in the USAGE file. I think these .java files need to go to %doc
> (of the main package). Alternatively, you can build them and put them in
> %{_datadir}/%{name} or so. (You mention about these files in the %description
> too)
Ok, I put *.java into docs.

> * There is a tests directory. The README file mentions about building and
> running these compile tests. We should make a %check section and run these
> tests, if possible.
Tests added.

> ? Shouldn't the group tag be "System Environment/Libraries"?
I do not know. Seriously. Let it be, if you want.

> ! Since you are building the javadoc from source, you can remove the existing
> doc/ directory in %prep
Added.

> * README file says: 
>    "dnsjava is placed under the BSD license.  Several files are also under
>    additional licenses; see the individual files for details."
> I found that the files org/xbill/DNS/Tokenizer.java,
> org/xbill/DNS/ZoneTransferIn.java are licensed under MIT
> This makes the license BSD and MIT
I must place "BSD and MIT" into License tag? Or what I must do with it?

> * This comment contains single % macro
>    #ant -Dj2se.javadoc=%{_javadocdir}/java clean docsclean dnssec jar docs
> Do we need this comment?
No, this commetn unneeded anymore. Deleted.

> ! Also these comments are not needed. They can be removed:
>    #Epoch:         0
>    #Vendor:                JPackage Project
>    #Distribution:  JPackage
Off course. I comment out it, but leave for historical reasons. Any disadvantage from it?

> 
> * This changelog entry contains single % macro
>    - In Source0 tag inject %%{name} and %{version} macroses.
Fixed.
Hmmm, very strange why rpmlint was silent on it?! I recheck it now and it is also silent about this concrete error.

> (Also macroses->macros)
> 
> * "%attr(-,root,root)" is redundant in the line
>    %attr(-,root,root) %{_libdir}/gcj/%{name}
> I reported this to java folks a while ago. They still didn't fix this
> guideline.
I thought also when copied...
May you correct guidelines?
Fixed in my spec.

> ! In the description, please separate the paragraphs with blanks lines. It'll
> look better.
Ok :)
Done.

> * These BR's seem unnecessary: jce, java-javadoc
Why? It comes from JPackage rpm and i do not touch this.
 
> * BR: jpackage-utils is listed twice.
Fixed.

> * You don't want to write "specific_version" in Requires. If you need to pull
> openjdk-devel instead of gcj-devel, you can use something like >=1.7 or
> >=1:1.6.0
> * Also use the same number (>=1.7 or >=1:1.6.0) for Requires: java  
Sorry. It is my stupid copy/past. Fixed.



http://hubbitus.net.ru/rpm/Fedora9/dnsjava/dnsjava-2.0.6-3.fc9.src.rpm
Comment 5 Orcan Ogetbil 2009-04-14 18:30:57 EDT
(In reply to comment #4)
> (In reply to comment #3)
> 
> >    dnsjava.x86_64: W: file-not-utf8 /usr/share/doc/dnsjava-2.0.6/Changelog
> >       We need to fix this. "iconv" will help.
> Off course I seen this. But I do not know from *what* encoding it should be
> recoded. Enca also do not help me:
> $ enca Changelog
> Unrecognized encoding
> 

Isn't it iso-8859-1 ? Just guessing.

> I think it is not very big problem in any case.
> 
> > ! There are some example .java files in the root of the tarball. Their usage
> > are explained in the USAGE file. I think these .java files need to go to %doc
> > (of the main package). Alternatively, you can build them and put them in
> > %{_datadir}/%{name} or so. (You mention about these files in the %description
> > too)
> Ok, I put *.java into docs.
> 

    install -d %{buildroot}/%{_docdir}/%{name}-%{version}/
    install *.java %{buildroot}/%{_docdir}/%{name}-%{version}/
Just a notice: We generally pass a -p switch to install to preserve timestamps.

But actually, you don't need to install them explicitly in %install
It's enough to put *.java to %doc. Actually, when you put *.java to %doc, it will copy them one more time and the above installation will get overriden.


> > * There is a tests directory. The README file mentions about building and
> > running these compile tests. We should make a %check section and run these
> > tests, if possible.
> Tests added.
> 
> > * README file says: 
> >    "dnsjava is placed under the BSD license.  Several files are also under
> >    additional licenses; see the individual files for details."
> > I found that the files org/xbill/DNS/Tokenizer.java,
> > org/xbill/DNS/ZoneTransferIn.java are licensed under MIT
> > This makes the license BSD and MIT
> I must place "BSD and MIT" into License tag? Or what I must do with it?
> 

Yes. 
License: BSD and MIT


> > ! Also these comments are not needed. They can be removed:
> >    #Epoch:         0
> >    #Vendor:                JPackage Project
> >    #Distribution:  JPackage
> Off course. I comment out it, but leave for historical reasons. Any
> disadvantage from it?
> 

No, it doesn't really matter. I just said that for making things cleaned up.

> 
> > * These BR's seem unnecessary: jce, java-javadoc
> Why? It comes from JPackage rpm and i do not touch this.
> 

jce is provided by both java-1.6.0-openjdk and java-1.5.0-gcj. Adding BR:jce (without a version) will pull java-1.5.0-gcj, which is already being pulled by BR: java-gcj-compat-devel >= 1.0.31. Also, java-1.6.0-openjdk is pulled via java-devel >= 1.7 anyways. 

java-javadoc is not needed during the building of the package. The guidelines forbid unnecessary BR's.


> > * BR: jpackage-utils is listed twice.
> Fixed.
> 

It's still there. I guess you removed R: jpackage-utils instead.



Other than these, the package does not build with gcj. You need to add 
   BR: java-devel >= 1.7
also
    R: java >= 1.7
(I'm confused. Didn't you have these before already? Did you remove them?)
Comment 6 Pavel Alexeev 2009-04-15 04:43:44 EDT
(In reply to comment #5)
> Isn't it iso-8859-1 ? Just guessing.
I even have no idea about it!
> you don't need to install them explicitly in %install
> It's enough to put *.java to %doc. Actually, when you put *.java to %doc, it
> will copy them one more time and the above installation will get overriden.
Ok. Thank you for the hint. Fixed.

> Yes. 
> License: BSD and MIT
Ok, license changed.
 
> jce is provided by both java-1.6.0-openjdk and java-1.5.0-gcj. Adding BR:jce
> (without a version) will pull java-1.5.0-gcj, which is already being pulled by
> BR: java-gcj-compat-devel >= 1.0.31. Also, java-1.6.0-openjdk is pulled via
> java-devel >= 1.7 anyways. 
> 
> java-javadoc is not needed during the building of the package. The guidelines
> forbid unnecessary BR's.
Ok, I delete its. Thank you for the explanation.

> > > * BR: jpackage-utils is listed twice.
> > Fixed.
> > 
> 
> It's still there. I guess you removed R: jpackage-utils instead.
Sory for mistake. Fixed (again :) )

> Other than these, the package does not build with gcj.
So, in this case all work to add conditional GCJ build is nought???

> You need to add 
>    BR: java-devel >= 1.7
> also
>     R: java >= 1.7
Added.
> (I'm confused. Didn't you have these before already? Did you remove them?)
I don't rememeber. I think no. (Sorry, I add it into my svn repository too late - when review was already in progress and can't say exact)

And what is more strange: This package builded fine on my local machine, but failed in koji (http://koji.fedoraproject.org/koji/taskinfo?taskID=1299756) on %check stage. May be internet connection not available in mock chroot??
Any suggestions?

http://hubbitus.net.ru/rpm/Fedora9/dnsjava/dnsjava-2.0.6-4.fc9.src.rpm
Comment 7 Orcan Ogetbil 2009-04-15 12:19:26 EDT
(In reply to comment #6)
> (In reply to comment #5)
> > Isn't it iso-8859-1 ? Just guessing.
> I even have no idea about it!

I guess it is. Putting
   iconv -f iso8859-1 -t utf8 Changelog > tmpfile
   touch -r  Changelog tmpfile
   mv -f tmpfile Changelog
into %prep solved the issue for me.

> 
> > Other than these, the package does not build with gcj.
> So, in this case all work to add conditional GCJ build is nought???
> 

I think this is my bad. I should've tested this before I spoke in the first place. Yes, if GCJ (which supports only up to java 1.5) doesn't build the jar file properly, it doesn't make sense to build the aot bits.

So, you can change the first line of the SPEC file to:
   %global with_gcj 0
or alternatively, remove the GCJ related bits from the SPEC file. Sorry for the confusion.

> 
> And what is more strange: This package builded fine on my local machine, but
> failed in koji (http://koji.fedoraproject.org/koji/taskinfo?taskID=1299756) on
> %check stage. May be internet connection not available in mock chroot??
> Any suggestions?
> 
> http://hubbitus.net.ru/rpm/Fedora9/dnsjava/dnsjava-2.0.6-4.fc9.src.rpm  

Another thing I should have tested. Koji disables internet connection during package building, which is a good thing. Since we do need a connection for these tests, it doesn't make sense to run these tests during a standard build on koji.

But I think we should keep the code there and comment it out (or put an 
   %if do_test
   %check
   <code>
   %endif
clause around it and disable the tests on top of the SPEC file via
   %global do_test 0
). In either case, we need to explain in the SPEC file why we disabled them by default.
Comment 8 Pavel Alexeev 2009-04-15 17:09:05 EDT
> I guess it is. Putting
>    iconv -f iso8859-1 -t utf8 Changelog > tmpfile
>    touch -r  Changelog tmpfile
>    mv -f tmpfile Changelog
> into %prep solved the issue for me.
Guess from what?? Using iso8859-15 give the same result. And any other 8-bit encoding may be on this place.
"Ville Skyttä" is right family how you think?


> I think this is my bad. I should've tested this before I spoke in the first
> place. Yes, if GCJ (which supports only up to java 1.5) doesn't build the jar
> file properly, it doesn't make sense to build the aot bits.
Everyone has the right to make a mistake. No problem. I delete this bits.

> > And what is more strange: This package builded fine on my local machine, but
> > failed in koji (http://koji.fedoraproject.org/koji/taskinfo?taskID=1299756) on
> > %check stage. May be internet connection not available in mock chroot??
> > Any suggestions?
> > 
> > http://hubbitus.net.ru/rpm/Fedora9/dnsjava/dnsjava-2.0.6-4.fc9.src.rpm  
> 
> Another thing I should have tested. Koji disables internet connection during
> package building, which is a good thing. Since we do need a connection for
> these tests, it doesn't make sense to run these tests during a standard build
> on koji.
> 
> But I think we should keep the code there and comment it out (or put an 
>    %if do_test
>    %check
>    <code>
>    %endif
> clause around it and disable the tests on top of the SPEC file via
>    %global do_test 0
> ). In either case, we need to explain in the SPEC file why we disabled them by
> default.  
I think the same. Done.

http://hubbitus.net.ru/rpm/Fedora9/dnsjava/dnsjava-2.0.6-5.fc9.src.rpm
Comment 9 Orcan Ogetbil 2009-04-15 21:28:48 EDT
(In reply to comment #8)
> > I guess it is. Putting
> >    iconv -f iso8859-1 -t utf8 Changelog > tmpfile
> >    touch -r  Changelog tmpfile
> >    mv -f tmpfile Changelog
> > into %prep solved the issue for me.
> Guess from what?? Using iso8859-15 give the same result. And any other 8-bit
> encoding may be on this place.
> "Ville Skyttä" is right family how you think?
> 

It might work, but I don't know if Ville will allow you to decode his family.

Anyway, I think what happens is: These iso8859-X encodings are very close to each other. They only have a small number of characters that differ between each other. Since this text doesn't contain any nontrivial characters that are different in some of these iso8859-X encodings, all (or most) of these encodings will work during the iconv conversion.

The package is good to go. Beware that, on the last SRPM above, the default is set to *do* the tests. You will need to change that behavior before you commit, otherwise koji will fail.

------------------------------------------
This package (dnsjava) is APPROVED by oget
------------------------------------------

Since you are now more experienced in packaging java stuff, would you like to check out my java review requests? They are all quite similar to this one in terms of packaging.
Comment 10 Pavel Alexeev 2009-04-16 06:24:57 EDT
(In reply to comment #9)
> It might work, but I don't know if Ville will allow you to decode his family.
I speak what you may decode it from ANY encoding in any language (Greek, Russian, French, Polish) and I do not understood you guessing what it should be based on iso8859-X family!!!

> The package is good to go. Beware that, on the last SRPM above, the default is
> set to *do* the tests. You will need to change that behavior before you commit,
> otherwise koji will fail.
Off course. It is done.

> 
> ------------------------------------------
> This package (dnsjava) is APPROVED by oget
> ------------------------------------------

Orcan Ogetbil, tenk you very much for the review.

> Since you are now more experienced in packaging java stuff, would you like to
> check out my java review requests? They are all quite similar to this one in
> terms of packaging.
Ok, I try do review. But, but... do not wait it shortly. I do not make review until now! I start reading guidelines for that now. And, ok, first review will be yours!
Comment 11 Pavel Alexeev 2009-04-16 06:37:45 EDT
New Package CVS Request
=======================
Package Name: dnsjava
Short Description: Java DNS implementation
Owners: hubbitus
Branches: F-9 F-10 F-11 EL-5
InitialCC:
Comment 12 Kevin Fenzi 2009-04-17 17:39:46 EDT
cvs done.
Comment 13 Fedora Update System 2009-04-19 04:13:47 EDT
dnsjava-2.0.6-6.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/dnsjava-2.0.6-6.fc9
Comment 14 Fedora Update System 2009-04-19 04:14:46 EDT
dnsjava-2.0.6-6.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/dnsjava-2.0.6-6.fc10
Comment 15 Fedora Update System 2009-04-19 04:15:29 EDT
dnsjava-2.0.6-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/dnsjava-2.0.6-6.fc11
Comment 16 Fedora Update System 2009-04-21 20:50:54 EDT
dnsjava-2.0.6-6.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 17 Fedora Update System 2009-04-21 21:10:26 EDT
dnsjava-2.0.6-6.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 18 Fedora Update System 2009-05-09 00:12:24 EDT
dnsjava-2.0.6-6.fc11 has been pushed to the Fedora 11 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.