The first thing that sticks out to me is that source rpm is named gnome-kiosk-40.alpha-1.fc34.src.rpm while the spec file goes to great lengths to use ~alpha instead of .alpha. Looks like something has gone wrong with it if the srpm file is named like this?

In other gnome packages we've used a system where we put this at the top of each file:

%global tarball_version %%(echo %{version} | tr '~' '.')

and then used %{name}-%{tarball_version} in the Source line and in the %setup line, which then automatically translates 40~alpha to 40.alpha for those two places. Maybe it would be easiest to use the same system here? See e.g. https://src.fedoraproject.org/rpms/gnome-shell/blob/rawhide/f/gnome-shell.spec


Another thing that sticks out is that none of the C code files in the tarball have license headers. Would be nice to add them -- although I don't think it's a requirement.


Some in line comments about the spec file:

> #VCS:           git:git://gitlab.gnome.org/halfline/gnome-kiosk

I think you can just take the VCS line out, it was for Colin's old automation that nobody uses any more.

> %package -n gnome-kiosk-search-appliance
> Summary:        Example search application application that uses GNOME Kiosk
> License:        GPLv2+
> Requires:       %{name}%{?_isa} = %{version}-%{release}

You can't use %{?_isa} in a noarch package -- it's going to be different on different arches, but noarch means that it's supposed to work on all arches.

> Requires:       firefox
> BuildArch:      noarch



%files -n gnome-kiosk-search-appliance
%{_datadir}/applications/org.gnome.Kiosk.SearchApp.desktop
%{_datadir}/gnome-session/sessions/org.gnome.Kiosk.SearchApp.session
%{_datadir}/xsessions/org.gnome.Kiosk.SearchApp.Session.desktop
%{_datadir}/wayland-sessions/org.gnome.Kiosk.SearchApp.Session.desktop

Looking at the files list, it sounds like the package should have runtime requires on gnome-session to work?


Should the package have appdata to show up in gnome-software?

(In reply to Kalev Lember from comment #1)
> The first thing that sticks out to me is that source rpm is named
> gnome-kiosk-40.alpha-1.fc34.src.rpm while the spec file goes to great
> lengths to use ~alpha instead of .alpha. Looks like something has gone wrong
> with it if the srpm file is named like this?
heh.  Missing slashes on the my sed line. Dunno how I missed that.
 
> In other gnome packages we've used a system where we put this at the top of
> each file:
> 
> %global tarball_version %%(echo %{version} | tr '~' '.')
> 
> and then used %{name}-%{tarball_version} in the Source line and in the
> %setup line, which then automatically translates 40~alpha to 40.alpha for
> those two places. Maybe it would be easiest to use the same system here? See
> e.g.
> https://src.fedoraproject.org/rpms/gnome-shell/blob/rawhide/f/gnome-shell.
> spec
I kind of like keeping the upstream version at the top, and encoding from it,
since it seems more "canonical" to me.

> Another thing that sticks out is that none of the C code files in the
> tarball have license headers. Would be nice to add them -- although I don't
> think it's a requirement.
That was actually intentional.  Those top blurbs are redundant, get stale,
end up listing the wrong copyright holders, or just a subset of copy right holders,
might have the wrong address in them etc.

I figured since this is a new project I'd just do away with them. 
 
> Some in line comments about the spec file:
> 
> > #VCS:           git:git://gitlab.gnome.org/halfline/gnome-kiosk
> 
> I think you can just take the VCS line out, it was for Colin's old
> automation that nobody uses any more.
wfm.

> 
> > %package -n gnome-kiosk-search-appliance
> > Summary:        Example search application application that uses GNOME Kiosk
> > License:        GPLv2+
> > Requires:       %{name}%{?_isa} = %{version}-%{release}
> 
> You can't use %{?_isa} in a noarch package -- it's going to be different on
> different arches, but noarch means that it's supposed to work on all arches.
oops. I actually added the BuildArch: noarch at the last minute when I was rereading
the spec, and missed this. will fix.

> %files -n gnome-kiosk-search-appliance
> %{_datadir}/applications/org.gnome.Kiosk.SearchApp.desktop
> %{_datadir}/gnome-session/sessions/org.gnome.Kiosk.SearchApp.session
> %{_datadir}/xsessions/org.gnome.Kiosk.SearchApp.Session.desktop
> %{_datadir}/wayland-sessions/org.gnome.Kiosk.SearchApp.Session.desktop
> 
> Looking at the files list, it sounds like the package should have runtime
> requires on gnome-session to work?
indeed.

> Should the package have appdata to show up in gnome-software?
I don't think it makes sense to show up in gnome-software. Maybe it should be NoDisplay=true
even.

oh it's already NoDisplay=true, good.

Okay updated:


Spec URL: https://rstrode.fedorapeople.org/gnome-kiosk/gnome-kiosk.spec
SRPM URL: https://rstrode.fedorapeople.org/gnome-kiosk/gnome-kiosk-40~alpha-1.fc34.src.rpm

actually let me just drop the sed goo, there's no reason to be "different" than the other packages here, it's just going to create confusion at release time.

(done)

Looks good to me! ​Licensing looks good and the package follows the packaging standards.


Just two tiny nits that are not blockers for the review:

> %autosetup -S git -n %{name}-40.alpha

You can use %{tarball_version} instead of hardcoding 40.alpha here

> %package -n %{name}-search-appliance

A shorter form of this is '%package search-appliance' which avoids '-n %{name}-'


APPROVED

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

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/gnome-kiosk

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