Bug 459916 (freedink-dfarc) - Review Request: freedink-dfarc - Frontend and .dmod installer for GNU FreeDink
Summary: Review Request: freedink-dfarc - Frontend and .dmod installer for GNU FreeDink
Keywords:
Status: CLOSED NEXTRELEASE
Alias: freedink-dfarc
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: freedink
TreeView+ depends on / blocked
 
Reported: 2008-08-24 13:51 UTC by Sylvain Beucler
Modified: 2013-01-13 14:46 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-10-06 13:54:57 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Sylvain Beucler 2008-08-24 13:51:15 UTC
Spec URL: http://www.freedink.org/snapshots/fedora/dfarc.spec
SRPM URL: http://www.freedink.org/snapshots/fedora/dfarc-2.99.20080823-1.fc8.src.rpm
Description: Frontend and .dmod installer for GNU FreeDink
DFArc2 makes it easy to play and manage the Dink Smallwood game and
it's numerous Dink Modules (or D-Mods).

Comment 1 Mamoru TASAKA 2008-09-03 07:27:32 UTC
Dependency loop:
dfarc Requires freedink and freedink Requires dfarc

Would you resolve this first unless this is very intentional?

Comment 2 Sylvain Beucler 2008-09-03 07:52:28 UTC
Ah, interesting question.

This is intentional, but hopefully there's a cleaner way to do it.
A user installing 'dfarc', the front-end, will want to install freedink as well. Similarly, somebody installing 'freedink' will want to install the front-end to play extensions or start the game in windowed mode (for example).

Typically, the engine and the frontend are shipped in the same .exe installer in the ms windows version. Still, they are  developped independently, so they are provided as 2 separate packages for GNU/Linux.

In the Debian packaging I used the Recommends: field, which is not a hard dependency, but it is treated as a dependency on desktop systems.

What would you recommend to do?

Comment 3 Sylvain Beucler 2008-09-03 07:53:33 UTC
(updating the title as the latest version changed the package name)

Comment 4 Mamoru TASAKA 2008-09-03 08:15:23 UTC
(In reply to comment #2)
> Ah, interesting question.
> 
> This is intentional, but hopefully there's a cleaner way to do it.
> A user installing 'dfarc', the front-end, will want to install freedink as
> well. Similarly, somebody installing 'freedink' will want to install the
> front-end to play extensions or start the game in windowed mode (for example).

For me it seems that you are saying
- With freedink we can play some games, however some additional functions
  (like some extentions, modes in window mode) are not available
- With freedink-dfarc we can play freedink in some additional effects, however
  freedink-dfrac always requires freedink.

If I am correct, I recommend to remove "Requires: (freedink-)dfarc" from 
freedink srpm and keep "Requires: freedink" in freedink-dfarc srpm.

By the way, share/README and COPYING.DFArc-2.0 seem to be saying that
share/pixmaps/dfarc.png is licensed under COPYING.DFArc-2.0 and
it doesn't seem COPYING.DFArc-2.0 is enough free. How do you think?

Comment 5 Sylvain Beucler 2008-09-03 09:14:21 UTC
I think that people installing 'freedink' will expect to get the front-end.

I think it would be best to have:
- freedink (empty package) - Requires: freedink-engine, freedink-dfarc
- freedink-engine - Requires: freedink-data
- dfarc - Requires: <none>
- freedink-data - Requires: <none>

What do you think?

> By the way, share/README and COPYING.DFArc-2.0 seem to be saying that
> share/pixmaps/dfarc.png is licensed under COPYING.DFArc-2.0 and
> it doesn't seem COPYING.DFArc-2.0 is enough free.  How do you think?

The license looks free enough to me.  Were you confused by the stanza about the "Dink Media" (which references a completely different set of files, and which was incidentally released this month under the zlib license, cf. #459915)?

Comment 6 Mamoru TASAKA 2008-09-03 10:25:15 UTC
(In reply to comment #5)
> I think that people installing 'freedink' will expect to get the front-end.
> 
> I think it would be best to have:
> - freedink (empty package) - Requires: freedink-engine, freedink-dfarc
> - freedink-engine - Requires: freedink-data
> - dfarc - Requires: <none>
> - freedink-data - Requires: <none>
> 
> What do you think?

If you are okay with this, I have no objections.

> > By the way, share/README and COPYING.DFArc-2.0 seem to be saying that
> > share/pixmaps/dfarc.png is licensed under COPYING.DFArc-2.0 and
> > it doesn't seem COPYING.DFArc-2.0 is enough free.  How do you think?
> 
> The license looks free enough to me.  Were you confused by the stanza about the
> "Dink Media" (which references a completely different set of files, and which
> was incidentally released this month under the zlib license, cf. #459915)?

Because the license does not _define_ what "graphics" are under the influence
of the license precisely. You may guess share/pixmaps/dfarc.png is not under this license,
however as I said before as far as I read the source codes.
- share/README seems to be saying that dfarc.png is under the license
- COPYING.DFArc-2.0 says that all files from the old (?) 2.0 dfarc source are under this
  license
- the license says that "graphics" files have non-free restriction
- And from the license I cannot make it sure what files this file refers to as the "graphics".

So for now I will guess that as dfarc.png is "graphics" file this file has non-free
restriction.

Comment 7 Mamoru TASAKA 2008-09-13 05:50:50 UTC
Any news?

Comment 8 Sylvain Beucler 2008-09-14 21:27:07 UTC
New spec: http://www.freedink.org/snapshots/fedora/freedink-dfarc.spec

dfarc is renamed to freedink-dfarc to suit the new dependency/naming scheme.
There're a few caveats due to src_name != pkg_name.
I haven't had the opportunity to discuss dfarc.png clarification yet.

Comment 9 Mamoru TASAKA 2008-09-17 07:47:12 UTC
Once setting FE-Legal until the license of dfarc.png is clarified.

Comment 10 Sylvain Beucler 2008-09-21 10:40:41 UTC
Hi,

Here's a new release:
http://www.freedink.org/snapshots/fedora/freedink-dfarc.spec
http://www.freedink.org/snapshots/fedora/freedink-dfarc-3.2-1.src.rpm

- opensuse parts removed
(for reference: sed '/%if 0%{?suse_version}/,/%endif/d' < freedink.spec)

- icon replaced

Comment 11 Mamoru TASAKA 2008-09-21 13:00:34 UTC
For this package, please check BuildRequires again.

Comment 12 Sylvain Beucler 2008-09-21 13:49:37 UTC
> For this package, please check BuildRequires again.

I said it would be error-prone, didn't I? ;)

http://www.freedink.org/snapshots/fedora/freedink-dfarc.spec
http://www.freedink.org/snapshots/fedora/freedink-dfarc-3.2-1.src.rpm

Comment 13 Mamoru TASAKA 2008-09-21 17:03:10 UTC
Removing FE-Legal.

Comment 14 Mamoru TASAKA 2008-09-21 17:13:51 UTC
For 3.2-1

* Source0
  - seems 404.

* Dependency
  - build.log shows:
------------------------------------------------------
   140  checking for wxglade... 
   141  no
   142  configure: WARNING: You need to install wxglade
------------------------------------------------------
    Perhaps "Requires: wxGlade" is needed?

* Timestamps
  - Please consider to use
------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
------------------------------------------------------
    to keep timestamps on installed files as much as possible.
    This method usually works for Makefiles generated from
    recent autotools.

* Desktop file
  - Any installed desktop files must be treated by
    desktop-file-{install,validate}:
    https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage

* Scriptlets
  - As a XML file is installed under %_datadir/mime/packages/,
    mime data must be updated:
    https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo

* Documents
  - Please add "ChangeLog" to %doc. Also, doc/dfarc.txt can be added
    to %doc.

* Directory ownership issue
  - %_datadir/icons/hicolor (and directories under this directories) are
    already owned by hicolor-icon-theme and should not be owned by
    this package.

* %changelog format
---------------------------------------------------------
freedink-dfarc.i386: W: incoherent-version-in-changelog 3.2 3.2-1.fc10
---------------------------------------------------------
  - %changelog should contain EVR (Epoch-Version-Release) information
    (not just Epoch-Version) (%dist information can be removed):
    https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

Comment 15 Mamoru TASAKA 2008-09-21 17:19:57 UTC
For other packages I will check them later.

Comment 16 Sylvain Beucler 2008-09-21 19:09:37 UTC
Hi,

I'd appreciate to get all problems at once, not one after the other.
Most of these issues were already here a month ago.
If there is *anything* else to complain about, please let me know now.

Thanks.

Comment 17 Mamoru TASAKA 2008-09-22 04:03:32 UTC
I am always trying to find out all problems at once, however as I am a
human it may happen that some issues are overlooked at one time of
my check (and this also happens to reviews done by other reviewers).

Note that usually when the package doesn't even build or there are
any license issues on the package review procedure usually stops there.
Reviewers usually say that "the package doesn't build, so the review
cannot be proceeded" or "the review is postponed until the license
issue is resolved".

Also there are many cases in which fixes to comments by reviewers
will bring about new problems or show other problems which could
not be seen.

So please fix your package now so that I can recheck your srpm
again.

Comment 18 Sylvain Beucler 2008-09-23 20:35:49 UTC
(In reply to comment #14)
> For 3.2-1
> 
> * Source0
>   - seems 404.

Fixed with new proper release.

> * Dependency
>   - build.log shows:
> ------------------------------------------------------
>    140  checking for wxglade... 
>    141  no
>    142  configure: WARNING: You need to install wxglade
> ------------------------------------------------------
>     Perhaps "Requires: wxGlade" is needed?

Clarified, this is a developer tool, only needed if you want to modify the UI files.

> * Timestamps
>   - Please consider to use
> ------------------------------------------------------
> make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
> ------------------------------------------------------
>     to keep timestamps on installed files as much as possible.
>     This method usually works for Makefiles generated from
>     recent autotools.

Done.

> * Desktop file
>   - Any installed desktop files must be treated by
>     desktop-file-{install,validate}:
>    
> https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage

Done.

> * Scriptlets
>   - As a XML file is installed under %_datadir/mime/packages/,
>     mime data must be updated:
>     https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo

Done.

> * Documents
>   - Please add "ChangeLog" to %doc. Also, doc/dfarc.txt can be added
>     to %doc.

I added ChangeLog. I didn't feel like including doc/dfarc.txt since it's developer information.

> * Directory ownership issue
>   - %_datadir/icons/hicolor (and directories under this directories) are
>     already owned by hicolor-icon-theme and should not be owned by
>     this package.

I think I fixed it by being more precise in the %files section:
-%{_datadir}/icons/*
+%{_datadir}/icons/hicolor/32x32/mimetypes/*

> * %changelog format
> ---------------------------------------------------------
> freedink-dfarc.i386: W: incoherent-version-in-changelog 3.2 3.2-1.fc10
> ---------------------------------------------------------
>   - %changelog should contain EVR (Epoch-Version-Release) information
>     (not just Epoch-Version) (%dist information can be removed):
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

Oops, fixed.


http://www.freedink.org/snapshots/fedora-review/freedink-dfarc.spec
http://www.freedink.org/snapshots/fedora-review/freedink-dfarc-3.2.1-1.fc8.src.rpm



By the way, under F9 and mock, I keep getting rpmlint errors about g+w directories:
freedink-dfarc.i386: E: non-standard-dir-perm /usr/share/doc/freedink-dfarc-3.2.1 02755
A standard directory should have permission set to 0755. If you get this
message, it means that you have wrong directory permissions in some dirs
included in your package.

Any clue?

Comment 19 Mamoru TASAKA 2008-09-24 08:00:14 UTC
(In reply to comment #18)
> By the way, under F9 and mock, I keep getting rpmlint errors about g+w
> directories:
> freedink-dfarc.i386: E: non-standard-dir-perm
> /usr/share/doc/freedink-dfarc-3.2.1 02755

I don't get such strange behavoir...

For 3.2.1-1:

* scriptlet
---------------------------------------------------
    %post
    # http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database
    update-desktop-database &> /dev/null || :
    # http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GTK.2B_icon_cache
    touch --no-create %{_datadir}/icons/hicolor
    if [ -x %{_bindir}/gtk-update-icon-cache ] ; then
      %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
    # http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo
!!! update-mime-database %{_datadir}/mime &> /dev/null || :
    fi
---------------------------------------------------
    The line marked as !!! is inside of if-fi structure (same for %postun)

* macros in comment
---------------------------------------------------
# Don't install desktop files, use %post instead
---------------------------------------------------
  - In the comment (and %changelog), use %% instead of % to avoid
    unwanted macros expansion.

* %changelog
  - Please keep the old %changelog entry

Fix the issues above, all others are good.

-------------------------------------------------------------
    This package (freedink-dfarc) is APPROVED by mtasaka
-------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
After you request for sponsorship a mail will be sent to sponsor 
members automatically (which is invisible for you) which notifies 
that you need a sponsor. After that, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 8/9, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Comment 20 Sylvain Beucler 2008-09-24 20:15:37 UTC
Thanks for the quick reply.

Updated package:
http://www.freedink.org/snapshots/fedora-review/freedink-dfarc.spec
http://www.freedink.org/snapshots/fedora-review/freedink-dfarc-3.2.1-2.fc8.src.rpm

* Wed Sep 24 2008 Sylvain Beucler <beuc> - 3.2.1-2
- Fix update-mime-database call (was conditional due to typo)
- Fix macros in comments
- Tidy changelog


For the sake of accepting the Fedora Contributor License Agreement (which is apparently necessary to request sponsorship?) I hereby remind that DFArc is Not a Contribution, and that freedink-dfarc.spec is a Contribution.

Comment 21 Sylvain Beucler 2008-09-24 20:21:39 UTC
I requested for membership to group 'packager' in the FAS (is it what you meant by "request for sponsorship"?).
My account login is "beuc".

Comment 22 Mamoru TASAKA 2008-09-25 03:04:37 UTC
Okay, now I am sponsoring you. Please follow "Join" wiki again.

Removing NEEDSPONSOR.

Comment 23 Mamoru TASAKA 2008-09-26 04:37:39 UTC
Please write CVS request also for this review request and freedink-data.

Comment 24 Sylvain Beucler 2008-09-26 06:47:55 UTC
Sorry, I was waiting for the first request to be complete to request the other ones (just in case I did something wrong, to avoid doing the mistake 3x).

CVS maintainers, would you mind creating this module? :)

New Package CVS Request
=======================
Package Name: freedink-dfarc
Short Description: Frontend and .dmod installer for GNU FreeDink
Owners: beuc
Branches: F-8 F-9
InitialCC: beuc

Comment 25 Kevin Fenzi 2008-09-28 19:22:58 UTC
cvs done.

Comment 26 Fedora Update System 2008-10-05 16:03:16 UTC
freedink-dfarc-3.2.1-2.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/freedink-dfarc-3.2.1-2.fc9

Comment 27 Fedora Update System 2008-10-05 16:23:00 UTC
freedink-dfarc-3.2.1-2.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/freedink-dfarc-3.2.1-2.fc8

Comment 28 Mamoru TASAKA 2008-10-06 13:54:57 UTC
Thanks. Now closing.

When you think the submitted packages can be moved to stable repository,
please modify (edit) the request on bodhi.

Comment 29 Sylvain Beucler 2008-10-06 18:37:17 UTC
Thanks for your review :)

I'll wait for the 'testing' update request to be moderated.

Comment 30 Fedora Update System 2008-10-20 20:27:38 UTC
freedink-dfarc-3.2.1-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 31 Fedora Update System 2008-10-20 22:06:53 UTC
freedink-dfarc-3.2.1-2.fc8 has been pushed to the Fedora 8 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.