Bug 1388945

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.2.20161103gitb20fa7f.fc25.src.rpm

%changelog
* Wed Nov 09 2016 Martin Gansser <martinkg> - 0.1-0.2.20161103gitb20fa7f
- Update to new git snapshot 0.1-0.2.20161103gitb20fa7f

Hello!  I'm new to the fedora packaging thing, and I am going to try to review this package.

It compiles on my computer, there are no errors in rpmlint and the package works once I installed it.  It is a really slick extension as well.

I was reading the spec file, and I see that the source files are manually enumerated:

    %{_gsextdir}/%{uuid}/metadata.json
    %{_gsextdir}/%{uuid}/extension.js
    %{_gsextdir}/%{uuid}/dbus.js
    %{_gsextdir}/%{uuid}/lib.js
    %{_gsextdir}/%{uuid}/panel.js
    %{_gsextdir}/%{uuid}/player.js
    %{_gsextdir}/%{uuid}/prefs.js
    %{_gsextdir}/%{uuid}/settings.js
    %{_gsextdir}/%{uuid}/widget.js
    %{_gsextdir}/%{uuid}/stylesheet.css

Could you use a wildcard instead?  I worry that the upstream might add new files (as developers like to do - it would be a very normal thing) and the package would ship broken.  Other scripting language based packages, eg. Sugar in python [1], use wildcards for this kind of thing.

[1] https://pkgs.fedoraproject.org/cgit/rpms/sugar.git/tree/sugar.spec#n219

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.3.20161103gitb20fa7f.fc25.src.rpm

%changelog
* Tue Nov 15 2016 Martin Gansser <martinkg> - 0.1-0.3.20161103gitb20fa7f
- Used wildcard instead of manually enumerate files in section files

Sam, can you run fedora-review on this bug?

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.4.20161119git3946aba.fc25.src.rpm

%changelog
* Sat Nov 19 2016 Martin Gansser <martinkg> - 0.1-0.4.20161119git3946aba
- Update to new git snapshot 0.1-0.4.20161119git3946aba

because it easier to update (spectool -g <package.spec) the package with a valid SOURC URL.

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.4.20170201gite7ed852.fc25.src.rpm

%changelog
* Fri Feb 03 2017 Martin Gansser <martinkg> - 0.1-0.4.20170203gite7ed852
- Update to new git snapshot 0.1-0.4.20170203gite7ed852

I can do a formal review. To be succinct, I won't include the MUSTs and SHOULDs that pass.

[!]: License field in the package spec file matches the actual license.

        Your spec file claims the license is GPLv3+, but the package's included
        COPYING file is a copy of the GPLv2. Version 3 might be more accurate,
        because the license comment at the top of widget.js says version 3,
        which should therefore apply to the whole project. Still, COPYING does
        not match your spec. Upstream should resolve this issue.

[!]: Package functions as described.

        The extension works with Rhythmbox, but does nothing if I'm playing
        music with Banshee instead.

        I'm testing this extension based on the latest SRPM you've linked,
        installed on Fedora 25 Workstation x86_64, with GNOME Shell 3.22.3,
        and Banshee 2.6.2. I do have several other extensions installed, so I
        suppose it's possible there's some conflict...

        * gnome-shell-extension-alternate-tab.noarch
        * gnome-shell-extension-apps-menu.noarch
        * gnome-shell-extension-auto-move-windows.noarch
        * gnome-shell-extension-background-logo.noarch
        * gnome-shell-extension-common.noarch
        * gnome-shell-extension-drive-menu.noarch
        * gnome-shell-extension-gpaste.noarch
        * gnome-shell-extension-launch-new-instance.noarch
        * gnome-shell-extension-media-player-indicator.noarch
        * gnome-shell-extension-places-menu.noarch
        * gnome-shell-extension-pomodoro.x86_64
        * gnome-shell-extension-sustmi-windowoverlay-icons.noarch
        * gnome-shell-extension-user-theme.noarch
        * gnome-shell-extension-window-list.noarch
        * gnome-shell-extension-windowsNavigator.noarch

        Curiously, Media Play Indicator does not appear in GNOME Tweak Tool
        either. I had to enable it from the command line with
        gnome-shell-extension-tool.

[x]: Latest version is packaged.

        Hard to say, since the upstream GitHub repo does not tag its releases.
        The commit that you use as the snapshot is recent, so I'm assuming it's
        okay. However, the package versioning guide say that when upstream has
        never tagged a release, the version tag should simply be set to 0 (not
        0.1). Or is the idea here that you're packaging a *prerelease* of the
        eventual upstream 0.1 release?
        <https://fedoraproject.org/wiki/Packaging:Versioning>

[!]: Packages should try to preserve timestamps of original installed files.

        This is not mandatory, but I think it's meant to help with verifying
        the source files. If no one has asked the upstream about this yet, you
        might ask them to consider editing their Makefile to use `cp -p` or
        `install -p` or equivalent.

rpmlint complains about "explicit-lib-dependency glib2". I guess you don't need to include that in your dependencies.

(In reply to Andrew Toskin from comment #8)
> I can do a formal review. To be succinct, I won't include the MUSTs and
> SHOULDs that pass.
> 
> [!]: License field in the package spec file matches the actual license.
> 
>         Your spec file claims the license is GPLv3+, but the package's
> included
>         COPYING file is a copy of the GPLv2. Version 3 might be more
> accurate,
>         because the license comment at the top of widget.js says version 3,
>         which should therefore apply to the whole project. Still, COPYING
> does
>         not match your spec. Upstream should resolve this issue.

opened Ticket:
https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/323

> [!]: Package functions as described.
> 
>         The extension works with Rhythmbox, but does nothing if I'm playing
>         music with Banshee instead.
> 
>         I'm testing this extension based on the latest SRPM you've linked,
>         installed on Fedora 25 Workstation x86_64, with GNOME Shell 3.22.3,
>         and Banshee 2.6.2. I do have several other extensions installed, so I
>         suppose it's possible there's some conflict...
> 
>         * gnome-shell-extension-alternate-tab.noarch
>         * gnome-shell-extension-apps-menu.noarch
>         * gnome-shell-extension-auto-move-windows.noarch
>         * gnome-shell-extension-background-logo.noarch
>         * gnome-shell-extension-common.noarch
>         * gnome-shell-extension-drive-menu.noarch
>         * gnome-shell-extension-gpaste.noarch
>         * gnome-shell-extension-launch-new-instance.noarch
>         * gnome-shell-extension-media-player-indicator.noarch
>         * gnome-shell-extension-places-menu.noarch
>         * gnome-shell-extension-pomodoro.x86_64
>         * gnome-shell-extension-sustmi-windowoverlay-icons.noarch
>         * gnome-shell-extension-user-theme.noarch
>         * gnome-shell-extension-window-list.noarch
>         * gnome-shell-extension-windowsNavigator.noarch
> 
>         Curiously, Media Play Indicator does not appear in GNOME Tweak Tool
>         either. I had to enable it from the command line with
>         gnome-shell-extension-tool.

i activated it with
gnome-shell-extension-prefs

> 
> [x]: Latest version is packaged.
> 
>         Hard to say, since the upstream GitHub repo does not tag its
> releases.
>         The commit that you use as the snapshot is recent, so I'm assuming
> it's
>         okay. However, the package versioning guide say that when upstream
> has
>         never tagged a release, the version tag should simply be set to 0
> (not
>         0.1). Or is the idea here that you're packaging a *prerelease* of the
>         eventual upstream 0.1 release?
>         <https://fedoraproject.org/wiki/Packaging:Versioning>

I think it is a PreRelease ?

> 
> [!]: Packages should try to preserve timestamps of original installed files.
> 
>         This is not mandatory, but I think it's meant to help with verifying
>         the source files. If no one has asked the upstream about this yet,
> you
>         might ask them to consider editing their Makefile to use `cp -p` or
>         `install -p` or equivalent.
> 

Upstream apparently does not know how to change that.
opened Ticket: 
https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/324


> rpmlint complains about "explicit-lib-dependency glib2". I guess you don't
> need to include that in your dependencies.

I have added glib2 as a requirement, so rpmlint is not so notifyable.

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.5.20170308git67e54ae.fc25.src.rpm

%changelog
* Wed Mar 08 2017 Martin Gansser <martinkg> - 0.1-0.5.20170308git67e54ae
- Update to new git snapshot 0.1-0.5.20170308git67e54ae
- Add INSTALL="install -p"

I just noticed that the spec says it requires GNOME 3.20+, but the extension's metadata.json file says it supports 3.16. This maybe isn't a big deal for Fedora, since Fedora 23 reached end-of-life. But allowing GNOME 3.16 would make your package usable in EPEL... if/when they rebase to a version of GNOME newer than 3.14.

In any case, I think it would be better to stick with the upstream's stated supported versions of GNOME, unless you *know* the extension does not actually work with GNOME older than 3.20.


(In reply to mgansser from comment #9)
> i activated it with  gnome-shell-extension-prefs

Turns out my problem with Banshee was that I needed to enable the MPRIS D-Bus Interface extension in the Banshee preferences.
https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/270

I feel like this is going to be a common point of confusion for new users, but I'm not sure how we could avoid it. It's already noted in the packaged README, but how many people read those? Not me, apparently :P

FYI: The method used to activate the extension shouldn't really matter. Launching gnome-shell-extension-prefs, the GNOME Tweak Tool, or running the gnome-shell-extension-tool command line -- it's all the same.

Also, the extension does show up in Tweak Tool now. I had reloaded GNOME Shell (Alt+F2, r, Enter), but maybe it still needed me to logout and login for some reason.


> I think it is a PreRelease ?

I guess that depends on whether upstream ever plans to tag releases, and what they intend their first version number to be. Some projects use 0.1 or 1.0 for their first official release. If they tag their first release as version 0.1, then you're okay: You would simply bump your Release tag from a "prerelease" number (0.x) to a regular release (1). And if they tag version 1.0, then that will always be sorted as newer than version 0.1, so you'd still be good.

However, some developers really like to count from 0, and tag their first release as 0.0 -- and *that* would be a problem for your package, because RPM won't recognize the tagged version 0.0 as being newer than the version "0.1" you've been packaging. You would have to add an Obsoletes tag or something, and things would get messy...

So if upstream doesn't plan on ever tagging their releases, or if they are undecided on their versioning scheme in any way, I would set the spec's Version to plain 0. And continue with the "prerelease" Release tag you're using, unless or until upstream formally designates an actual version.

(In reply to Andrew Toskin from comment #10)
> I just noticed that the spec says it requires GNOME 3.20+, but the
> extension's metadata.json file says it supports 3.16. This maybe isn't a big
> deal for Fedora, since Fedora 23 reached end-of-life. But allowing GNOME
> 3.16 would make your package usable in EPEL... if/when they rebase to a
> version of GNOME newer than 3.14.
> 
> In any case, I think it would be better to stick with the upstream's stated
> supported versions of GNOME, unless you *know* the extension does not
> actually work with GNOME older than 3.20.


Will stay with the support of GNOME 3.20 as F24 EOL is soon reached.

> (In reply to mgansser from comment #9)
> > i activated it with  gnome-shell-extension-prefs
> 
> Turns out my problem with Banshee was that I needed to enable the MPRIS
> D-Bus Interface extension in the Banshee preferences.
> https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/270
> 
> I feel like this is going to be a common point of confusion for new users,
> but I'm not sure how we could avoid it. It's already noted in the packaged
> README, but how many people read those? Not me, apparently :P
> 
> FYI: The method used to activate the extension shouldn't really matter.
> Launching gnome-shell-extension-prefs, the GNOME Tweak Tool, or running the
> gnome-shell-extension-tool command line -- it's all the same.
> 
> Also, the extension does show up in Tweak Tool now. I had reloaded GNOME
> Shell (Alt+F2, r, Enter), but maybe it still needed me to logout and login
> for some reason.


Thanks for the info, works for me too.

> > I think it is a PreRelease ?
> 
> I guess that depends on whether upstream ever plans to tag releases, and
> what they intend their first version number to be. Some projects use 0.1 or
> 1.0 for their first official release. If they tag their first release as
> version 0.1, then you're okay: You would simply bump your Release tag from a
> "prerelease" number (0.x) to a regular release (1). And if they tag version
> 1.0, then that will always be sorted as newer than version 0.1, so you'd
> still be good.
> 
> However, some developers really like to count from 0, and tag their first
> release as 0.0 -- and *that* would be a problem for your package, because
> RPM won't recognize the tagged version 0.0 as being newer than the version
> "0.1" you've been packaging. You would have to add an Obsoletes tag or
> something, and things would get messy...
> 
> So if upstream doesn't plan on ever tagging their releases, or if they are
> undecided on their versioning scheme in any way, I would set the spec's
> Version to plain 0. And continue with the "prerelease" Release tag you're
> using, unless or until upstream formally designates an actual version.

Use Version 0 as you mentioned, until upstream formally designates an actual version.

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0-0.6.20170308git67e54ae.fc25.src.rpm

%changelog
* Thu Mar 09 2017 Martin Gansser <martinkg> - 0-0.6.20170308git67e54ae
- Use Version 0 until upstream formally designates an actual version

Great, thanks. I think we're now just waiting on the issues we filed with the upstream GitHub.

About preserving the file timestamps
(https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/324),
could you do the pull-request? I do think it should just be a matter of find/replace -- more time will be spent forking, cloning, testing that `make` still succeeds, pushing, and opening the pull request, and I'm juggling a few other things this week.

(In reply to Andrew Toskin from comment #12)
> Great, thanks. I think we're now just waiting on the issues we filed with
> the upstream GitHub.
> 
> About preserving the file timestamps
> (https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/
> 324),
> could you do the pull-request? I do think it should just be a matter of
> find/replace -- more time will be spent forking, cloning, testing that
> `make` still succeeds, pushing, and opening the pull request, and I'm
> juggling a few other things this week.

done
https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/pull/325

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0-0.7.20170311git0810df8.fc25.src.rpm

%changelog
* Sat Mar 11 2017 Martin Gansser <martinkg> - 0-0.7.20170311git0810df8
- Update to new git snapshot 0.1-0.5.20170308git0810df8
- Clanup specfile
- Add pushd/popd
- Update license field to GPLv2+

Okay, sorry, NOW I think think there's just one last issue -- managing gschemas -- with a couple subpoints.


## Compiling the gsettings schemas

According to this section of the Packaging wiki <https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:ScriptletSnippets#GSettings_Schema>, you shouldn't need to compile the gschemas yourself anymore. Bug #1409315 is an open thread about removing the post-transaction schema compilation rule from fedora-review.

This is just FYI. I think you may choose to keep the transaction scriptlets for now, if you wish to avoid the glib warnings from fedora-review, at least until fedora-review gets updated. But unless I'm missing some information here, the scriptlet will be removed in the future.


## Dependencies

"BuildRequires: glib2-devel" is needed because the source configure script includes "GLIB_GSETTINGS". This part seems a little obscure to me, but I'm guessing it's used for compiling gschemas. Upstream probably needs this for users who will only install this extension to their home directory. (You'll also need to (Build)Require glib2 for compiling schemas in the post-transaction scriptlet, but as noted above, that's on its way out.) So unless upstream were interested in figuring out how to make that part optional, you will need to keep the BuildRequire.

However, "Requires: glib2" is definitely not needed. This is why rpmlint returns the error "explicit-lib-dependency glib2". RPM already detects this dependency for you. I don't personally think slightly overstating the dependencies here should be a big deal, but rpmlint marks this as an *error* rather than a warning, so I really do think you should remove it.


Once this last minor issue is resolved, I'll approve the package :)

(In reply to Andrew Toskin from comment #15)
> Okay, sorry, NOW I think think there's just one last issue -- managing
> gschemas -- with a couple subpoints.
> 
> 
> ## Compiling the gsettings schemas
> 
> According to this section of the Packaging wiki
> <https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:
> ScriptletSnippets#GSettings_Schema>, you shouldn't need to compile the
> gschemas yourself anymore. Bug #1409315 is an open thread about removing the
> post-transaction schema compilation rule from fedora-review.
> 
> This is just FYI. I think you may choose to keep the transaction scriptlets
> for now, if you wish to avoid the glib warnings from fedora-review, at least
> until fedora-review gets updated. But unless I'm missing some information
> here, the scriptlet will be removed in the future.

keept the transaction scriptlets for now, as you mentioned.

> ## Dependencies
> 
> "BuildRequires: glib2-devel" is needed because the source configure script
> includes "GLIB_GSETTINGS". This part seems a little obscure to me, but I'm
> guessing it's used for compiling gschemas. Upstream probably needs this for
> users who will only install this extension to their home directory. (You'll
> also need to (Build)Require glib2 for compiling schemas in the
> post-transaction scriptlet, but as noted above, that's on its way out.) So
> unless upstream were interested in figuring out how to make that part
> optional, you will need to keep the BuildRequire.
> 
> However, "Requires: glib2" is definitely not needed. This is why rpmlint
> returns the error "explicit-lib-dependency glib2". RPM already detects this
> dependency for you. I don't personally think slightly overstatin%changelog
* Tue Mar 14 2017 Martin Gansser <martinkg> - 0-0.8.20170314git61d9118
- Update to new git snapshot 0.1-0.8.20170314git61d9118
- Remove RR glib2g the
> dependencies here should be a big deal, but rpmlint marks this as an *error*
> rather than a warning, so I really do think you should remove it.

glib2 removed
> 
> Once this last minor issue is resolved, I'll approve the package :)

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0-0.8.20170314git61d9118.fc25.src.rpm


%changelog
* Tue Mar 14 2017 Martin Gansser <martinkg> - 0-0.8.20170314git61d9118
- Update to new git snapshot 0.1-0.8.20170314git61d9118
- Remove RR glib2

Just noticed your spec's changelogs are still using version 0.1 for the package. Should probably fix that too.

But anyway, APPROVED!

(In reply to Andrew Toskin from comment #17)
> Just noticed your spec's changelogs are still using version 0.1 for the
> package. Should probably fix that too.

already fixed.
> 
> But anyway, APPROVED!

thanks for your review !

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/gnome-shell-extension-media-player-indicator

package has been built successfully on fc24, fc25, fc26 and rawhide.