Bug 866901

Summary: Review Request: gogui - GUI to play game of Go
Product: [Fedora] Fedora Reporter: Christophe Burgun <linuxed_fedora>
Component: Package ReviewAssignee: Pierre-YvesChibon <pingou>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: bioinfornatics, ffotorel, notting, package-review, pikachu.2014, pingou
Target Milestone: ---Flags: pingou: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-02-25 21:52:47 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Christophe Burgun 2012-10-16 05:48:00 EDT
Spec URL: http://jouty.fedorapeople.org/gogui.spec
SRPM URL: jouty.fedorapeople.org/gogui-1.4.6-1.fc17.src.rpm
Description: Graphical interface to programs that play
the game of Go and use the Go Text Protocol (GTP), 
such as GNU Go. GoGui has special features 
that are useful for Go program developers.
Fedora Account System Username: jouty
Comment 1 Pierre-YvesChibon 2012-10-18 03:34:01 EDT
Some initial comments after having quickly looked at the spec file:

- No %doc on the sources?
- Some lines are really long...
- Check if libfoo-devel requires libfoo, that would reduce the list of BuildRequires
- Check the guidelines for install .desktop files
- Check the guidelines regarding the icons (in %post and %postun)
- There is no comment if the patch has been submitted upstream, has it?
Comment 2 Christophe Burgun 2012-10-18 10:48:18 EDT
Hi Pierre-Yves,

- I have fill the %doc
- I have cut the long lignes :)
- I have look the requires of libxslt-devel, libxml2-devel
- I have change the .desktop install same in guidelines
- I have add the icons %post and %postun same in guidelines
- Upstream has been informed about the patch http://sourceforge.net/users/jouty/
Comment 3 Florencia Fotorello 2012-10-19 08:17:32 EDT
Hi,

I'm trying to run "fedora-review", but I got this message:

----------------
$ fedora-review -b 866901
Processing review bug : 866901
Getting .spec and .srpm Urls from bug report : 866901
no SRPM file URL found in bug #866901
Cannot find any .spec and .srpm URLs in bugreport
----------------

Could you please add the new links in the ticket?

Thanks!
Comment 4 Christophe Burgun 2012-10-19 08:28:31 EDT
Hi Florancia,

Yes of course i add the new links  :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-1.fc17.src.rpm
Comment 5 Florencia Fotorello 2012-10-22 14:31:20 EDT
Hi Christophe, 

Thanks for the update.
I run “fedora-review” and there are some tests that failed. Could you please check them?

----------------------------------
$ cat 866901/gogui-review.txt 

Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated

==== Generic ====
[!]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
[!]: MUST Rpmlint output is silent.
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/Florencia/866901/gogui-1.4.6-src.tar.gz :
  MD5SUM this package     : a7cce6b4e314d0048f5e569e5fd43b73
  MD5SUM upstream package : 90da61b841b47a1c655c2a01205d2459

==== Java ====
[!]: MUST Javadocs are placed in %{_javadocdir}/%{name} (no -%{version}
     symlink)
     Note: No /usr/share/javadoc/gogui found

==== Maven ====
Issues:
[!]: MUST Javadocs are placed in %{_javadocdir}/%{name} (no -%{version}
     symlink)
     Note: No /usr/share/javadoc/gogui found
[!]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
[!]: MUST Rpmlint output is silent.
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
----------------------------------
Comment 6 Florencia Fotorello 2012-10-22 14:32:24 EDT
Complete fedora-review output:

-----------------
$ cat 866901/gogui-review.txt 

Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[ ]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[ ]: MUST Package contains no bundled libraries.
[ ]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[ ]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[ ]: MUST Macros in Summary, %description expandable at SRPM build time.
[!]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
[ ]: MUST Package requires other packages for directories it uses.
[ ]: MUST Package uses nothing in %doc for runtime.
[ ]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[ ]: MUST License field in the package spec file matches the actual license.
[ ]: MUST License file installed when any subpackage combination is installed.
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
[ ]: MUST Package meets the Packaging Guidelines.
[x]: MUST Package is named according to the Package Naming Guidelines.
[ ]: MUST Package does not generates any conflict.
[ ]: MUST Package obeys FHS, except libexecdir and /usr/target.
[ ]: MUST Package must own all directories that it creates.
[ ]: MUST Package does not own files or directories owned by other packages.
[ ]: MUST Package installs properly.
[ ]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.

rpmlint gogui-1.4.6-1.fc19.noarch.rpm

gogui.noarch: I: enchant-dictionary-not-found fr_FR
gogui.noarch: W: non-standard-group Unspecified
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gogui-javadoc-1.4.6-1.fc19.noarch.rpm

gogui-javadoc.noarch: I: enchant-dictionary-not-found fr_FR
gogui-javadoc.noarch: W: non-standard-group Unspecified
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gogui-1.4.6-1.fc19.src.rpm

gogui.src: I: enchant-dictionary-not-found fr_FR
gogui.src: W: non-standard-group Unspecified
gogui.src:103: E: files-attr-not-set
gogui.src:104: E: files-attr-not-set
gogui.src:105: E: files-attr-not-set
gogui.src:106: E: files-attr-not-set
gogui.src:107: E: files-attr-not-set
gogui.src:108: E: files-attr-not-set
gogui.src:109: E: files-attr-not-set
gogui.src:112: E: files-attr-not-set
gogui.src: W: no-cleaning-of-buildroot %install
gogui.src: W: no-cleaning-of-buildroot %clean
gogui.src: W: no-buildroot-tag
gogui.src: W: no-%clean-section
1 packages and 0 specfiles checked; 8 errors, 5 warnings.


[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/Florencia/866901/gogui-1.4.6-src.tar.gz :
  MD5SUM this package     : a7cce6b4e314d0048f5e569e5fd43b73
  MD5SUM upstream package : 90da61b841b47a1c655c2a01205d2459

[ ]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[ ]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[ ]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[ ]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[ ]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[ ]: SHOULD Package functions as described.
[ ]: SHOULD Package does not include license text files separate from
     upstream.
[ ]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[ ]: SHOULD Scriptlets must be sane, if used.
[!]: SHOULD SourceX / PatchY prefixed with %{name}.
     Note: Patch0: manifest.patch (manifest.patch)
[x]: SHOULD SourceX is a working URL.
[ ]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[ ]: SHOULD %check is present and all tests pass.
[ ]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.


==== Java ====
[ ]: MUST If source tarball includes bundled jar/class files these need to be
     removed prior to building
[x]: MUST Packages have proper BuildRequires/Requires on jpackage-utils
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Javadoc documentation files are generated and included in -javadoc
     subpackage
[x]: MUST Javadoc subpackages have Requires: jpackage-utils
[!]: MUST Javadocs are placed in %{_javadocdir}/%{name} (no -%{version}
     symlink)
     Note: No /usr/share/javadoc/gogui found
[ ]: SHOULD Package has BuildArch: noarch (if possible)
[ ]: SHOULD Package uses upstream build method (ant/maven/etc.)


==== Maven ====
[x]: MUST Old add_to_maven_depmap macro is not being used
[ ]: MUST If package contains pom.xml files install it (including depmaps)
     even when building with ant

Issues:
[!]: MUST Javadocs are placed in %{_javadocdir}/%{name} (no -%{version}
     symlink)
     Note: No /usr/share/javadoc/gogui found
[!]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
[!]: MUST Rpmlint output is silent.

rpmlint gogui-1.4.6-1.fc19.noarch.rpm

gogui.noarch: I: enchant-dictionary-not-found fr_FR
gogui.noarch: W: non-standard-group Unspecified
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gogui-javadoc-1.4.6-1.fc19.noarch.rpm

gogui-javadoc.noarch: I: enchant-dictionary-not-found fr_FR
gogui-javadoc.noarch: W: non-standard-group Unspecified
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint gogui-1.4.6-1.fc19.src.rpm

gogui.src: I: enchant-dictionary-not-found fr_FR
gogui.src: W: non-standard-group Unspecified
gogui.src:103: E: files-attr-not-set
gogui.src:104: E: files-attr-not-set
gogui.src:105: E: files-attr-not-set
gogui.src:106: E: files-attr-not-set
gogui.src:107: E: files-attr-not-set
gogui.src:108: E: files-attr-not-set
gogui.src:109: E: files-attr-not-set
gogui.src:112: E: files-attr-not-set
gogui.src: W: no-cleaning-of-buildroot %install
gogui.src: W: no-cleaning-of-buildroot %clean
gogui.src: W: no-buildroot-tag
gogui.src: W: no-%clean-section
1 packages and 0 specfiles checked; 8 errors, 5 warnings.


[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/Florencia/866901/gogui-1.4.6-src.tar.gz :
  MD5SUM this package     : a7cce6b4e314d0048f5e569e5fd43b73
  MD5SUM upstream package : 90da61b841b47a1c655c2a01205d2459



Generated by fedora-review 0.1.2
External plugins:
-----------------
Comment 7 Pierre-YvesChibon 2012-10-22 14:36:20 EDT
Running fedora-review by itself without going through the checklist at the end and check the point that are remaining (and making sure there are no false positive/negative) is pretty much useless.

Please don't do that, either do a full review (helped with feora-review) or just report the error but don't just paste the output of fedora-review without checking it.

That said, pointing out mistakes is always good to do, we are all subject at making some ;-)
Comment 8 Christophe Burgun 2012-10-24 07:03:17 EDT
Hi Florencia, Pierre-Yves

[!]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
=> The desktop file name follow now the guidelines %{name}.desktop

[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     Note: Using both %{buildroot} and $RPM_BUILD_ROOT
=> I have solved this issue from the install desktop entry and change hard path from the %post and %postun too.

[!]: MUST Rpmlint output is silent.
=> I have just the enchant-dictionary-not-found fr_FR which is normal

[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/Florencia/866901/gogui-1.4.6-src.tar.gz :
  MD5SUM this package     : a7cce6b4e314d0048f5e569e5fd43b73
  MD5SUM upstream package : 90da61b841b47a1c655c2a01205d2459
=> The Source0 url has been changed according sourceforge's url in the guidelines 

==== Java ====
[!]: MUST Javadocs are placed in %{_javadocdir}/%{name} (no -%{version}
     symlink)
     Note: No /usr/share/javadoc/gogui found
=>[root@pollux gogui]# pwd
/var/lib/mock/fedora-rawhide-x86_64/root/builddir/build/BUILDROOT/gogui-1.4.6-2.fc19.x86_64/usr/share/javadoc/gogui
[root@pollux gogui]# ll
total 812
drwxr-xr-x. 4 builder mock   4096 23 oct.  16:55 .
drwxr-xr-x. 3 builder mock   4096 23 oct.  16:55 ..
-rw-r--r--. 1 builder mock  25487 23 oct.  16:55 allclasses-frame.html
-rw-r--r--. 1 builder mock  21667 23 oct.  16:55 allclasses-noframe.html
-rw-r--r--. 1 builder mock   8523 23 oct.  16:55 constant-values.html
-rw-r--r--. 1 builder mock   4235 23 oct.  16:55 deprecated-list.html
-rw-r--r--. 1 builder mock   8030 23 oct.  16:55 help-doc.html
-rw-r--r--. 1 builder mock 598749 23 oct.  16:55 index-all.html
-rw-r--r--. 1 builder mock   1497 23 oct.  16:55 index.html
drwxr-xr-x. 3 builder mock   4096 23 oct.  16:55 net
-rw-r--r--. 1 builder mock   2611 23 oct.  16:55 overview-frame.html
-rw-r--r--. 1 builder mock   7662 23 oct.  16:55 overview-summary.html
-rw-r--r--. 1 builder mock  45091 23 oct.  16:55 overview-tree.html
-rw-r--r--. 1 builder mock    403 23 oct.  16:55 package-list
drwxr-xr-x. 2 builder mock   4096 23 oct.  16:55 resources
-rw-r--r--. 1 builder mock  45659 23 oct.  16:55 serialized-form.html
-rw-r--r--. 1 builder mock  11139 23 oct.  16:55 stylesheet.css

The directory /usr/share/javadoc/gogui is here 

=> I have correct the directory ownership in %files section
and update %changelog section
=> I have change the patch name and comment it in spec file with upstream url

New link :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-2.fc17.src.rpm
Comment 9 Christophe Burgun 2012-10-25 07:25:13 EDT
I have change the spec file and rebuilt due to :
http://jouty.fedorapeople.org/upstream-info

changing xgd to install command (see changelogs)

here new links :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-3.fc17.src.rpm
Comment 10 Christophe Burgun 2012-10-26 09:03:16 EDT
For info some change have been made due to :

http://jouty.fedorapeople.org/upstream-info
Comment 11 Pierre-YvesChibon 2012-10-31 04:53:16 EDT
I think we can still improve this:

install -d $RPM_BUILD_ROOT%{_datadir}/thumbnailers
cat config/gogui.thumbnailer | sed "s;/usr/bin/gogui-thumbnailer;$PREFIX/bin/gogui-thumbnailer;" \
> $RPM_BUILD_ROOT%{_prefix}/share/thumbnailers/gogui.thumbnailer

- consistent use of %{_datadir}
- put the sed in %prep
- use install to install the file as you did for the others
Comment 12 Christophe Burgun 2012-10-31 07:21:37 EDT
- The /usr/share has been changed with the macro %{_datadir}
- The sed has been push in %prep section
- The installation for thumbnailer is now with install command

New links :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-4.fc17.src.rpm
Comment 13 Florencia Fotorello 2012-10-31 08:01:16 EDT
Hello,

Some informal comments:

1)  You can use %{name}.desktop instead of gogui.desktop.

2)  Regarding rpmlint output, just some comments, no action needed:

--------------
gogui.noarch: I: enchant-dictionary-not-found fr_FR
gogui.noarch: W: non-standard-group Unspecified
--------------

Details:
===>>> gogui.noarch: I: enchant-dictionary-not-found fr_FR 
A dictionary for the Enchant spell checking library is not available for the
language given in the info message.  Spell checking will proceed with
rpmlint's built-in implementation for localized tags in this language.

===>>> gogui.noarch: W: non-standard-group Unspecified
All current versions of Fedora (and their respective RPM versions) treat the Group tag as optional. Packages may include a Group: field for compatibility with EPEL, but are not required to do so.
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Group_tag

3) rpmlint output for .src.rpm is:

-------------
gogui.src: I: enchant-dictionary-not-found fr_FR
gogui.src: W: non-standard-group Unspecified
gogui.src:115: E: files-attr-not-set
gogui.src:116: E: files-attr-not-set
gogui.src:117: E: files-attr-not-set
gogui.src:118: E: files-attr-not-set
gogui.src:119: E: files-attr-not-set
gogui.src:120: E: files-attr-not-set
gogui.src:121: E: files-attr-not-set
gogui.src:122: E: files-attr-not-set
gogui.src:123: E: files-attr-not-set
gogui.src:124: E: files-attr-not-set
gogui.src:125: E: files-attr-not-set
gogui.src:128: E: files-attr-not-set
gogui.src: W: no-cleaning-of-buildroot %install
gogui.src: W: no-cleaning-of-buildroot %clean
gogui.src: W: no-buildroot-tag
gogui.src: W: no-%clean-section
1 packages and 0 specfiles checked; 12 errors, 5 warnings.
-------------

Details:

===>>> gogui.src:115: E: files-attr-not-set
This is now the default and no longer necessary to explicitly include.
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#File_Permissions


===>>> gogui.src: W: no-cleaning-of-buildroot %install
It’s only necessary for F-12 and below or EPEL 5.
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#.25clean

===>>> gogui.src: W: no-buildroot-tag
The BuildRoot tag isn't used in your spec. It must be used in order to allow
building the package as non root on some systems. For some rpm versions (e.g.
rpm.org >= 4.6) the BuildRoot tag is not necessary in specfiles and is ignored
by rpmbuild; if your package is only going to be built with such rpm versions
you can ignore this warning.
Comment 14 Christophe Burgun 2012-11-02 05:55:44 EDT
Thanks Florencia for the informal comments :

1) I have change some gogui with the %{name} macro (see changelog)

2) The dictionary message is normal, the group tag isn't need
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Group_tag

3) I haven't %defattr in the spec file about the files-attr-not-set error
my rpmlint ( rpmlint-1.4-10.fc17.src.rpm) doesn't put this output error
Concerning the other warnings there are only used with EPEL.

New links :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-5.fc17.src.rpm
Comment 15 Mohamed El Morabity 2012-11-02 06:20:29 EDT
Some comments too:

1) as I told you recently on IRC, sentences shouldn't be used in Summary; "Graphical user interface to programs that play the board game Go" may be enough.

2) On the contrary, use real sentences to describe the role of gogui in Description; by the way, here is an improved French description (for example, don't literally translate "Go Text", it's a standard: http://fr.wikipedia.org/wiki/Go_Text_Protocol):

%description
Gogui is a graphical interface to programs that play
the game of Go and use the Go Text Protocol (GTP), 
such as GNU Go. GoGui has special features 
that are useful for Go program developers.

%description -l fr_FR
Gogui est une interface graphique pour les programmes de go implémentant le protocal Go Text Protocol (GTP), tels que GNU Go. Gogui présente des fonctionnalités utiles aux concepteurs de programmes Go.

3) You don't have to specify BuildRequires in sub-packages (see javadoc subpackage). BuildRequires must only be specified on the main package header section.

4) DON'T pipe sed through cat, it's useless, unreadable and considered bad practice. Simply use sed only instead:

for FILE in bin/*; do
	if [ -f $FILE -a -x $FILE ]; then
		sed -e "s;^GOGUI_LIB=.*;GOGUI_LIB=\"%{_javadir}/%{name}\";" \
		-e "s;^JAVA_DEFAULT=.*;JAVA_DEFAULT=\"$JAVA_DEFAULT\";" \
		> $RPM_BUILD_ROOT%{_prefix}/$FILE
		chmod a+x $RPM_BUILD_ROOT%{_prefix}/$FILE
	fi
done

5) You don't have to "manually" install documentation in %install target; use the %doc macro instead in files:
- Remove this line:
install -pm 644 doc/manual/html/*.html $RPM_BUILD_ROOT%{_defaultdocdir}/%{name}
- Install HTML documentation in %files as below:
%files
...
%doc COPYING.html README.html doc/manual/html/*.html
Comment 16 Mohamed El Morabity 2012-11-02 06:22:06 EDT
> 4) DON'T pipe sed through cat, it's useless, unreadable and considered bad
> practice. Simply use sed only instead:
> 
> for FILE in bin/*; do
> 	if [ -f $FILE -a -x $FILE ]; then
> 		sed -e "s;^GOGUI_LIB=.*;GOGUI_LIB=\"%{_javadir}/%{name}\";" \
> 		-e "s;^JAVA_DEFAULT=.*;JAVA_DEFAULT=\"$JAVA_DEFAULT\";" \
> 		> $RPM_BUILD_ROOT%{_prefix}/$FILE
> 		chmod a+x $RPM_BUILD_ROOT%{_prefix}/$FILE
> 	fi
> done
Mistake, use this instead:

 for FILE in bin/*; do
 	if [ -f $FILE -a -x $FILE ]; then
 		sed -e "s;^GOGUI_LIB=.*;GOGUI_LIB=\"%{_javadir}/%{name}\";" \
 		-e "s;^JAVA_DEFAULT=.*;JAVA_DEFAULT=\"$JAVA_DEFAULT\";" $FILE \
 		> $RPM_BUILD_ROOT%{_prefix}/$FILE
 		chmod a+x $RPM_BUILD_ROOT%{_prefix}/$FILE
 	fi
 done
Comment 17 Christophe Burgun 2012-11-02 10:44:04 EDT
Tanks for the help Mohamed

- I have change the Summary and Description as your recommendations
- The Buildrequires have been delete from javadoc subpackage
- The sed syntax has been change without the cat
- The manual files are installed through the %doc section and not manually anymore

New links :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-6.fc17.src.rpm
Comment 18 Pierre-YvesChibon 2012-11-08 04:45:22 EST
>- The sed syntax has been change without the cat
Almost: 
  cat config/%{name}.thumbnailer | sed "s;/usr/bin/%{name}-thumbnailer;$PREFIX/bin/%{name}-thumbnailer;"
Comment 19 Christophe Burgun 2012-11-08 07:22:02 EST
I have change the sed in the prep section without cat too

New links :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-7.fc17.src.rpm
Comment 20 Pierre-YvesChibon 2012-11-22 03:55:48 EST
There is something missing in the javadoc package wrt to license ;-)
Comment 21 Christophe Burgun 2012-11-23 05:37:33 EST
I have add the licence file to the subpackage

New links :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-8.fc17.src.rpm
Comment 22 Christophe Burgun 2012-12-11 17:53:20 EST
Koji build :

http://koji.fedoraproject.org/koji/taskinfo?taskID=4780700
Comment 23 Pierre-YvesChibon 2013-01-07 03:06:47 EST
While reading the log from the build, it seems that ant -p is missing a parameters (and thus uses a default), but it might be safer to add it.
Comment 24 Christophe Burgun 2013-01-07 06:46:01 EST
Spec has been updated with your recommendations (see changelog)

New links :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-9.fc17.src.rpm
Comment 25 Christophe Burgun 2013-01-10 09:55:15 EST
Spec has been updated with your recommendations (see changelog)

New links :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-10.fc17.src.rpm
Comment 26 Pierre-YvesChibon 2013-02-05 10:00:41 EST
Ok so here is the official review:

Basically, it looks all good to go, the only question is:
- Should gogui require gnugo?
One can use it without gnugo installed in a multiplayer setting but for single played gnugo would be one way to go.
Are there other program in Fedora with which one could play gogui in a single player mode?

What do you think?

Let's discuss this point and I'll approve the package and you as a packager.
Sorry for taking so long though :-s

Key:
[x] = Pass               [?] = Not evaluated
[!] = Fail               [-] = Not applicable


===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Package complies to the Packaging Guidelines
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Large documentation must go in a -doc subpackage.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: No rpmlint messages.

Java:
[x]: Packages have proper BuildRequires/Requires on jpackage-utils
[x]: Fully versioned dependency in subpackages, if present.
[x]: Javadoc documentation files are generated and included in -javadoc
     subpackage
[x]: Javadoc subpackages have Requires: jpackage-utils
[x]: Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlink)
[x]: Bundled jar/class files should be removed before build

Maven:
[-]: If package contains pom.xml files install it (including depmaps) even
     when building with ant
[x]: Old add_to_maven_depmap macro is not being used

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: update-mime-database is invoked as required
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

Java:
[x]: Package has BuildArch: noarch (if possible)
[x]: Package uses upstream build method (ant/maven/etc.)

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: gogui-1.4.6-10.fc17.noarch.rpm
          gogui-javadoc-1.4.6-10.fc17.noarch.rpm
gogui.noarch: I: enchant-dictionary-not-found fr_FR
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

Rpmlint (installed packages)
----------------------------
# rpmlint gogui-javadoc gogui
gogui-javadoc.noarch: I: enchant-dictionary-not-found fr_FR
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'



MD5-sum check
-------------
http://downloads.sourceforge.net/gogui/gogui-1.4.6.zip :
  CHECKSUM(SHA256) this package     : 0ca99fcb0957d077e691bd65861d17761bedf14144cc9820d2b2e71a096f9fe3
  CHECKSUM(SHA256) upstream package : 0ca99fcb0957d077e691bd65861d17761bedf14144cc9820d2b2e71a096f9fe3
Comment 27 Christophe Burgun 2013-02-06 11:24:01 EST
Hi Pierre-Yves,

Gogui don't require gnugo 
You can use GoGui without any Go engine as a SGF viewer/editor or as a
local Go board for playing games between humans.
However its main use is as a GUI for Go engines so i have include a new patch who add a second desktop for gnugo if he is installed.
So user can use it or not.

New links :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-11.fc17.src.rpm
Comment 28 Mohamed El Morabity 2013-02-06 12:14:25 EST
(In reply to comment #27)
> Gogui don't require gnugo 
> You can use GoGui without any Go engine as a SGF viewer/editor or as a
> local Go board for playing games between humans.
> However its main use is as a GUI for Go engines so i have include a new
> patch who add a second desktop for gnugo if he is installed.
I don't think this patch is a good idea, unless you explicitely set gnugo as a Requires. What would happen if I launched gogui through this new desktop entry, without having gnugo installed?
Such a patch is probably superfluous for gogui. The description of gogui is clear enough to imply it's GTP client, after all.

By the way, as I told you several weeks ago on IRC, please use macros provided by jpackage-utils to generate executable wrappers:
   https://fedoraproject.org/wiki/Packaging:Java#Wrapper_Scripts
To my opinion, it's safer and easier to maintain than patching the launcher scripts provided upstream with complicated sed queries.
Comment 29 Mohamed El Morabity 2013-02-06 12:26:57 EST
More comments:
  >>> %description -l fr_FR
"fr_FR" is maybe a little bit too restrictive, "fr" may be enough. I don't think your package description is specific to France.

  >>> BuildRequires:	... libxslt-devel, libxml2-devel ...
Are those BR really required? They're quite strange for a Java program which doesn't build JNI bits.
Comment 30 Pierre-YvesChibon 2013-02-06 12:27:38 EST
(In reply to comment #28)
> (In reply to comment #27)
> > Gogui don't require gnugo 
> > You can use GoGui without any Go engine as a SGF viewer/editor or as a
> > local Go board for playing games between humans.
> > However its main use is as a GUI for Go engines so i have include a new
> > patch who add a second desktop for gnugo if he is installed.
> I don't think this patch is a good idea

I agree on this, I think providing the .desktop file as Source1 would be nicer.

> unless you explicitely set gnugo as
> a Requires. What would happen if I launched gogui through this new desktop
> entry, without having gnugo installed?

If my reading of the description of the TryExec key in the desktop file is correct, nothing should happen.

(source: http://standards.freedesktop.org/autostart-spec/autostart-spec-latest.html)

> By the way, as I told you several weeks ago on IRC, please use macros
> provided by jpackage-utils to generate executable wrappers:
>    https://fedoraproject.org/wiki/Packaging:Java#Wrapper_Scripts
> To my opinion, it's safer and easier to maintain than patching the launcher
> scripts provided upstream with complicated sed queries.

Not a blocker, but I clearly agree on this to :)
Comment 31 Christophe Burgun 2013-02-07 11:09:11 EST
- Delete patch gnugo desktop because upstream would prefer gogui package alone
- Remove fr_FR and just let fr
- Delete BR libxslt-devel, libxml2-devel

New link :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-12.fc17.src.rpm

@Mohamed El Morabity

I have try the %jpackage_script but this hang on at startup 

Could you please look with me if i do something wrong?

Link with jpackage :


SPEC : http://jouty.fedorapeople.org/jpackage/gogui.spec
SRPM : http://jouty.fedorapeople.org/jpackage/gogui-1.4.6-12.fc17.src.rpm

[root@pollux bin]# ./gogui
/usr/bin/build-classpath: error: Could not find /usr/share/java/gogui/gogui.jar Java extension for this JVM
/usr/bin/build-classpath: error: Some specified jars were not found
Erreur : impossible de trouver ou charger la classe principale net.sf.gogui.gogui
[root@pollux bin]# ll /usr/share/java/gogui/gogui.jar
-rw-r--r--. 1 root root 964204  7 févr. 16:06 /usr/share/java/gogui/gogui.jar
Comment 32 Christophe Burgun 2013-02-13 08:22:24 EST
Hi,

I have change the sed with the Wrapper scripts as your recommendations:
See changelogs

New link :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-13.fc17.src.rpm
Comment 33 Christophe Burgun 2013-02-14 04:25:39 EST
Add rm for class files
See changelogs:

New link :

SPEC : http://jouty.fedorapeople.org/gogui.spec
SRPM : http://jouty.fedorapeople.org/gogui-1.4.6-14.fc17.src.rpm
Comment 34 Pierre-YvesChibon 2013-02-14 06:47:06 EST
Well this looks all good.

This package is APPROVED.

Christophe, I will now sponsor you in the packager group, congratulations!
You can now do formal review to approve packages from other packagers. You will not be able to approve new packagers though.

Use your power wisely and keep your package in good shape.

You may now follow the procedures from: 
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner
Comment 35 Christophe Burgun 2013-02-14 07:52:53 EST
New Package SCM Request
=======================
Package Name: gogui
Short Description: Graphical user interface to programs that play the board game Go
Owners: jouty
Branches: f17 f18
Comment 36 Jon Ciesla 2013-02-14 11:28:33 EST
fas email address and bugzilla email address need to match.
Comment 37 Pierre-YvesChibon 2013-02-14 11:31:35 EST
Ah, that's why Christophe couldn't set the flag himself. Sorry I missed this.
Comment 38 Christophe Burgun 2013-02-14 16:13:30 EST
New Package SCM Request
=======================
Package Name: gogui
Short Description: Graphical user interface to programs that play the board game Go
Owners: jouty
Branches: f17 f18
Comment 39 Jon Ciesla 2013-02-14 17:18:32 EST
Git done (by process-git-requests).
Comment 40 Fedora Update System 2013-02-16 08:21:02 EST
gogui-1.4.6-14.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/gogui-1.4.6-14.fc17
Comment 41 Fedora Update System 2013-02-16 08:21:34 EST
gogui-1.4.6-14.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/gogui-1.4.6-14.fc18
Comment 42 Fedora Update System 2013-02-16 22:21:14 EST
gogui-1.4.6-14.fc17 has been pushed to the Fedora 17 testing repository.
Comment 43 Fedora Update System 2013-02-25 21:52:50 EST
gogui-1.4.6-14.fc17 has been pushed to the Fedora 17 stable repository.
Comment 44 Fedora Update System 2013-02-25 21:55:25 EST
gogui-1.4.6-14.fc18 has been pushed to the Fedora 18 stable repository.
Comment 45 Fedora Update System 2013-04-10 17:06:13 EDT
gogui-1.4.7-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/gogui-1.4.7-1.fc18
Comment 46 Fedora Update System 2013-04-10 17:06:29 EDT
gogui-1.4.7-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/gogui-1.4.7-1.fc17
Comment 47 Fedora Update System 2013-04-10 17:08:23 EDT
gogui-1.4.7-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/gogui-1.4.7-1.fc19