[Bug 1603146] New: Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

Bug ID: 1603146
           Summary: Review Request: cockpit-ostree - Cockpit user
                    interface for rpm-ostree
           Product: Fedora
           Version: rawhide
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]

Spec URL:
https://github.com/cockpit-project/cockpit-ostree/blob/master/cockpit-ostree...

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/coc...

SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/@cockpit/cockpit-preview/fed...

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

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.

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

Martin Pitt  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Review Request:             |Review Request:
                   |cockpit-ostree - Cockpit    |cockpit-ostree - Cockpit
                   |user interface for          |user interface for
                   |rpm-ostree                  |rpm-ostree (split out from
                   |                            |existing cockpit.srpm)

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

Igor Gnatenko  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |[e-mail]
           Assignee|[e-mail]    |[e-mail]

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #1 from Martin Pitt  ---
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!

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #2 from Igor Gnatenko  ---
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?

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #3 from Martin Pitt  ---
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.

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #4 from Igor Gnatenko  ---
what I mean is that if you have /foo/bar/baz in the %files, the /foo and
/foo/bar are not owned by anyone.

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #5 from Martin Pitt  ---
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/ ]

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #6 from Igor Gnatenko  ---
why don't you just write something like...

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

?

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

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

%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.

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #8 from Martin Pitt  ---
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 without macros (the "real" one):
https://copr-dist-git.fedorainfracloud.org/cgit/@cockpit/cockpit-preview/coc...

SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/@cockpit/cockpit-preview/fed...
(that directory also has build log and binary RPM)

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

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

Igor Gnatenko  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |POST
              Flags|                            |fedora-review+

--- Comment #9 from Igor Gnatenko  ---
> /usr/share/cockpit/*

Should be really %{_datadir}/cockpit/*

Apart from that, looks good.

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #10 from Martin Pitt  ---
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!

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #11 from Martin Pitt  ---
Created SCM request: https://pagure.io/releng/fedora-scm-requests/issue/7561

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

--- Comment #12 from Igor Gnatenko  ---
(fedscm-admin):  The Pagure repository was created at
https://src.fedoraproject.org/rpms/cockpit-ostree

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

Martin Pitt  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|POST                        |RELEASE_PENDING
           Assignee|[e-mail]  |[e-mail]

--- Comment #13 from Martin Pitt  ---
Imported into dist-git and built:
https://koji.fedoraproject.org/koji/taskinfo?taskID=28794277

[Bug 1603146] Review Request: cockpit-ostree - Cockpit user interface for rpm-ostree ( split out from existing cockpit.srpm)

https://bugzilla.redhat.com/show_bug.cgi?id=1603146

Igor Gnatenko  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RELEASE_PENDING             |CLOSED
         Resolution|---                         |RAWHIDE
           Assignee|[e-mail]            |[e-mail]
        Last Closed|                            |2018-08-02 14:15:23