Bug 1460317 - Review Request: gnome-panel - GNOME Flashback panel
Review Request: gnome-panel - GNOME Flashback panel
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
26
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW 1460319 1460325
  Show dependency treegraph
 
Reported: 2017-06-09 13:00 EDT by Yaakov Selkowitz
Modified: 2017-08-16 13:58 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-08-16 13:58:21 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Yaakov Selkowitz 2017-06-09 13:00:01 EDT
Spec URL: https://yselkowitz.fedorapeople.org/gnome-panel.spec
SRPM URL: https://yselkowitz.fedorapeople.org/gnome-panel-3.24.1-1.fc27.src.rpm
Description: GNOME Flashback panel
Fedora Account System Username: yselkowitz

Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=19932574
(for F26; the F27 buildroots were messed up this morning.)
Comment 1 Fabio Valentini 2017-06-15 14:20:19 EDT
Taking this review.
Comment 2 Fabio Valentini 2017-06-15 14:52:25 EDT
There seems to be a LOT of outdated cruft in the .spec file.

 1) The URL tag is pointing at the GNOME homepage, not a GNOME Panel page.
 2) Don't use the Group: tag.
 3) Remove the commented VCS: tag.
 4) Use the %{name} macro in the Source0: tag consistently.
 5) Remove the commented Patch0.
 6) It seems GPLv2+ and LGPLv2+ for the main package and LGPLv2+ for the -libs subpackage are the correct licenses now (no more GFDL).
 7) Is the explicit Requires: gnome-desktop3 necessary (libgnome-desktop* is pulled in automatically).
 8) Remove the explicit Requires: libwnck3 (this is pulled in automatically).
 9) Remove the explicit Requires: e-d-s (this is pulled in automatically).
10) Move Requires(post): hicolor-icon-theme to Requires: hicolor-icon-theme.

11) Remove Requires(*): GConf2.
12) Remove BR: GConf2-devel.
13) Move BR: foo-devel to pkgconfig(foo) where possible.
14) Remove BR: libXt-devel (looks like it's no longer needed).
15) Remove the duplicate BR: dconf-devel.
16) Remove BR: libXau-devel (looks like it's no longer needed).
17) Remove BR: telepathy-glib-devel (looks like it's no longer needed).
18) Remove the duplicate BR: libtool.
19) Remove outdated Obsoletes (these were generated on fedora 15, so they're obviously no longer relevant).
20) Wrap description lines at 72 characters.

21) Confirm that the package descriptions are accurate.
22) Use %autosetup to replace the whole %setup line.
23) Remove the suspicious steps in %prep.
24) Either enable building gtk-doc's or remove the unnecessary BRs.
25) Use the %make_build macro instead of "make %{?_smp_mflags} V=1".
26) Why are you truncating NEWS? It would be easier to just not ship it as %doc.
27) Use the %make_install macro instead of "make DESTDIR=$RPM_BUILD_ROOT install"
28) Is the removal of the scrollkeeper directory still necessary?
29) Are there even *.a files installed anymore? Are *.la files present?
30) Possibly remove commented FIXME in %install.

31) Remove glib-compile-schemas scriptlets from %post, %postun and %posttrans (they are obsolete).
32) Please sort the entries in %files alphabetically (if possible, by the actual values of the macros, not by the macro names. so, %{_libdir} is sorted before %{_datadir}). This makes it much easier to read.
33) Use a glob for the different icon directories (%{_datadir}/icons/hicolor/*/apps/gnome-panel*).
34) Mark owned directories with a trailing backslash so it's clear it's an owned directory and not an owned file (e.g. %{_datadir}/gnome-panel/).
35) The installed gnome-panel.desktop entry must be validated in %check (or, worse, at the end of %install) - to temporarily ignore errors, append " || :" at the end of the line containing the "desktop-file-validate" command.
36) Just own "%{_libdir}/gnome-panel/", no need to specify all contents by hand.
37) Use a glob for the glib schema xml files (%{_datadir}/glib-2.0/schemas/org.gnome.gnome-panel.*.xml).
38) Do NOT use a glob for the .so* files in %{_libdir}.
39) Since you're not building gtk-docs, don't include COPYING-DOCS and don't own the /usr/share/gtk-doc directory (or build gtk-docs).
40) Mark COPYING and COPYING.LIB as %license, not %doc.

Either fix all those things (where appropriate), or just start fresh with a new .spec file without all the cruft.
Comment 3 Yaakov Selkowitz 2017-06-15 16:18:35 EDT
This started as a reinstatement of the previously existing package with incremental changes, so there may be cruft.  A lot of your points are stylistic, however.
Comment 4 Fabio Valentini 2017-06-16 05:51:55 EDT
That's why I wrote "where appropriate", because some of those points are only my opinion on how the "best possible" .spec file would look like, and not enforcible guidelines. However, I only pointed out things that make the current .spec file prone to errors and/or hard to read by a human, and most of them are (I think) reasonable suggestions or even necessary requirements for improvements.

I admit that I am a bit pedantic when it comes to clarity and readability, so if you object to specific points I made and provide reasoning for why you are doing it the way it's done right now, I won't block the review on those points.

Also, it seems I forgot to add two things to my list above:
41) Use %global instead of %define.
42) Fix present bogus dates in the %changelog.
Comment 5 Yaakov Selkowitz 2017-06-20 00:56:25 EDT
I cleaned up a lot of the cruft.  Files at the same location as comment 0.
Comment 6 Fabio Valentini 2017-06-20 10:57:06 EDT
There are only a few minor issues left:

Blocking positive review:

6) The license tag is still incorrect - according to licensecheck / fedora-review, no GFDL licensed files are present in the tarball.

10) Move "Requires(post): hicolor-icon-theme" to "Requires: hicolor-icon-theme". Otherwise, the created icon directories will be unowned.

23) Remove the autoreconf invocation in %prep. This step is superfluous when building from release tarballs.

35) The installed gnome-panel.desktop entry MUST be validated in %check:

%check
desktop-file-validate $RPM_BUILD_ROOT/%{_datadir}/applications/*.desktop || :

42) The "Requires: %{name}-libs = %{version}-%{release}" tag is missing the %{?_isa} macro in the -devel subpackage.


Non-blocking issues:

1) Consider putting "https://wiki.gnome.org/Projects/GnomePanel" as the URL (as that is the GNOME panel project's homepage).

20) Description lines are still longer than 72 characters (though this is not a blocker).

25) Use the modern %make_build macro instead of "make %{?_smp_mflags} V=1".

27) Use the modern %make_install macro instead of "make DESTDIR=$RPM_BUILD_ROOT install".

29) Use a less broad pattern to remove *.la files (other files with extensions ending in "a" are removed right now, too). "find $RPM_BUILD_ROOT -name '*.la' -delete -print"

30) Remove the commented FIXME in %install.

32) Sorted %files and BuildRequires would be nice.
Comment 7 Fabio Valentini 2017-07-19 04:58:36 EDT
Any progress on this? Otherwise I'll have to mark this package review as stalled.
Comment 8 Yaakov Selkowitz 2017-07-20 22:34:54 EDT
I'll try to get back to it this weekend.
Comment 9 Fabio Valentini 2017-08-16 13:58:21 EDT
Since no progress at all was made in two months - despite my ping after the first month - I'm marking this package review as dead.

Reopen if you can finish this review.

Note You need to log in before you can comment on or make changes to this bug.