[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

ceph.spec.in: use %_udevrulesdir to eliminate conditionals #5079

Merged
merged 1 commit into from
Jul 9, 2015
Merged

ceph.spec.in: use %_udevrulesdir to eliminate conditionals #5079

merged 1 commit into from
Jul 9, 2015

Conversation

smithfarm
Copy link
Contributor

The conditionals governing where 50-rbd.rules is installed were
not doing the right thing on SUSE distros.

http://tracker.ceph.com/issues/12166 Fixes: #12166

Signed-off-by: Nathan Cutler ncutler@suse.com

@ktdreyer
Copy link
Member

Looks good!

We could simplify this a tiny bit by defining %{_udevrulesdir} on the platforms that don't have it (like we're doing for %python_sitelib / %python_sitearch).

Something like this:

%if (0%{?rhel} && 0%{?rhel} < 7) || (0%{?suse_version} && 0%{?suse_version} < 1310)
%global _udevrulesdir /lib/udev/rules.d/
%endif

... and then the macro's available for use elsewhere.

@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/6110/

@smithfarm smithfarm force-pushed the wip-12166 branch 2 times, most recently from edb38cc to 180ae88 Compare June 26, 2015 09:46
@smithfarm
Copy link
Contributor Author

revamped to use %_udevrulesdir -- good idea, much cleaner

@loic-bot
Copy link

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/6124/

@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/6125/

ktdreyer
ktdreyer reviewed Jun 26, 2015
View reviewed changes
ceph.spec.in
%if 0%{?rhel} >= 7 || 0%{?fedora}
install -m 0644 -D udev/50-rbd.rules $RPM_BUILD_ROOT/usr/lib/udev/rules.d/50-rbd.rules
install -m 0644 -D udev/60-ceph-partuuid-workaround.rules $RPM_BUILD_ROOT/usr/lib/udev/rules.d/60-ceph-partuuid-workaround.rules
Copy link
Member

Choose a reason for hiding this comment

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

We can move both 50-rbd.rules and 60-ceph-partuuid-workaround.rules up, now, and eliminate this particular conditional entirely :)

Copy link
Member

Choose a reason for hiding this comment

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

install -m 0644 -D udev/60-ceph-partuuid-workaround.rules $RPM_BUILD_ROOT%{_udevrulesdir}/60-ceph-partuuid-workaround.rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, but I was trying to keep the commit small and atomic

@smithfarm smithfarm force-pushed the wip-12166 branch 2 times, most recently from d82c79a to 8ecbe61 Compare June 26, 2015 18:44
@smithfarm smithfarm changed the title ceph.spec.in: fix 50-rbd.rules conditionals ceph.spec.in: use %_udevrulesdir to eliminate conditionals Jun 26, 2015
@smithfarm
Copy link
Contributor Author

@ktdreyer rebased and expanded.

@ktdreyer
Copy link
Member

cool, looks good to me. Let's be sure this passes on gitbuilder, then merge

Reviewed-by: Ken Dreyer <kdreyer@redhat.com>

@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/6144/

@smithfarm
Copy link
Contributor Author

@ktdreyer did it pass on gitbuilder?

@ghost
Copy link
ghost commented Jul 9, 2015

@smithfarm pushed under the branch SUSE-wip-12166 in gitbuilder

@b-ranto
Copy link
Contributor
b-ranto commented Jul 9, 2015

I'm just wondering whether the PR is not redefining _udevrulesdir somewhere where it is already defined or if there is a platform left where it is not defined at all? Maybe, something like

%{!?_udevrulesdir: %global _udevrulesdir /lib/udev/rules.d}

would be a safer solution?

@smithfarm
Copy link
Contributor Author

@BRANTO1: that seems to be saying that the udev rules directory is /lib/udev/rules.d in all cases when _udevrulesdir is not defined.

@b-ranto
Copy link
Contributor
b-ranto commented Jul 9, 2015

@smithfarm: Yes, it should cover all the cases that are covered by the patch with rhel and suse conditionals though.

@smithfarm
Copy link
Contributor Author

Maybe this would address @BRANTO1 's concern?

%{!?_udevrulesdir: %global _udevrulesdir /usr/lib/udev/rules.d}
%if (0%{?rhel} && 0%{?rhel} < 7) || (0%{?suse_version} && 0%{?suse_version} < 1310)
%{!?_udevrulesdir: %global _udevrulesdir /lib/udev/rules.d}
%endif

@b-ranto
Copy link
Contributor
b-ranto commented Jul 9, 2015

@smithfarm: That looks like it should work, I just wanted to avoid complicated conditions with that one-liner. :-)

My rationale was simply that the _udevrulesdir macro was introduced when the udev files were moved to the new location so if it is not defined then they are probably in the old location (i.e. /lib/udev/rules.d). There might have been some fedoras where this did not hold but those are no longer supported. Not sure about suse here though.

@ktdreyer
Copy link
Member
ktdreyer commented Jul 9, 2015 
%{!?_udevrulesdir: %global _udevrulesdir /usr/lib/udev/rules.d}
%if (0%{?rhel} && 0%{?rhel} < 7) || (0%{?suse_version} && 0%{?suse_version} < 1310)
%{!?_udevrulesdir: %global _udevrulesdir /lib/udev/rules.d}
%endif

By adding the !?_udevrulesdir check to both %global lines, we're effectively ensuring that this will always set _udevrulesdir to /usr/lib/udev/rules.d, even on RHEL < 7 or SUSE < 1310.

What if we moved it to the bottom, ie

%if (0%{?rhel} && 0%{?rhel} < 7) || (0%{?suse_version} && 0%{?suse_version} < 1310)
%{!?_udevrulesdir: %global _udevrulesdir /lib/udev/rules.d}
%endif
%{!?_udevrulesdir: %global _udevrulesdir /usr/lib/udev/rules.d}

@ktdreyer
Copy link
Member
ktdreyer commented Jul 9, 2015

Or, we could just assume that modern platforms will define _udevrulesdir, and just go with the original one-line proposal:

%{!?_udevrulesdir: %global _udevrulesdir /lib/udev/rules.d}

I don't know of any platforms where that would cause issues.

@smithfarm
ceph.spec.in: use _udevrulesdir to eliminate conditionals
8aa758e 
The conditionals governing where 50-rbd.rules is installed were not doing the
right thing on SUSE distros.

Start using the %_udevrulesdir RPM macro, while taking care that it is defined
and set to the right value. Use it to eliminate some conditionals around other
udev rules files as well.

http://tracker.ceph.com/issues/12166 Fixes: ceph#12166

Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
Copy link
Contributor Author

OK, replaced three-line conditional with @BRANTO1 's one-liner.

@b-ranto
Copy link
Contributor
b-ranto commented Jul 9, 2015

Looks good to me, if the patch passes the gitbuilders, you can consider this

Reviewed-by: Boris Ranto <branto@redhat.com>

ktdreyer added a commit that referenced this pull request Jul 9, 2015
@ktdreyer
Merge pull request #5079 from SUSE/wip-12166
f25f2eb 
ceph.spec.in: use %_udevrulesdir to eliminate conditionals

Reviewed-by: Ken Dreyer <kdreyer@redhat.com>
Reviewed-by: Boris Ranto <branto@redhat.com>
@ktdreyer
Copy link
Member
ktdreyer commented Jul 9, 2015

Awesome!

I've pushed this to https://github.com/ceph/ceph/tree/wip-12166 in order to run it on the gitbuilders.

@smithfarm
Copy link
Contributor Author

I wonder if the same applies to %_sbindir - i.e. it was introduced simultaneously with the switch from /sbin/ to /usr/sbin/ ?

@smithfarm
Copy link
Contributor Author

oh, and by the way, wip-12166 looks good overall in the gitbuilders. The only failure seems to be on a Debian system?

@ktdreyer
Copy link
Member
ktdreyer commented Jul 9, 2015

The only failure seems to be on a Debian system?

gitbuilder-squeeze-amd64? yeah, looks like that one's just broken :(

@ktdreyer ktdreyer merged commit 8aa758e into ceph:master Jul 9, 2015
ktdreyer added a commit that referenced this pull request Jul 9, 2015
@ktdreyer
Merge pull request #5079 from SUSE/wip-12166
bc0992f 
ceph.spec.in: use %_udevrulesdir to eliminate conditionals
@smithfarm smithfarm deleted the wip-12166 branch July 9, 2015 20:02
@b-ranto
Copy link
Contributor
b-ranto commented Jul 9, 2015

@smithfarm: %_sbindir is a different thing. The location of %_sbindir has been configurable for a long time (you can usually supply --sbindir= to ./configure script). I suppose it was originally introduced to cover exactly this.

The switch from /sbin to /usr/sbin is just a relatively new thing and is an attempt to unify the location of (system) binaries (i.e. to no longer have /sbin and /usr/sbin but only one of them).

The original idea behind /bin and /sbin was to have an ability to do a basic boot of the system before mounting /usr (a pretty common scenario in the olden days) -- i.e. /bin and /sbin contained things like shell, mount, init and similar essential binaries that allowed the system to boot before /usr was mounted. AFAIK, the locations of these few binaries that went to /bin and /sbin were hard-coded -- it was up to developer/packager to decide whether a given binary is essential for boot.

@loic-bot
Copy link
loic-bot commented Jul 9, 2015

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/6321/

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

@ktdreyer ktdreyer

Labels
bug-fix build/ops
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@smithfarm @ktdreyer @loic-bot @b-ranto