Bug 1603146

Igor, do you still plan to do this review? It's of course totally fine if you don't have time, but then I'd like to unassign it again so that this doesn't get stuck. Thanks!

Ugh, I was pretty sure I did the review. I'm very sorry.

> Source: cockpit-ostree-%{version}.tar.gz

Add some comment how to obtain it.

> %build
> # There is nothing to do, release tarballs already have dist/.

Remove this entirely.

> make install DESTDIR=%{buildroot}

%make_install

> find %{buildroot} -type f >> files.list
> sed -i "s|%{buildroot}find %{buildroot} -type f >> files.list
sed -i "s|%{buildroot}||" *.list||" *.list

Isn't there some better way? Also what about directory ownership?

Thanks Igor for the suggestions! I applied them in the parent project first: https://github.com/cockpit-project/starter-kit/pull/38/files

Once that lands, I'll pull it into cockpit-ostree and do a new release.

> Also what about directory ownership?

I don't see anything special here. Somewhere between %make_install and rpmbuild there is some kind of fakeroot involved which makes sure the files are correctly owned by root (I know how that works in Debian, not sure about RPM - but supposedly it's quite similar).

"rpm -qvlp *.rpm" has all files as root:root as intended.

what I mean is that if you have /foo/bar/baz in the %files, the /foo and /foo/bar are not owned by anyone.

Ah, because of the find -type f. So that should be fixed with this PR as well now, as the directories are now included in the rpm as well:

Note that /usr/share/metainfo/ itself is not included, as that's already owned by the "filesystem" package; and /usr/share/cockpit/ is owned by cockpit:

❱❱❱ rpm -qlp cockpit-ostree-1-1.fc28.noarch.rpm
/usr/share/cockpit/cockpit-ostree
/usr/share/cockpit/cockpit-ostree/index.html
/usr/share/cockpit/cockpit-ostree/manifest.json
[... other files in cockpit-ostree/ ]

why don't you just write something like...

%files
%{_datadir}/cockpit/cockpit-ostree/

?

That's what I do now, more or less (https://github.com/martinpitt/starter-kit/blob/simplify-spec/cockpit-starter-kit.spec.in#L22):

%files
/usr/share/cockpit/*
/usr/share/metainfo/*

I'm happy to replace /usr/share/ with %{_datadir} though (will update the PR in a sec). I just don't want to hardcode the project name in starter-kit (as every project that descends from it will have to change it), but I can change it in ostree if that seems clearer.

I addressed Igor's review comments (thank you!), did a new upstream release 175 (to be newer than cockpit's current 174 release) and also updated copr. Updated URLs:

Upstream spec URL: https://github.com/cockpit-project/cockpit-ostree/blob/master/cockpit-ostree.spec.in

spec without macros (the "real" one):
https://copr-dist-git.fedorainfracloud.org/cgit/@cockpit/cockpit-preview/cockpit-ostree.git/tree/cockpit-ostree.spec?h=f28

SRPM URL: https://copr-be.cloud.fedoraproject.org/results/@cockpit/cockpit-preview/fedora-28-x86_64/00783500-cockpit-ostree/cockpit-ostree-175-1.fc28.src.rpm
(that directory also has build log and binary RPM)

COPR BUILD: https://copr.fedorainfracloud.org/coprs/g/cockpit/cockpit-preview/build/783500/

> /usr/share/cockpit/*

Should be really %{_datadir}/cockpit/*

Apart from that, looks good.

Argh, yes! I did that in starter-kit, and forgot to change it in ostree as well. Done in https://github.com/cockpit-project/cockpit-ostree/pull/7 so it will be in the next release.

Thanks for the review!

Created SCM request: https://pagure.io/releng/fedora-scm-requests/issue/7561

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

Imported into dist-git and built: https://koji.fedoraproject.org/koji/taskinfo?taskID=28794277