Don't hard code the dracut module directory #2312

FransUrbo · 2014-05-07T17:32:43Z

Instead, check for it (or accept the value provided by --with-dracutdir).

This check also don't seem to install dracut if the module directory don't exist (it sets 'dracutdir' in the makefile to nothing).

Closes #546, #1680, #2310, #2356.
A little less complicated than commit described in #546.

prakashsurya · 2014-05-07T18:19:07Z

I haven't looked at the changes, but I'm seeing RPM build failures with these running ./autogen.sh && ./configure --enable-debug && make pkg:

RPM build errors:
    File not found by glob: /tmp/zfs-build--HpajJEmY/BUILDROOT/zfs-0.6.2-275_g8b4f96c.fc18.x86_64/usr/lib/dracut/modules.d/*
make[1]: *** [rpm-common] Error 1
make[1]: Leaving directory `/root/zfs/fedora-18-x86_64-builder/build'
make: *** [rpm-utils] Error 2

FransUrbo · 2014-05-07T19:43:02Z

Works for me. Have now tested it on two systems, my primary Debian GNU/Linux Wheezy build machine (which makes rpms and then convert them to debs) and on a Fedora 20.

prakashsurya · 2014-05-07T22:08:59Z

Now that I look closer, it's only the first commit that fails. With both applied it works.

FransUrbo · 2014-05-07T22:33:53Z

Ah, perfect! Thanx for testing.

behlendorf · 2014-05-19T23:28:54Z

@FransUrbo Extending --with-dracutdir to optionally check for the right dracut directory is a nice addition. But I think we could make that AS_IF([test "x$dracutdir" = xcheck] a little more robust. I believe there are just two common paths to check.

${_prefix}/lib/dracut
${_prefix}/share/dracut

Regarding c476d06 it's generally not recommended to use configure results like this in spec files. The problem is that you need to be able to rebuild a stand alone srpm regardless of the environment. This patch wouldn't allow you to get different default paths for say RHEL6 and RHEL7.

It's for this reason that spec files will explicitly defines these varaibles based on the platform. Doing something like this would be the most portable, although I agree it's uglier.

diff --git a/rpm/generic/zfs.spec.in b/rpm/generic/zfs.spec.in
index 5c2196f..4955bdb 100644
--- a/rpm/generic/zfs.spec.in
+++ b/rpm/generic/zfs.spec.in
@@ -1,6 +1,7 @@
 %global _sbindir    /sbin
 %global _libdir     /%{_lib}
-%if 0%{?fedora} >= 17
+
+%if 0%{?fedora} >= 17 || 0%{?redhat} >= 7 || 0%{?centos} >= 7
 %global _udevdir    %{_prefix}/lib/udev
 %global _dracutdir  %{_prefix}/lib/dracut
 %else

And just for reference: http://www.gnu.org/software/autoconf/manual/autoconf.html#External-Software

FransUrbo · 2014-05-20T05:20:26Z

This still makes it hardcoded for everything else than Fedora17, Redhat7 and CentOS7... So if there's a RPM based distribution out there that don't put it in /usr/share/dracut, we're still doing it wrong...

I was thinking of a compromise:

diff --git a/rpm/generic/zfs.spec.in b/rpm/generic/zfs.spec.in
index 5c2196f..8e82c8f 100644
--- a/rpm/generic/zfs.spec.in
+++ b/rpm/generic/zfs.spec.in
@@ -1,12 +1,15 @@
 %global _sbindir    /sbin
 %global _libdir     /%{_lib}
-%if 0%{?fedora} >= 17
+%global _dracutdir  @dracutdir@
+%if "%{?_dracutdir:%{_dracutdir}}%{!?_dracutdir:0}" != "\@dracutdir\@"
+%if 0%{?fedora} >= 17 || 0%{?redhat} >= 7 || 0%{?centos} >= 7
 %global _udevdir    %{_prefix}/lib/udev
 %global _dracutdir  %{_prefix}/lib/dracut
 %else
 %global _udevdir    /lib/udev
 %global _dracutdir  %{_prefix}/share/dracut
 %endif
+%endif

 %bcond_with    debug
 %bcond_with    blkid

This way, you can still keep the specific distribution %if updated at every release, but a fallback for 'everyone else' should be the value from --with-dracutdir AND still allow for stand alone builds.

However, I haven't really gotten the new %if to work...

FransUrbo · 2014-05-20T10:13:28Z

For the spec file, this might (!) work. I'm still a little suspicious of the first %if though, but so far it seems to be working.

%global _dracutdir  /usr/lib/dracut
%if !%{defined _dracutdir} || "%{?_dracutdir}" == "@dracutdir\@"
%if 0%{?fedora} >= 17 || 0%{?redhat} >= 7 || 0%{?centos} >= 7
%global _dracutdir  %{_prefix}/lib/dracut
%else
%global _dracutdir  %{_prefix}/share/dracut
%endif
%endif

%if 0%{?fedora} >= 17 || 0%{?redhat} >= 7 || 0%{?centos} >= 7
%global _udevdir    %{_prefix}/lib/udev
%else
%global _udevdir    /lib/udev
%endif

FransUrbo · 2014-05-20T11:00:05Z

The 0caa7df commit should hopefully address your concerns about being able to be used stand alone, but still allow for the configure option.

If you feel this is a good start, I can rebase them and we can go from there.

behlendorf · 2014-05-20T21:31:41Z

@FransUrbo What about just inverting the logic. From what I can tell all the distributions are moving to %{_prefix}/lib/dracut and it's only the older ones which are still using %{_prefix}/share/dracut. So something like the following would special case the legacy value for older systems and we'd default to the new (presumably more likely to be correct) for everything else.

%if 0%{?fedora} <= 16 || 0%{?redhat} <= 6 || 0%{?centos} <= 6
%global _dracutdir  %{_prefix}/share/dracut
%else
%global _dracutdir  %{_prefix}/lib/dracut
%endif

If you really want to be able to pass the configure result such that it's picked up by the make rpm target I'd suggested adding it to RPM_DEFINE_UTIL in the ZFS_AC_RPM macro. Basically it allows you to pass any configure result as a --define when building the source RPM. This is currently how options such as --enable-debug are passed in without having to add them to the spec.in.

FransUrbo · 2014-05-21T09:36:54Z

Ok, so how about the new rebase then?

behlendorf · 2014-05-21T15:15:22Z

Absolutely. A rebase would be good. I think the change here is small enough you could squash it in to a single commit.

FransUrbo · 2014-05-21T15:22:29Z

@behlendorf Ok, a one commit-rebase done.

behlendorf · 2014-05-21T20:54:42Z

@FransUrbo Nice, this is much better. Let me actually give it a spin and make sure its working properly but then we can get it in. It occurs to me we could go one small step further and handle _udevdir the same way. You're almost there already.

FransUrbo · 2014-05-22T07:44:19Z

Ok, did a force push (added a 'Checking/Looking for dracut directory' message to the dracut part and also did the same check for the udev directories.

I'm not sure about using strings to get the directories, but it IS the most reliable. Maybe I should make sure strings is available first?

FransUrbo · 2014-05-22T07:45:13Z

Btw, maybe do the same for the systemd stuff?

Hardcoding paths is one of my pet-peves! They almost always break sooner than later - no system is exactly alike...

behlendorf · 2014-05-22T16:53:45Z

If we rely on strings and which in the auto-detect case then we'll need to add checks to ensure they are installed. If possible it would be nice to avoid adding more dependencies even if they are for common tools.

As for systemd that's already mostly handled. There's a bcond_with defined to enable/disable support and the _presetdir macro will be defined to be the correct path for the given build platform. I think the only bit missing is the ability to enable/disable this support through the RPM_DEFINE_COMMON macro. Although to support that we'd need to write a reliable autoconf check to determine that for the given platform and I suspect that's not worth the trouble.

FransUrbo · 2014-05-22T17:54:52Z

So what's your recommendation regarding strings and which?

And I was thinking about:

DebianZFS-Wheezy-SCST2:/usr/src/zfs# egrep 'systemd.*/usr' config/*
config/user-systemd.m4:         [install systemd unit files in dir [[/usr/lib/systemd/system]]]),
config/user-systemd.m4:         systemdunitdir=$withval,systemdunitdir=/usr/lib/systemd/system)
config/user-systemd.m4:         [install systemd preset files in dir [[/usr/lib/systemd/system-preset]]]),
config/user-systemd.m4:         systemdpresetdir=$withval,systemdpresetdir=/usr/lib/systemd/system-preset)

Why hardcode a default value, when it's better to just search for it?

behlendorf · 2014-06-03T00:21:38Z

So what's your recommendation regarding strings and which?

The most portable thing to do would be to avoid using them. We could simply do this with a conditional that checks the known possible paths.

Why hardcode a default value, when it's better to just search for it?

Because the distribution specific packaging should be authoritative about where things have to be installed for that distribution. It's normally up to that packaging to pass these values in to configure in the first place. But in order to make things a little more convenient for end users we've extended our build system to assume the settings on the local system are correct and to override the defaults in the packaging. Which is a little backwards since it assumes you're going to be running these packages on the build machine which may not be true...

FransUrbo · 2014-06-03T13:22:14Z

We could simply do this with a conditional that checks the known possible paths.

I really dislike hardcoding paths. They WILL break again, and we'll end up with yet another set of paths (and people complaining and have to wait months for the next version).
This isn't the only place where we've hardcoded paths, and eventually it will be a real nuisance keeping track of them all.
Why hardcode a default value, when it's better to just search for it?

Because the distribution specific packaging should be authoritative about where things have to be installed for that distribution.

Indeed! That's why we have the '--with-dracutdir' in the first place. That's for them to override any locations that might or might not be on the build system.

And for those that don't know or don't care (the majority), we search for it (we're assuming that the build machine is also the install system - but that's no different from what we do in other places!).

I think strings would be quite safe to use, it's in the binutils package which is required to build anything anyway.

And I've never seen a system which didn't have 'which'... On Debian GNU/Linux, that is in the 'debianutils' package with priority 'required'. So on any Debian GNU/Linux (and deviates), that should be safe to use as well. What's the status of that command in other systems (RedHat/Fedora based)?

If it's already a required command/package anyway... ?

for it (or accept the value provided by --with-dracutdir). + Don't call configure in the build target with dracutdir. New functionality is to search for it... + Call rpmbuild with the dracut dir to override the value in the spec file.

… dracut... Accept kernel source dir(s) specified by ./configure This adds ability to set the location of the kernel via defines when building from the spec files. This is useful when building against a kernel installed in a non-standard location. Signed-off-by: Turbo Fredriksson <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Closes openzfs#1874

FransUrbo · 2014-06-06T14:15:09Z

Rebased against latest master...

behlendorf · 2014-06-07T00:03:30Z

I'll get this reviewed and merged as soon as I can.

behlendorf · 2014-06-10T23:59:24Z

@FransUrbo How about something a little more like #2385. Because there was a lot of common code and the changes are related I squashed the commits together. The updated patch uses the same basic approach but there are a few small differences.

Very simple udev/dracut auto-detection code. There are only two paths I'm aware of in common distributions where these files are installed, we simply check both of them. I know this is a little ugly but it very maintainable and doesn't introduce any new dependencies. It does leave those paths hard coded but you they can be easily overridden.
Added code to pass _udevruledir as well. If we're going to do this we might as well allow this value to be passed as well.
Updated the spec file to include RHEL7 and CentOS7 defaults.

FransUrbo · 2014-06-11T00:33:49Z

I'm ok with that. Still hardcoded, but a good compromise...

behlendorf · 2014-06-11T22:55:09Z

@FransUrbo OK. Then let's go with this. I just hit one snag, which I've fixed, while testing this. But I think it's a good cautionary tale for why auto-detecting these things sounds like a good idea but can bite you if you're not very very careful.

It turns out that the ubuntu udev package creates both a /lib/udev and /usr/lib/udev directory. The /lib/udev directory contains all the expected stuff and the /usr/lib/udev directory is empty. I suspect this is just some sort of packaging bug. Regardless, because this patch was checking for the existence of the /usr/lib/udev directory before /lib/udev it caused us to install the udev bits in the wrong place. Consequently none of the udev helpers worked which thankfully resulted in a mess of test regression test failures.

behlendorf · 2014-06-11T23:25:09Z

Obsoleted by #2385

behlendorf added this to the 0.6.3 milestone May 7, 2014

behlendorf added the Build Issue label May 7, 2014

srlefevre mentioned this pull request Jun 3, 2014

zfs-dracut on CentOS 6.5 broken but I have a few fixes #2356

Closed

FransUrbo added 2 commits June 6, 2014 16:09

FransUrbo mentioned this pull request Jun 7, 2014

rpm/generic/zfs.spec.in ignores --with-dracutdir from ./configure #1680

Closed

behlendorf closed this Jun 11, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't hard code the dracut module directory #2312

Don't hard code the dracut module directory #2312

FransUrbo commented May 7, 2014

prakashsurya commented May 7, 2014

FransUrbo commented May 7, 2014

prakashsurya commented May 7, 2014

FransUrbo commented May 7, 2014

behlendorf commented May 19, 2014

FransUrbo commented May 20, 2014

FransUrbo commented May 20, 2014

FransUrbo commented May 20, 2014

behlendorf commented May 20, 2014

FransUrbo commented May 21, 2014

behlendorf commented May 21, 2014

FransUrbo commented May 21, 2014

behlendorf commented May 21, 2014

FransUrbo commented May 22, 2014

FransUrbo commented May 22, 2014

behlendorf commented May 22, 2014

FransUrbo commented May 22, 2014

behlendorf commented Jun 3, 2014

FransUrbo commented Jun 3, 2014

FransUrbo commented Jun 6, 2014

behlendorf commented Jun 7, 2014

behlendorf commented Jun 10, 2014

FransUrbo commented Jun 11, 2014

behlendorf commented Jun 11, 2014

behlendorf commented Jun 11, 2014

Don't hard code the dracut module directory #2312

Don't hard code the dracut module directory #2312

Conversation

FransUrbo commented May 7, 2014

prakashsurya commented May 7, 2014

FransUrbo commented May 7, 2014

prakashsurya commented May 7, 2014

FransUrbo commented May 7, 2014

behlendorf commented May 19, 2014

FransUrbo commented May 20, 2014

FransUrbo commented May 20, 2014

FransUrbo commented May 20, 2014

behlendorf commented May 20, 2014

FransUrbo commented May 21, 2014

behlendorf commented May 21, 2014

FransUrbo commented May 21, 2014

behlendorf commented May 21, 2014

FransUrbo commented May 22, 2014

FransUrbo commented May 22, 2014

behlendorf commented May 22, 2014

FransUrbo commented May 22, 2014

behlendorf commented Jun 3, 2014

FransUrbo commented Jun 3, 2014

FransUrbo commented Jun 6, 2014

behlendorf commented Jun 7, 2014

behlendorf commented Jun 10, 2014

FransUrbo commented Jun 11, 2014

behlendorf commented Jun 11, 2014

behlendorf commented Jun 11, 2014