Bug 1377631

Summary: Review Request: gnome-shell-extension-netspeed - A gnome-shell extension to show speed of the internet
Product: [Fedora] Fedora Reporter: MartinKG <mgansser>
Component: Package ReviewAssignee: Andrew Toskin <andrew>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: andrew, mgansser, package-review, sam
Target Milestone: ---Flags: andrew: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-03-02 13:12:52 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description MartinKG 2016-09-20 08:58:00 UTC
Spec URL: https://www.dropbox.com/s/0cfb4boq3dhclu9/gnome-shell-extension-netspeed.spec?dl=0
SRPM URL: https://www.dropbox.com/s/qsc4khyihlxnm9h/gnome-shell-extension-netspeed-3.20-1.20160806git16a25ec.fc24.src.rpm?dl=0

Description: Add an Internet speed indicator to status area.
Fedora Account System Username: martinkg

%changelog
* Fri Sep 16 2016 Martin Gansser <martinkg@fedoraproject.org> - 3.20-1.20160806git16a25ec
- Update to 3.20

rpmlint -i gnome-shell-extension-netspeed.spec /home/martin/rpmbuild/SRPMS/gnome-shell-extension-netspeed-3.20-1.20160806git16a25ec.fc24.src.rpm /home/martin/rpmbuild/RPMS/noarch/gnome-shell-extension-netspeed-3.20-1.20160806git16a25ec.fc24.noarch.rpm
2 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 1 leigh scott 2016-09-21 07:54:39 UTC
Release is wrong as it's git

change from

1.%{gitdate}git%{shortcommit}%{?dist}


to

0.1.%{gitdate}git%{shortcommit}%{?dist}


As for the version, don't guess and make it up.

Version:        3.20

3.16 was the last release so use

Version:        3.17

Comment 2 MartinKG 2016-09-21 08:20:23 UTC
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-netspeed.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-netspeed-3.17-0.2.20160806git16a25ec.fc24.src.rpm

%changelog
* Wed Sep 21 2016 Martin Gansser <martinkg@fedoraproject.org> - 3.17-0.2.20160806git16a25ec
- Use correct git release version
- Use correct version 3.17

Comment 4 Sam P. 2016-11-15 09:35:22 UTC
What is the status of the upstream for this extension?  The spec applies the patch version 26 patch [1], which is noted as "approved" and "active" on extensions.gnome.org.  It was approved by the user EGO "hedayaty".

But the package is based on the git repository, which contains version 25 [2].  However the git repo is maintained by GitHub user "hedayaty" - the same person who applied the patch on EGO.

I opened an issue asking them what is going on, because right now it is confusing and odd:  https://github.com/hedayaty/NetSpeed/issues/52

Anyway, the spec file looks good to my untrained eyes and the extension works with the built rpm.

[1]  https://extensions.gnome.org/review/6097
[2] https://github.com/hedayaty/NetSpeed/blob/master/metadata.json#L9

Comment 5 Andrew Toskin 2017-03-16 08:29:14 UTC
I don't know where Leigh came up with incrementing the version from 3.16 to 3.17. As Sam said, according to the extension's metadata.json file, the version is either 25 or 26, depending on whether you're getting it from the extensions website, or from GitHub... And since the spec uses commit tags on GitHub as the Source0, I suppose it's a "pre-release" of version 26 (even though 26 has been released elsewhere).

Other issues:

[x]: glib-compile-schemas is run in %postun and %posttrans if package has
     *.gschema.xml files.

        You correctly compile the gschema in the system glib directory, so
        you don't need to include the precompiled copy (or even the
        directory) at:
        %{builddir}/%{_datadir}/gnome-shell/extensions/%{uuid}/schemas/

        You can remove it at the end of the %install section, probably
        just before %find_lang would be best.

[?]: The spec file handles locales properly.

        Looks to me like the locale files are all in the right place, but
        when I changed my system language to French and German, the settings
        widget was still all in English.

        This is *probably* a problem upstream, since the version
        installed from the EGO site doesn't seem to translate either.

        https://github.com/hedayaty/NetSpeed/issues/55

[!]: Requires correct, justified where necessary.

        Should require GNOME Shell 3.10+, according to metadata.json's
        list of compatible versions of GNOME.

        For any GNOME Shell extension, you should also list
        gnome-shell-extension-common as a dependency.

[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.

        There's an open issue about this:
        https://github.com/hedayaty/NetSpeed/issues/50


The patch which adds newer versions of GNOME to the compatibility list should hopefully be taken care of upstream:

https://github.com/hedayaty/NetSpeed/issues/42

Comment 6 leigh scott 2017-03-16 08:44:34 UTC
(In reply to Andrew Toskin from comment #5)
> I don't know where Leigh came up with incrementing the version from 3.16 to
> 3.17. 


The changelog declares the current version as 3.16

https://github.com/hedayaty/NetSpeed/blob/master/CHANGELOG

so packaging any git snapshot should be bumped by +1 to the minor

> As Sam said, according to the extension's metadata.json file, the
> version is either 25 or 26, depending on whether you're getting it from the
> extensions website, or from GitHub... And since the spec uses commit tags on
> GitHub as the Source0, I suppose it's a "pre-release" of version 26 (even
> though 26 has been released elsewhere).

Comment 7 MartinKG 2017-03-18 10:09:48 UTC
(In reply to Andrew Toskin from comment #5)
> I don't know where Leigh came up with incrementing the version from 3.16 to
> 3.17. As Sam said, according to the extension's metadata.json file, the
> version is either 25 or 26, depending on whether you're getting it from the
> extensions website, or from GitHub... And since the spec uses commit tags on
> GitHub as the Source0, I suppose it's a "pre-release" of version 26 (even
> though 26 has been released elsewhere).
> 
> Other issues:
> 
> [x]: glib-compile-schemas is run in %postun and %posttrans if package has
>      *.gschema.xml files.
> 
>         You correctly compile the gschema in the system glib directory, so
>         you don't need to include the precompiled copy (or even the
>         directory) at:
>         %{builddir}/%{_datadir}/gnome-shell/extensions/%{uuid}/schemas/
> 
>         You can remove it at the end of the %install section, probably
>         just before %find_lang would be best.

done
> 
> [?]: The spec file handles locales properly.
> 
>         Looks to me like the locale files are all in the right place, but
>         when I changed my system language to French and German, the settings
>         widget was still all in English.
> 
>         This is *probably* a problem upstream, since the version
>         installed from the EGO site doesn't seem to translate either.
> 
>         https://github.com/hedayaty/NetSpeed/issues/55

I have also determined, but still no solution. Upstream is dead ?

> [!]: Requires correct, justified where necessary.
> 
>         Should require GNOME Shell 3.10+, according to metadata.json's
>         list of compatible versions of GNOME.
> 
>         For any GNOME Shell extension, you should also list
>         gnome-shell-extension-common as a dependency.

done
> 
> [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.
> 
>         There's an open issue about this:
>         https://github.com/hedayaty/NetSpeed/issues/50

added link to ticket.
> 
> The patch which adds newer versions of GNOME to the compatibility list
> should hopefully be taken care of upstream:
> 
> https://github.com/hedayaty/NetSpeed/issues/42
done

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-netspeed.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-netspeed-3.17-0.4.20160806git16a25ec.fc25.src.rpm

Comment 8 MartinKG 2017-03-20 08:28:04 UTC
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-netspeed.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-netspeed-3.17-0.5.20160806git16a25ec.fc25.src.rpm

%changelog
* Mon Mar 20 2017 Martin Gansser <martinkg@fedoraproject.org> - 3.17-0.5.20160806git16a25ec
- Add fix_gettext-domain.patch

Comment 9 Andrew Toskin 2017-03-20 21:21:57 UTC
(In reply to leigh scott from comment #6)
> The changelog declares the current version as 3.16
> 
> https://github.com/hedayaty/NetSpeed/blob/master/CHANGELOG

I'd interpreted the version numbers there to be the version of GNOME Shell with which they were working to be compatible. This would be an abuse of the usual CHANGELOG format, but would explain why:

* Each listed releases seems to skip several releases -- not just odd/even releases like the current GNOME convention, but often *four* "minor" releases at a time. 
* The "Oct 12 2013" release has two different versions, which match the explicitly referenced GNOME Shell versions in that release's description.
* A version number of 3.x matching the GNOME scheme conflicts with the version number used in metadata.json which is a single integer, currently in the mid 20s.

...But maybe that's also something upstream should clarify. I've added another comment to this issue:

  https://github.com/hedayaty/NetSpeed/issues/52

I'm beginning to have doubts that upstream is ever going to answer, though.

Comment 10 Andrew Toskin 2017-03-21 00:18:19 UTC
Anyway, more review:

Line 17 of the spec: Source1 tag is missing a colon.


You're still including the schemas in the extension's own directory. You don't need this. Spec file line 50:

  mkdir -p %{buildroot}%{_datadir}/gnome-shell/extensions/%{uuid}/schemas

Lines 56 and 57:

  install -Dp -m 0644 schemas/gschemas.compiled \
      %{buildroot}%{_datadir}/gnome-shell/extensions/%{uuid}/schemas/gschemas.compiled


Everything else looks good to me, except for the pending issues on GitHub.

Comment 11 MartinKG 2017-03-21 08:11:04 UTC
(In reply to Andrew Toskin from comment #10)
> Anyway, more review:
> 
> Line 17 of the spec: Source1 tag is missing a colon.
> 

removed
> 
> You're still including the schemas in the extension's own directory. You
> don't need this. Spec file line 50:
> 
>   mkdir -p %{buildroot}%{_datadir}/gnome-shell/extensions/%{uuid}/schemas
> 
> Lines 56 and 57:
> 
>   install -Dp -m 0644 schemas/gschemas.compiled \
>      
> %{buildroot}%{_datadir}/gnome-shell/extensions/%{uuid}/schemas/gschemas.
> compiled

removed
> 
> Everything else looks good to me, except for the pending issues on GitHub.


Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-netspeed.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-netspeed-3.17-0.6.20160806git16a25ec.fc25.src.rpm

%changelog
* Tue Mar 21 2017 Martin Gansser <martinkg@fedoraproject.org> - 3.17-0.6.20160806git16a25ec
- Add missing colon at Source1 tag
- Remove schemas in the extension's own directory

Comment 12 MartinKG 2017-03-26 09:29:29 UTC
(In reply to Andrew Toskin from comment #10)
> Anyway, more review:
> 
> Line 17 of the spec: Source1 tag is missing a colon.
> 
> 
> You're still including the schemas in the extension's own directory. You
> don't need this. Spec file line 50:
> 
>   mkdir -p %{buildroot}%{_datadir}/gnome-shell/extensions/%{uuid}/schemas
> 
> Lines 56 and 57:
> 
>   install -Dp -m 0644 schemas/gschemas.compiled \
>      
> %{buildroot}%{_datadir}/gnome-shell/extensions/%{uuid}/schemas/gschemas.
> compiled
> 
> 
> Everything else looks good to me, except for the pending issues on GitHub.

I have made your suggestion, but I get the following error message when running the Preferences dialog of gnome-tweak-tool or gnome-shell-extension-prefs:

"GLib.FileError: Failed to open file '/usr/share/gnome-shell/extensions/netspeed@hedayaty.gmail.com/schemas/gschemas.compiled': open() failed: No such file or directory

Stack trace:
  @/usr/share/gnome-shell/extensions/netspeed@hedayaty.gmail.com/prefs.js:31
  Application<._getExtensionPrefsModule@resource:///org/gnome/shell/extensionPrefs/main.js:74
  wrapper@resource:///org/gnome/gjs/modules/lang.js:178
  Application<._selectExtension@resource:///org/gnome/shell/extensionPrefs/main.js:89
  wrapper@resource:///org/gnome/gjs/modules/lang.js:178
  Application<._extensionFound/<@resource:///org/gnome/shell/extensionPrefs/main.js:202
  main@resource:///org/gnome/shell/extensionPrefs/main.js:377
  @<main>:1"

upstream ticket: https://github.com/hedayaty/NetSpeed/issues/56

Comment 13 MartinKG 2017-03-29 07:19:37 UTC
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-netspeed.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-netspeed-3.17-0.7.20160806git16a25ec.fc25.src.rpm

%changelog
* Wed Mar 29 2017 Martin Gansser <martinkg@fedoraproject.org> - 3.17-0.7.20160806git16a25ec
- Add netspeed_schema.patch

Comment 14 MartinKG 2017-07-19 12:53:23 UTC
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-netspeed.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-netspeed-3.17-0.8.20170719git0b769d5.fc26.src.rpm

%changelog
* Wed Jul 19 2017 Martin Gansser <martinkg@fedoraproject.org> - 3.17-0.8.20170719git0b769d5
- Update to recent git version 3.17-0.8.20170719git0b769d5

Comment 15 Andrew Toskin 2017-07-21 05:25:37 UTC
There are a couple important issues still open upstream, but it's nice to see things moving again :)

Comment 16 MartinKG 2018-02-07 19:29:11 UTC
@Andrew I think we can go ahead with the review.

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-netspeed.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-netspeed-3.26-0.1.20180207git02a51b2.fc27.src.rpm

%changelog
* Wed Feb 07 2018 Martin Gansser <martinkg@fedoraproject.org> - 3.26-0.1.20180207git02a51b2
- Update to recent git version 3.26-0.1.20180207git02a51b2

Comment 17 Andrew Toskin 2018-02-10 23:18:40 UTC
I'm pretty sure 3.26 is supposed to be the latest version of GNOME Shell which the extension was tested against. GNOME Shell *extensions* all have a metadata.json file, which includes a "version" property. This is supposed to state the version of the extension itself, and for NetSpeed, that property is 28.

https://github.com/hedayaty/NetSpeed/blob/master/metadata.json

I've tried asking the upstream developer, hedayaty, to git-tag his releases. Or at least to explain to us how to know when the code is stable enough for release. But he doesn't seem to get it, and I'm not sure how else to explain what I'm asking about :/

So, I guess continue with your %{gitdate} and %{shortcommit} Releases. But I'm pretty sure the spec Version should be 28.

I'll double check everything else one last time when that's done.

Comment 18 MartinKG 2018-02-11 09:04:25 UTC
(In reply to Andrew Toskin from comment #17)
> I'm pretty sure 3.26 is supposed to be the latest version of GNOME Shell
> which the extension was tested against. GNOME Shell *extensions* all have a
> metadata.json file, which includes a "version" property. This is supposed to
> state the version of the extension itself, and for NetSpeed, that property
> is 28.
> 
> https://github.com/hedayaty/NetSpeed/blob/master/metadata.json
ok, i will take the version from the metadata file.

> I've tried asking the upstream developer, hedayaty, to git-tag his releases.
> Or at least to explain to us how to know when the code is stable enough for
> release. But he doesn't seem to get it, and I'm not sure how else to explain
> what I'm asking about :/
> 
> So, I guess continue with your %{gitdate} and %{shortcommit} Releases. But
> I'm pretty sure the spec Version should be 28.
yes, that's how I see it too.

> I'll double check everything else one last time when that's done.
ok


Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-netspeed.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-netspeed-3.28-0.2.20180208gite3cea60.fc27.src.rpm

%changelog
* Wed Feb 07 2018 Martin Gansser <martinkg@fedoraproject.org> - 3.28-0.2.20180208gite3cea60
- Update to recent git version 3.28-0.2.20180208gite3cea60

Comment 19 Andrew Toskin 2018-02-13 02:37:31 UTC
I'd meant the version should be *just* 28, because that's what the "version" property in the json file says, not 3.28.

But I won't make you wait for yet another review because that.

Sorry, this has taken so long, but there's only so much you can do when the upstream developer gets busy with other things in their life. At least it's done now, though :)

APPROVED!

The next steps include requesting the creation of the repository for you package in tje Fedora SCM, and requesting all branches for all versions of Fedora and/or EPEL you plan on building for. (master will be created automatically, but you'll need to manually request f27 and f26, and epel7 if you wish to support that release too.)

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers%2FJoin#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Comment 20 MartinKG 2018-02-13 07:50:45 UTC
Thanks for the review !
the version is now gnome-shell-extension-netspeed-28-0.2.20180208gite3cea60
hope that's ok now.

Comment 21 MartinKG 2018-02-16 11:57:06 UTC
creating a new repo fails at the moment ...
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org
/thread/IEEBDYWNBMFSPJYAQI5J3YTDLWBAH2RK/

Comment 22 Andrew Toskin 2018-02-27 00:57:59 UTC
Did you get it working? I don't see netspeed's repo request on Pagure.

Comment 23 Andrew Toskin 2018-02-27 01:09:37 UTC
Never mind, I found you.

https://pagure.io/releng/fedora-scm-requests/issue/4602

Your Red Hat Bugzilla and Fedora Account System accounts need to use the same email address.

Comment 24 MartinKG 2018-02-27 09:27:07 UTC
I have send a email to the bugzilla-owner@redhat.com with this content:

Does the Bugzilla Admins can merge the two accounts mgansser(at)online.de and mgansser(at)alice.de or disable/delete the mgansser(at)alice.de account.

I received the following message as an answer:
Hi Eric,

Ticket URL:  PNT0170047

Thank you for contacting the Red Hat PnT Devops team. Your ticket has been opened with the following Priority, Severity, Impact, and Urgency:

Priority: 4 - Normal
Severity: 4 - Normal
Impact: 4 - Low
Urgency: 4 - Low

This request will stay at the current priority and severity until one of our technicians reviews it. Your ticket will have no guarantee of an SLA until it's properly triaged. All email requests come to us without service specifications.

Triaging email requests takes time, and email is not an efficient way to get a hold of us. We have been hard at work creating forms to better filter and triage your requests on the PnT DevOps One Portal (https://pnthelp.redhat.com). Simply search for what you want or peer through our service catalog by clicking "Create a Ticket." This saves us all time, and will ultimately increase the speed at which your request is handled.

As your request is processed, you will receive email notifications with updates.

Regards,

Red Hat PnT DevOps Team
 
Ref:MSG13543581

Comment 25 MartinKG 2018-02-28 12:03:49 UTC
Hi Andrew,

I want to close the bugzilla ticket and open another ticket,
then the new ticket will also contain my new account information,
is this ok for you ?

Comment 26 Andrew Toskin 2018-03-01 01:12:22 UTC
I guess so. Post the link to the new ticket here, and I'll follow you to the new one.

Comment 27 MartinKG 2018-03-01 08:04:57 UTC
Hi Andrew,

I have created a new ticket:
https://bugzilla.redhat.com/show_bug.cgi?id=1550396