Bug 1603146 - Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree (split out from existing cockpit.srpm)
Summary: Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree (split...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Igor Raits
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-07-19 10:01 UTC by Martin Pitt
Modified: 2018-08-02 18:15 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-08-02 18:15:23 UTC
Type: ---
Embargoed:
igor.raits: fedora-review+


Attachments (Terms of Use)

Description Martin Pitt 2018-07-19 10:01:08 UTC
Spec URL: https://github.com/cockpit-project/cockpit-ostree/blob/master/cockpit-ostree.spec.in

The @VERSION@ gets replaced with the release number from the tag, i. e. "173". Otherwise the file is as-is. The replaced version is in the upstream release at 
https://github.com/cockpit-project/cockpit-ostree/releases, and in COPR:
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/00778957-cockpit-ostree/cockpit-ostree-173-1.fc28.src.rpm

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

Description: Cockpit component for managing software updates for ostree based systems

Fedora Account System Username: martinpitt (myself) or cockpit (the Cockpit CI bot that will actually upload the release, like for the cockpit package)



The "cockpit-ostree" rpm subpackage is currently built by the cockpit srpm. It is only useful on Atomic (Fedora and RHEL), so in downstream RHEL building it together with all the other cockpit packages is rather difficult: the same build needs to be shipped in multiple different products with different RPM filters. This gets even more hairy in the next RHEL version (please ask me for details in private, I can't elaborate on a public issue).

From an upstream POV it also makes sense to maintain cockpit-ostree separately, as different people may eventually maintain it, and it does not need a lot of releases as it changes rather seldomly.

For these reasons we recently moved the code into a separate cockpit-ostree upstream project: https://github.com/cockpit-project/cockpit-ostree/ . Functionality wise there would not be any changes, the code was imported without modifications (other than the build system and minor test adjustments).

I would like to do the same srpm split in Fedora (and RHEL downstream) as well.

Note that the first upstream release is "173" to be newer than the last cockpit release (172), so that upgrades work smoothly.

As soon as this lands, I will drop the cockpit-ostree RPM from cockpit.src.rpm.

Comment 1 Martin Pitt 2018-08-02 07:08:57 UTC
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!

Comment 2 Igor Raits 2018-08-02 10:19:51 UTC
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?

Comment 3 Martin Pitt 2018-08-02 12:48:59 UTC
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.

Comment 4 Igor Raits 2018-08-02 12:56:13 UTC
what I mean is that if you have /foo/bar/baz in the %files, the /foo and /foo/bar are not owned by anyone.

Comment 5 Martin Pitt 2018-08-02 13:13:40 UTC
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/ ]

Comment 6 Igor Raits 2018-08-02 13:15:52 UTC
why don't you just write something like...

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

?

Comment 7 Martin Pitt 2018-08-02 13:29:08 UTC
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.

Comment 8 Martin Pitt 2018-08-02 15:42:06 UTC
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/

Comment 9 Igor Raits 2018-08-02 15:43:14 UTC
> /usr/share/cockpit/*

Should be really %{_datadir}/cockpit/*

Apart from that, looks good.

Comment 10 Martin Pitt 2018-08-02 15:48:52 UTC
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!

Comment 11 Martin Pitt 2018-08-02 16:04:05 UTC
Created SCM request: https://pagure.io/releng/fedora-scm-requests/issue/7561

Comment 12 Igor Raits 2018-08-02 16:54:43 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/cockpit-ostree

Comment 13 Martin Pitt 2018-08-02 18:13:20 UTC
Imported into dist-git and built: https://koji.fedoraproject.org/koji/taskinfo?taskID=28794277


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