New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine versioned dependencies on -bridge/-shell RPM binary packages #6001

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@martinpitt
Member

martinpitt commented Mar 1, 2017

This implements the idea from issue #5997. This completely eliminates the remaining duplication of versions between manifests and the spec file (some of which wer even inconsistent), and now uses dependencies which exactly reflect the manifests, not a global maximum over all of those.

It also robustifies the package build to actually fail (instead of silently creating bogus ">= 0" dependencies) if we ever change the layout of the manifests again (commit 15dc531 silently broke this).

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Mar 1, 2017

Member

Illustration:

$ cp tools/cockpit.spec /tmp; tools/gen-spec-dependencies /tmp/cockpit.spec; diff -u tools/cockpit.spec /tmp/cockpit.spec

Particularly this uses 124.x for kubernetes and ostree, and 122 for everything else:

--- tools/cockpit.spec	2017-03-01 13:16:45.619133046 +0100
+++ /tmp/cockpit.spec	2017-03-01 13:05:01.374093261 +0100
@@ -304,8 +304,8 @@
 
 %package machines
 Summary: Cockpit user interface for virtual machines
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-system >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-system >= 122
 Requires: libvirt
 Requires: libvirt-client
 
@@ -317,8 +317,8 @@
 %package ostree
 Summary: Cockpit user interface for rpm-ostree
 # Requires: Uses new translations functionality
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-system >= %{required_base}
+Requires: %{name}-bridge >= 124.x
+Requires: %{name}-system >= 124.x
 %if 0%{?fedora} > 0 && 0%{?fedora} < 24
 Requires: rpm-ostree >= 2015.10-1
 %else
@@ -374,7 +374,7 @@
 %if 0%{?rhel}
 Requires: %{name}-bridge >= %{version}-%{release}
 %endif
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-shell >= 122
 Requires: storaged >= 2.1.1
 %if 0%{?fedora} >= 24 || 0%{?rhel} >= 8
 Recommends: storaged-lvm2 >= 2.1.1
@@ -500,8 +500,8 @@
 
 %package kdump
 Summary: Cockpit user interface for kernel crash dumping
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: kexec-tools
 BuildArch: noarch
 
@@ -512,8 +512,8 @@
 
 %package sosreport
 Summary: Cockpit user interface for diagnostic reports
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: sos
 BuildArch: noarch
 
@@ -525,8 +525,8 @@
 
 %package subscriptions
 Summary: Cockpit subscription user interface package
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: subscription-manager >= 1.13
 BuildArch: noarch
 
@@ -538,8 +538,8 @@
 
 %package networkmanager
 Summary: Cockpit user interface for networking, using NetworkManager
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: NetworkManager
 # Optional components (only when soft deps are supported)
 %if 0%{?fedora} >= 24 || 0%{?rhel} >= 8
@@ -558,8 +558,8 @@
 
 %package selinux
 Summary: Cockpit SELinux package
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: setroubleshoot-server >= 3.3.3
 BuildArch: noarch
 
@@ -575,8 +575,8 @@
 
 %package docker
 Summary: Cockpit user interface for Docker containers
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: docker >= 1.3.0
 Requires: python
 
@@ -594,8 +594,8 @@
 Summary: Cockpit user interface for Kubernetes cluster
 Requires: /usr/bin/kubectl
 # Requires: Needs newer localization support
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 124.x
+Requires: %{name}-shell >= 124.x
 BuildRequires: golang-bin
 BuildRequires: golang-src
Member

martinpitt commented Mar 1, 2017

Illustration:

$ cp tools/cockpit.spec /tmp; tools/gen-spec-dependencies /tmp/cockpit.spec; diff -u tools/cockpit.spec /tmp/cockpit.spec

Particularly this uses 124.x for kubernetes and ostree, and 122 for everything else:

--- tools/cockpit.spec	2017-03-01 13:16:45.619133046 +0100
+++ /tmp/cockpit.spec	2017-03-01 13:05:01.374093261 +0100
@@ -304,8 +304,8 @@
 
 %package machines
 Summary: Cockpit user interface for virtual machines
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-system >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-system >= 122
 Requires: libvirt
 Requires: libvirt-client
 
@@ -317,8 +317,8 @@
 %package ostree
 Summary: Cockpit user interface for rpm-ostree
 # Requires: Uses new translations functionality
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-system >= %{required_base}
+Requires: %{name}-bridge >= 124.x
+Requires: %{name}-system >= 124.x
 %if 0%{?fedora} > 0 && 0%{?fedora} < 24
 Requires: rpm-ostree >= 2015.10-1
 %else
@@ -374,7 +374,7 @@
 %if 0%{?rhel}
 Requires: %{name}-bridge >= %{version}-%{release}
 %endif
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-shell >= 122
 Requires: storaged >= 2.1.1
 %if 0%{?fedora} >= 24 || 0%{?rhel} >= 8
 Recommends: storaged-lvm2 >= 2.1.1
@@ -500,8 +500,8 @@
 
 %package kdump
 Summary: Cockpit user interface for kernel crash dumping
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: kexec-tools
 BuildArch: noarch
 
@@ -512,8 +512,8 @@
 
 %package sosreport
 Summary: Cockpit user interface for diagnostic reports
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: sos
 BuildArch: noarch
 
@@ -525,8 +525,8 @@
 
 %package subscriptions
 Summary: Cockpit subscription user interface package
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: subscription-manager >= 1.13
 BuildArch: noarch
 
@@ -538,8 +538,8 @@
 
 %package networkmanager
 Summary: Cockpit user interface for networking, using NetworkManager
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: NetworkManager
 # Optional components (only when soft deps are supported)
 %if 0%{?fedora} >= 24 || 0%{?rhel} >= 8
@@ -558,8 +558,8 @@
 
 %package selinux
 Summary: Cockpit SELinux package
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: setroubleshoot-server >= 3.3.3
 BuildArch: noarch
 
@@ -575,8 +575,8 @@
 
 %package docker
 Summary: Cockpit user interface for Docker containers
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 122
+Requires: %{name}-shell >= 122
 Requires: docker >= 1.3.0
 Requires: python
 
@@ -594,8 +594,8 @@
 Summary: Cockpit user interface for Kubernetes cluster
 Requires: /usr/bin/kubectl
 # Requires: Needs newer localization support
-Requires: %{name}-bridge >= %{required_base}
-Requires: %{name}-shell >= %{required_base}
+Requires: %{name}-bridge >= 124.x
+Requires: %{name}-shell >= 124.x
 BuildRequires: golang-bin
 BuildRequires: golang-src

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Mar 1, 2017

tools: Remove hardcoded dependencies in cockpit.spec
Now that we compute the individual binary package dependencies from the
manifests, stop duplicating/hardcoding them in the spec file and replace
them with %{required_base}.

Don't use %{required_base} for -tests though, as this does not
correspond to any actual pkg/* module and thus gen-spec-dependencies
fails on that. For the installed tests to actually succeed we expect the
matching (current) base packages.

Closes #6001
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Mar 1, 2017

Member

Note: I didn't do the equivalent Debian change yet, as that is much less urgent. In fact, in the Debian world the smallest practical change is a source package, so dependencies between different binaries of the same source are relatively irrelevant. I'm still happy to do it, of course, but this can be a separate PR>

Member

martinpitt commented Mar 1, 2017

Note: I didn't do the equivalent Debian change yet, as that is much less urgent. In fact, in the Debian world the smallest practical change is a source package, so dependencies between different binaries of the same source are relatively irrelevant. I'm still happy to do it, of course, but this can be a separate PR>

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Mar 1, 2017

tools: Remove hardcoded dependencies in cockpit.spec
Now that we compute the individual binary package dependencies from the
manifests, stop duplicating/hardcoding them in the spec file and replace
them with %{required_base}.

Don't use %{required_base} for -tests though, as this does not
correspond to any actual pkg/* module and thus gen-spec-dependencies
fails on that. For the installed tests to actually succeed we expect the
matching (current) base packages.

Closes #6001

@dperpeet dperpeet added the priority label Mar 1, 2017

martinpitt added some commits Feb 28, 2017

tools: Adjust min-base-version to manifest.json → .in rename
Commit 15dc531 renamed the manifest.json files to *.json.in, so follow
suit in min-base-version to actually compute correct package
dependencies. Prevent this from happening again by erroring out if we
could not determine the version.

Also fix KeyError crash if a manifest file does not have a "requires"
object.
tools: Teach min-base-version to compute dependency for a subset of p…
…ackages

We will use that to compute dependencies for binary packages
individually, instead of using a global maximum for all of them.
tools: Remove hardcoded dependencies in cockpit.spec
Now that we compute the individual binary package dependencies from the
manifests, stop duplicating/hardcoding them in the spec file and replace
them with %{required_base}.

Don't use %{required_base} for -tests though, as this does not
correspond to any actual pkg/* module and thus gen-spec-dependencies
fails on that. For the installed tests to actually succeed we expect the
matching (current) base packages.

Closes #6001
tools: Generate correct rpm binary package dependencies
We don't want every binary RPM package to depend on the global maximum
of requires["cockpit"] of all our pkg/*, as this is too strict for e. g.
RHEL. Instead, for a plugin package cockpit-foo we only want the
required version from pkg/foo/manifest.json.in.

Add tools/gen-spec-dependencies which iterates over a spec file and
replaces %{required_base} with the correct dependency (via
min-base-version <pkg>), and call that during "make dist" so that the
distributed spec file is correct.

This introduces the assumption that we use %{required_base} *only* for
binary packages which actually correspond to a pkg/* module. For our
base packages (-ws, -system) we still need to specify the dependencies
manually, which is fine as we can't represent them in our pkg/*
manifests anyway.

Fixes #5997
@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Mar 1, 2017

Member

I agree with the approach of using the package manifests as the source of truth for the dependencies. We may have to adapt our other release related scripts to use the right spec file though.

Member

dperpeet commented Mar 1, 2017

I agree with the approach of using the package manifests as the source of truth for the dependencies. We may have to adapt our other release related scripts to use the right spec file though.

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Mar 1, 2017

Member

make dist fails for me:

...

Edit: my checkout was missing a commit, testing again

Member

dperpeet commented Mar 1, 2017

make dist fails for me:

...

Edit: my checkout was missing a commit, testing again

@dperpeet dperpeet added needswork and removed needswork labels Mar 1, 2017

@dperpeet dperpeet closed this in 92d5ae1 Mar 1, 2017

@martinpitt martinpitt deleted the martinpitt:package-deps branch Mar 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment