Bug 1745846 - Review Request: f31-backgrounds - Fedora 31 default desktop background
Summary: Review Request: f31-backgrounds - Fedora 31 default desktop background
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Miro Hrončok
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: AcceptedBlocker
Depends On:
Blocks: BetaBlocker, F31BetaBlocker 1744266
TreeView+ depends on / blocked
 
Reported: 2019-08-27 05:41 UTC by Luya Tshimbalanga
Modified: 2019-09-13 17:12 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-09-04 20:54:42 UTC
mhroncok: fedora-review+


Attachments (Terms of Use)

Description Luya Tshimbalanga 2019-08-27 05:41:43 UTC
Spec URL: https://github.com/fedoradesign/backgrounds/releases/download/v31.0.0/f31-backgrounds.spec
SRPM URL: https://github.com/fedoradesign/backgrounds/releases/download/v31.0.0/f31-backgrounds-31.0.0-1.fc30.src.rpm
Description: This package contains desktop backgrounds for the Fedora 31 default theme.  Pulls in themes for GNOME, KDE, Mate and Xfce desktops.
Fedora Account System Username:luya

Comment 1 Luya Tshimbalanga 2019-08-27 05:47:09 UTC
COPR built available on https://copr.fedorainfracloud.org/coprs/luya/f31-backgrounds/build/1021674/

Added dependent and blocker report.

Comment 2 Miro Hrončok 2019-08-27 08:51:39 UTC
1) The License tag of the base subpackage is repeated redundantly. 



2) Attribution-Extras says:

$ grep Licence Attribution-Extras | sort | uniq
Licence: CC-BY-SA 4.0
Licence: CC-BY 4.0
Licence: CC0 1.0
Licence: Free Art 1.0


But the license tag of extras-base is:

License:	CC-BY and CC-BY-SA


When in fact it should be:

License:	CC-BY and CC-BY-SA and CC0 and Free Art


The license file also contain only the CC licenses, not Free Art:

%license CC-BY-SA-4.0 CC-BY-4.0 CC0-1.0 Attribution-Extras

Comment 3 Miro Hrončok 2019-08-27 09:01:39 UTC
Oh, there is Free Art 1.3 text in the package as well.

Comment 4 Miro Hrončok 2019-08-27 09:27:28 UTC
Some of the images in extras are quite small:


$ file extras/*.png 
extras/a-world-lightyears-from-home.png: PNG image data, 256 x 191, 8-bit/color RGB, non-interlaced
extras/cabines-de-bains.png:             PNG image data, 256 x 160, 8-bit/color RGB, non-interlaced
extras/descent-to-loch-ericht.png:       PNG image data, 4864 x 2736, 8-bit/color RGB, non-interlaced
extras/green-leaf.png:                   PNG image data, 256 x 144, 8-bit/color RGBA, non-interlaced
extras/icelandic-stone-beach.png:        PNG image data, 3840 x 2160, 8-bit/color RGB, non-interlaced
extras/last-light-on-antler-peak.png:    PNG image data, 256 x 144, 8-bit/color RGB, non-interlaced
extras/life-is-blue.png:                 PNG image data, 4032 x 2268, 8-bit/color RGBA, non-interlaced
extras/our-world.png:                    PNG image data, 256 x 144, 8-bit/color RGB, non-interlaced

Comment 5 Lukas Ruzicka 2019-08-27 11:17:16 UTC
The wallpaper looks good to me. It is very suitable for screens with icons on them, because of the subtle colours. Thank you.

Comment 6 Luya Tshimbalanga 2019-08-27 15:17:35 UTC
(In reply to Miro Hrončok from comment #2)
> 1) The License tag of the base subpackage is repeated redundantly. 

Removed

> 
> 2) Attribution-Extras says:
> 
> $ grep Licence Attribution-Extras | sort | uniq
> Licence: CC-BY-SA 4.0
> Licence: CC-BY 4.0
> Licence: CC0 1.0
> Licence: Free Art 1.0
> 
> 
> But the license tag of extras-base is:
> 
> > 
> The license file also contain only the CC licenses, not Free Art:
> 
> %license CC-BY-SA-4.0 CC-BY-4.0 CC0-1.0 Attribution-Extras

Free Art is now included.

Here is the updated files with fixed size on some extra wallpapers


Spec URL: https://github.com/fedoradesign/backgrounds/releases/download/v31.0.0/f31-backgrounds.spec
SRPM URL: https://github.com/fedoradesign/backgrounds/releases/download/v31.0.0/f31-backgrounds-31.0.1-1.fc30.src.rpm

Comment 7 Miro Hrončok 2019-08-27 15:48:55 UTC
The specfile version hasn't changed, but the SRPM version did. Also, the SRPM link gives 404. Are the links correct?

Comment 8 Luya Tshimbalanga 2019-08-27 18:48:56 UTC
I forgot to change v31.0.0 to v31.0.1.
Typing from a cellphone

Comment 10 Miro Hrončok 2019-08-28 04:53:33 UTC
Package should install a .desktop file using desktop-file-install or run desktop-file-validate.

Package must own all directories that it creates.
Directories without known owners: 
     /usr/share/wallpapers
     /usr/share/gnome-background-properties
     /usr/share/backgrounds/f31


License mismatch: The package installs Free-Art-1.3, but the files are under Free-Art-1.0 (according to Attribution-Extras).


Diff spec file in url and in SRPM
---------------------------------
--- /home/churchyard/rpmbuild/FedoraReview/1745846-f31-backgrounds/srpm/f31-backgrounds.spec	2019-08-28 06:28:18.636493744 +0200
+++ /home/churchyard/rpmbuild/FedoraReview/1745846-f31-backgrounds/srpm-unpacked/f31-backgrounds.spec	2019-08-27 15:08:09.000000000 +0200
@@ -37,4 +37,5 @@
 %package	base
 Summary:	Base images for Fedora %{relnum} default background
+License:	CC-BY-SA
 
 %description	base

Comment 11 Benson Muite 2019-08-28 10:09:00 UTC
Wallpaper is nice. However, COPR repository seemed to have trouble updating using
sudo dnf copr enable luya/f31-backgrounds
sudo dnf install f31-backgrounds-kde
gives an error
Failed to download metadata for repo 'copr:copr.fedorainfracloud.org:luya:f31-backgrounds'

Was easier to rebuild rpm, have made a pull request to update readme with build and installation instructions:
https://github.com/fedoradesign/backgrounds/pull/14
This follows Magazine article:
https://fedoramagazine.org/how-rpm-packages-are-made-the-source-rpm/ 
not the more complete documentation at:
https://docs.fedoraproject.org/en-US/quick-docs/creating-rpm-packages/

Comment 12 Luya Tshimbalanga 2019-08-28 10:33:44 UTC
(In reply to Miro Hrončok from comment #10)
> Package should install a .desktop file using desktop-file-install or run
> desktop-file-validate.

The .desktop file is specific to KDE Plasma as provided by KDE SIG themselves.
>  
> Package must own all directories that it creates.
> Directories without known owners: 
>      /usr/share/wallpapers
>      /usr/share/gnome-background-properties
>      /usr/share/backgrounds/f31
> 

Weird. Something is not right on the fedora-review. The spec file clearly lists:

%dir %{_datadir}/backgrounds/%{bgname}
%dir %{_datadir}/gnome-background-properties/

which should set to the right owners. The ownership of "/usr/share/wallpapers" should be set to KDE using their macro %{_kde4_datadir}.
It seems like a bug.

> 
> License mismatch: The package installs Free-Art-1.3, but the files are under
> Free-Art-1.0 (according to Attribution-Extras).
> 

Fixed.
 
> Diff spec file in url and in SRPM
> ---------------------------------
> ---
> /home/churchyard/rpmbuild/FedoraReview/1745846-f31-backgrounds/srpm/f31-
> backgrounds.spec	2019-08-28 06:28:18.636493744 +0200
> +++
> /home/churchyard/rpmbuild/FedoraReview/1745846-f31-backgrounds/srpm-unpacked/
> f31-backgrounds.spec	2019-08-27 15:08:09.000000000 +0200
> @@ -37,4 +37,5 @@
>  %package	base
>  Summary:	Base images for Fedora %{relnum} default background
> +License:	CC-BY-SA
>  
>  %description	base

Fixed.

SPECS: https://github.com/fedoradesign/backgrounds/releases/download/v31.0.2/f31-backgrounds.spec
SRPMS: https://github.com/fedoradesign/backgrounds/releases/download/v31.0.2/f31-backgrounds-31.0.2-1.fc30.src.rpm

Comment 13 Miro Hrončok 2019-08-28 10:52:42 UTC
(In reply to Luya Tshimbalanga from comment #12)
> (In reply to Miro Hrončok from comment #10)
> > Package should install a .desktop file using desktop-file-install or run
> > desktop-file-validate.
> 
> The .desktop file is specific to KDE Plasma as provided by KDE SIG
> themselves.

Does that mean it doesn't need to be validated? Why not?

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_files

"It is not simply enough to just include the .desktop file in the package, one MUST run desktop-file-install (in %install) OR desktop-file-validate (in %check or %install) and have BuildRequires: desktop-file-utils, to help ensure .desktop file safety and spec-compliance."



> > Package must own all directories that it creates.
> > Directories without known owners: 
> >      /usr/share/wallpapers
> >      /usr/share/gnome-background-properties
> >      /usr/share/backgrounds/f31
> > 
> 
> Weird. Something is not right on the fedora-review. The spec file clearly
> lists:
> 
> %dir %{_datadir}/backgrounds/%{bgname}
> %dir %{_datadir}/gnome-background-properties/
> 
> which should set to the right owners. The ownership of
> "/usr/share/wallpapers" should be set to KDE using their macro
> %{_kde4_datadir}.
> It seems like a bug.

OK, let me recheck:

/usr/share/backgrounds/f31 is owned by f31-backgrounds-base
/usr/share/gnome-background-properties is owned by f31-backgrounds-gnome
/usr/share/wallpapers is owned by kde-filesystem

Mea cupla, everything is indeed alright there.


Downloading SRPM to check the remaining two.

Comment 14 Miro Hrončok 2019-08-28 11:04:19 UTC
(In reply to Luya Tshimbalanga from comment #12)
> > License mismatch: The package installs Free-Art-1.3, but the files are under
> > Free-Art-1.0 (according to Attribution-Extras).
> > 
> 
> Fixed.

Indeed. Not sure if relicensing the work like this is ok, but it was done upstream, so I don't really cere.



> > Diff spec file in url and in SRPM
...
> 
> Fixed.

Indeed.

Comment 15 Miro Hrončok 2019-08-28 11:06:22 UTC
Rpmlint
-------

I've removed dangling-relative-symlink (too many) and invalid-url (my mock has no internet access):

Checking: f31-backgrounds-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-base-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-animated-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-kde-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-gnome-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-mate-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-xfce-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-extras-base-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-extras-gnome-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-extras-mate-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-extras-kde-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-extras-xfce-31.0.2-1.fc32.noarch.rpm
          f31-backgrounds-31.0.2-1.fc32.src.rpm
f31-backgrounds.noarch: W: no-documentation
f31-backgrounds-base.noarch: W: no-documentation
f31-backgrounds-animated.noarch: W: no-documentation
f31-backgrounds-kde.noarch: W: no-documentation
f31-backgrounds-gnome.noarch: W: no-documentation
f31-backgrounds-mate.noarch: W: no-documentation
f31-backgrounds-xfce.noarch: W: no-documentation
f31-backgrounds-extras-base.noarch: W: no-documentation
f31-backgrounds-extras-gnome.noarch: W: no-documentation
f31-backgrounds-extras-mate.noarch: W: no-documentation
f31-backgrounds-extras-kde.noarch: W: no-documentation
f31-backgrounds-extras-xfce.noarch: W: no-documentation
13 packages and 0 specfiles checked; 0 errors, 593 warnings.

Ok.


Rpmlint (installed packages)
----------------------------

f31-backgrounds-extras-xfce.noarch: W: no-documentation
f31-backgrounds-gnome.noarch: W: no-documentation
f31-backgrounds-animated.noarch: W: no-documentation
f31-backgrounds-mate.noarch: W: no-documentation
f31-backgrounds.noarch: W: no-documentation
f31-backgrounds-extras-kde.noarch: W: no-documentation
f31-backgrounds-extras-base.noarch: W: no-documentation
f31-backgrounds-extras-mate.noarch: W: no-documentation
f31-backgrounds-extras-gnome.noarch: W: no-documentation
f31-backgrounds-kde.noarch: W: no-documentation
f31-backgrounds-base.noarch: W: no-documentation
f31-backgrounds-xfce.noarch: W: no-documentation
12 packages and 0 specfiles checked; 0 errors, 605 warnings.

Ok.

Comment 16 Miro Hrončok 2019-08-28 11:07:55 UTC
Not to get lost in the comments. Lint the desktop files in %check or install them via the install tool and I will approve the package.

Comment 17 Luya Tshimbalanga 2019-08-29 00:42:24 UTC
desktop-file-validate will complain about the missing "Type" which only provides application, link and directory.

> desktop-file-validate rpmbuild/SOURCES/f31-metadata.desktop 
> rpmbuild/SOURCES/f31-metadata.desktop: error: required key "Type" in group "Desktop Entry" is not present

KDE SIG provided that workaround removing Type parameter for displaying wallpapers on Plasma on Fedora 25. 
https://src.fedoraproject.org/rpms/f25-backgrounds/c/9beaa478698cb6a0657034b7eb1db4c835f78b0b?branch=master

You can discuss with them.

Comment 19 Miro Hrončok 2019-08-30 08:22:27 UTC
install -D -p -m644 %{SOURCE1} \
%{buildroot}%{_datadir}/plasma/desktoptheme/%{Bg_Name}/metadata.desktop

%check
desktop-file-validate %{buildroot}%{_datadir}/plasma/desktoptheme/%{Bg_Name}/metadata.desktop


This can be probably replaced with one call to desktop-file-install.

Anyway, package APPROVED. Thanks

Comment 20 Luya Tshimbalanga 2019-08-30 21:35:24 UTC
Thank you Miro!

Comment 21 Luya Tshimbalanga 2019-08-31 10:19:48 UTC
(In reply to Lukas Ruzicka from comment #5)
> The wallpaper looks good to me. It is very suitable for screens with icons
> on them, because of the subtle colours. Thank you.

Glad to hear that.

Comment 22 Gwyn Ciesla 2019-09-03 14:57:50 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/f31-backgrounds

Comment 23 Geoffrey Marr 2019-09-03 21:15:03 UTC
Discussed during the 2019-09-03 blocker review meeting: [1]

The decision to classify this bug as an "AcceptedBlocker" was made as it violates the following criteria:

"The default desktop background must be different from that of the last two stable releases"

[1] https://meetbot.fedoraproject.org/fedora-blocker-review/2019-09-03/f31-blocker-review.2019-09-03-16.01.txt

Comment 24 Fedora Update System 2019-09-04 03:44:01 UTC
FEDORA-2019-8b2cccfabf has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2019-8b2cccfabf

Comment 25 Fedora Update System 2019-09-04 10:51:37 UTC
desktop-backgrounds-31.0.0-1.fc31, f31-backgrounds-31.0.2-2.fc31 has been pushed to the Fedora 31 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-8b2cccfabf

Comment 26 Fedora Update System 2019-09-04 20:54:42 UTC
desktop-backgrounds-31.0.0-1.fc31, f31-backgrounds-31.0.2-2.fc31 has been pushed to the Fedora 31 stable repository. If problems still persist, please make note of it in this bug report.

Comment 27 Adam Williamson 2019-09-13 15:38:50 UTC
Miro: for the record, the rules about FDO desktop file spec apply only to .desktop files that are actually application launchers, by my reading. This file is not an application launcher and is not actually intended to comply with the FDO spec, and 'correcting' it to pass desktop-file-validate actually breaks it. So please don't require that for any future reviews of background packages :)

See https://bugs.kde.org/show_bug.cgi?id=411876

Comment 28 Miro Hrončok 2019-09-13 17:12:00 UTC
I've asked why it should not pass the check, I have not demanded the desktop file is changed ;)

Anyway, here, a followup: https://pagure.io/packaging-committee/issue/925


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