[Bug 1312015] Review Request: javadocofflinesearch - Tool for offline searching in your docs via browser

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1312015



--- Comment #4 from jiri vanek <jvanek@xxxxxxxxxx> ---
(In reply to Raphael Groner from comment #3)
> Let give me some general advice before I can do a run of fedora-review tool.
> Please fix at least a., d. and j. (MUST).
> 
> a. License is GPLv3+, you can not downgrade version. MUST
> https://github.com/judovana/JavadocOfflineSearch/blob/master/LICENSE
> 
Crap. Thats (nearly) the same typo you had :) How could I!
Sure, will be fixed.
I'm really happy that I have the same bug in my spec I was so nasty to you :)

> b. Improve URL and Source0 consistently with macros. RECOMMENDED ¹
> URL:            https://github.com/judovana/%{srcname}
> Source0:       
> %{url}/releases/tag/%{srcname}-%{version}/%{srcname}-%{srcname}-%{version}.
> tar.gz

Right!

> 
> c. Please add a comment what your patch does. SHOULD
> > Patch1:         javadocFixes.patch
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
> 

yes, I already fixed this upstream. So I will add link.
> d. BuildRequires vs. Requires. MUST
> Why do you need everything listed in BR explicitly also in Requires?
> Please test to build without any listed Requires and add only those who do
> not get added automatically via rpmbuild. I think you can definitely remove:
> - BuildRequires:  jpackage-utils
> - BuildRequires:  java-devel
> - Requires:  jpackage-utils
> - Requires:  java-devel
> https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
> https://fedoraproject.org/wiki/Packaging:Java#BuildRequires_and_Requires

Hm. Not sure about that. First of all I need to fix
-Requires:       java 
+Requires:       java-headless

You are probably right with:
 - BuildRequires:  jpackage-utils
 - BuildRequires:  java-devel

And I really can safely remove them.

BUt I disagree with 
 - Requires:  jpackage-utils
 - Requires:  java-devel

First of all, I dont see `Requires:  java-devel` :)
And jpackage utils... hm.. I think Its safer to include them, But I can remove
them if you insists.


> test to build without any listed Requires and add only those who do
> not get added automatically via rpmbuild.

You may see that I have sme recommends in there. But hte thers? I wsa to often
surprised by missing generated dependence that I'm little bit unwiling to
follow this advice :( 

> 
> e. Description is same as Summary. Alternatively, use text got from upstream
> documentation. To improve general readability, insert an empty line before

Sure!

> %description. SHOULD
> #Suggestion:
> %description
> %{summary}
> #or:
> %description
> The goal of this project was to make searching in your (java)docs
> as easy and comfortable as when you are browsing them online.

Ok. Sopunds right.

> https://fedoraproject.org/wiki/Packaging:Guidelines#Summary_and_description
> 
> f. Improve removal of binaries. RECOMMENDED
> - find -name '*.class' -exec rm -f '{}' \;
> - find -name '*.jar' -exec rm -f '{}' \;
> + find -print -delete -name '*.class' -or -name '*.jar'

Sure!

> 
> g. Installation of jar file is wrong and needs fix. MUST
> - cp -r  dist/javadoc  $RPM_BUILD_ROOT/%{_javadocdir}/%{name}
> + install -m0644 assembly/*.jar -D $RPM_BUILD_ROOT/%{_javadir}/%{name}.jar
> https://fedoraproject.org/wiki/Packaging:Java#Installation_directory
> 

This advice do not sound right
I'm packing the lone jar to %{_javadir}
  cp dist/%{name}.jar $RPM_BUILD_ROOT/%{_javadir}
exactly as the guidelines says. Have I missed osmthing more?
Of ocuorse I canuse install insted of cp for this file.

Same for javadoc - it is placed right. And Wihtout that manual copy, no javadoc
appeared.

As fr "assembly" in your sugestion. Where did you get that? The ant run by
default do not run assembly target in build file.
Also this generated file would be against all guidelines (it is containing
budled lucene pdfbox .. and all deps)

So I would really stay with dist.



> h. Send your start script as a patch to upstream. SHOULD
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Upstream do not need this launcher. Its released jar have mainclass. So you
just java -jar it.

> 
> i. Obsolete lines. Please remove. SHOULD
> > Group:          Documentation
> > %defattr(-,root,root,-)
> > %defattr(-,root,root,-)
> (two times %defattr)
They an really miss? ok then.

> 
> j. Use %license. MUST
> - %doc LICENSE
> + %license LICENSE

>Oh! Sure!

> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
> 
> ¹ RECOMMENDED means here it's a personal preference and not (clearly) in
> guidelines, it's up to you to decide.

Not much to not-follow. the advices are good except the few I think I'm right.

Thanx!

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]