Skip to content
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

tools/cockpit.spec: Adjust storaged/udisks2 dependency #7886

Merged
merged 1 commit into from
Oct 16, 2017

Conversation

martinpitt
Copy link
Member

@martinpitt martinpitt commented Oct 16, 2017

Adjust the target OS specific dependencies of cockpit-storaged:

  • RHEL 7.4 and current CentOS 7 ship storaged and do not support
    Recommends:.
  • RHEL = 7.5 will only ship udisks2, which does (rightfully) not
    provide storaged. Still no support for Recommends:.
  • RHEL 8 will only ship udisks2, but does support Recommends:
  • Fedora 27 also moved to udisks2, earlier versions have storaged. All
    supported Fedora releases support Recommends:.

Standard RPM macros do not allow us to differentiate between RHEL 7.4
and 7.5 (%rhel == 7), so check /etc/os-release for that. This works for
"real-life" builds, but not for building on our test images as these use
a CentOS 7 mock chroot for all RHEL builds. Thus do the os-release
parsing in fedora.install and pass on the value as define.

The udisks2 modules for lvm2 and iscsi are not yet available on RHEL
7.5, so skip these dependencies for now.

@martinpitt martinpitt added the release-blocker Targetted for next release label Oct 16, 2017
@martinpitt
Copy link
Member Author

priority as this is urgent for a downstream release.

@martinpitt
Copy link
Member Author

Triggered a fedora-26 test run as well, as that's a separate case.

@martinpitt
Copy link
Member Author

Force-pushed a fix for CentOS 7, that currently behaves like RHEL 7.4. Unfortunately even the os-release trick doesn't work there, as VERSION_ID==7 there.

The cleanest solution for this mess would be alternative dependencies, but AFAIK rpm doesn't support these? Or does it?

@@ -20,6 +20,12 @@
%define rhel 0
%endif

# for testing this already gets set in fedora.install, as we want the target
# VERSION_ID, not the mock chroot's one
Copy link
Contributor

@stefwalter stefwalter Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you seen this in any other spec file? What happens in the cockpit RPM is installed before rhel-release-server in the same transaction?

I'm worried about this. Does it make sense just to have RHEL 7.5 behavior here, and RHEL 7.4 behavior on the rhel-7-4 branch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "this"? Parsing os-release in the .spec is relatively common, e. g. systemd's spec file does that too. Our spec file also does it by default, but this doesn't work in our testsuite as that builds a RHEL package in a CentOS chroot.

I don't understand your question about install transactions -- this is purely a build-time thing, the built rpms have fixed dependencies, so OS install/upgrade orders don't matter here.

We want to be able to test current master on RHEL 7.4 (and 7.5), so we can't have the dependencies in the back-branches only. The conditional dependencies aren't new at all, they just get updated in this PR. The new thing is the more fine-grained differentiation between 7.4 and 7.5, but we have to do that somehow. Whether we do it directly in the spec or add some indirection in our build system that pre-processes the .spec.in and builds a target specific .spec doesn't matter that much, but IMHO the latter would be much more intrusive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your question about install transactions -- this is purely a build-time thing, the built rpms have fixed dependencies, so OS install/upgrade orders don't matter here.

Okay perfect. If build time only then I'm fine with this.

Adjust the target OS specific dependencies of cockpit-storaged:

 * RHEL 7.4 and current CentOS 7 ship storaged and do not support
   Recommends:.
 * RHEL = 7.5 will only ship udisks2, which does (rightfully) not
   provide storaged. Still no support for Recommends:.
 * RHEL 8 will only ship udisks2, but does support Recommends:
 * Fedora 27 also moved to udisks2, earlier versions have storaged. All
   supported Fedora releases support Recommends:.

Standard RPM macros do not allow us to differentiate between RHEL 7.4
and 7.5 (%rhel == 7), so check /etc/os-release for that. This works for
"real-life" builds, but not for building on our test images as these use
a CentOS 7 mock chroot for all RHEL builds. Thus do the os-release
parsing in fedora.install and pass on the value as define.

The udisks2 modules for lvm2 and iscsi are not yet available on RHEL
7.5, so skip these dependencies for now.

Closes cockpit-project#7886
@martinpitt
Copy link
Member Author

Note: RPM 4.13 introduces a way to specify alternative dependencies, but on RHEL/CentOS 7.x we only have 4.11. So this won't be a problem any more in the future.

Copy link
Contributor

@stefwalter stefwalter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stefwalter stefwalter merged commit fbaff63 into cockpit-project:master Oct 16, 2017
@martinpitt martinpitt deleted the storage-deps branch October 16, 2017 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Targetted for next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants