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

Honor --with-mounthelperdir where applicable #6962

Merged
merged 1 commit into from Dec 17, 2017

Conversation

loli10K
Copy link
Contributor

@loli10K loli10K commented Dec 14, 2017

Description

Tiny fix: mount.zfs is always installed in "/sbin", if ZFS is ./configure'd with a custom (or the default "/usr/local") prefix dracut can't find the mount helper binary.

Motivation and Context

Custom ZFS builds may fail to mount filesystems.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Nice catch.

Copy link
Contributor

@dinatale2 dinatale2 left a comment

Choose a reason for hiding this comment

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

If you perform a ./configure --help you will notice that there is a --with-mounthelperdir option. We should honor that.

@behlendorf
Copy link
Contributor

Good catch @dinatale2, yes we should honor that. I'd forgotten it was added.

@loli10K
Copy link
Contributor Author

loli10K commented Dec 15, 2017

@dinatale2 thanks, i had missed that.

I'm kind of two minds about this: if i understand correctly --with-mounthelperdir was added to work around some specific Gentoo issue, so on one hand it's good to have the option to do that with a simple parameter.
On the other hand specifying --with-mounthelperdir=/any/other/path/than/sbin (or "/usr/sbin" considering usr merge?) will probably render the command useless, which defeats the purpose of having an option to begin with.

Out of curiosity i just installed ZFS on a Gentoo box and the build system doesn't seem to be using --with-mounthelperdir: does it still make sense to keep it or should we rip it out of the source tree?

>>> Preparing source in /var/tmp/portage/sys-fs/zfs-9999/work/zfs-9999 ...
 * Running eautoreconf in '/var/tmp/portage/sys-fs/zfs-9999/work/zfs-9999' ...
 * Running libtoolize --install --copy --force --automake ...            [ ok ]
 * Running aclocal -I config -I config ...                               [ ok ]
 * Running autoconf -I config --force ...                                [ ok ]
 * Running autoheader -I config ...                                      [ ok ]
 * Running automake --add-missing --copy --foreign --force-missing ...   [ ok ]
 * Running elibtoolize in: zfs-9999/
 * Running elibtoolize in: zfs-9999/config/
 *   Applying portage/1.2.0 patch ...
 *   Applying sed/1.5.6 patch ...
 *   Applying as-needed/2.4.3 patch ...
>>> Source prepared.
>>> Configuring source in /var/tmp/portage/sys-fs/zfs-9999/work/zfs-9999 ...
>>> Working in BUILD_DIR: "/var/tmp/portage/sys-fs/zfs-9999/work/zfs-9999"
 * econf: updating zfs-9999/config/config.sub with /usr/share/gnuconfig/config.sub
 * econf: updating zfs-9999/config/config.guess with /usr/share/gnuconfig/config.guess
/var/tmp/portage/sys-fs/zfs-9999/work/zfs-9999/configure --prefix=/usr --build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu --mandir=/usr/share/man --infodir=/usr/share/info --datadir=/usr/share --sysconfdir=/etc --localstatedir=/var/lib --disable-dependency-tracking --disable-silent-rules --libdir=/usr/lib64 --docdir=/usr/share/doc/zfs-9999 --enable-shared --disable-static --bindir=/bin --sbindir=/sbin --with-config=user --with-dracutdir=/usr/lib64/dracut --with-linux= --with-linux-obj= --with-udevdir=/lib/udev --with-blkid --disable-debug
configure: WARNING: unrecognized options: --with-blkid
checking for gawk... gawk
checking metadata... git describe
checking build system type... x86_64-pc-linux-gnu
checking host system type... x86_64-pc-linux-gnu
checking target system type... x86_64-pc-linux-gnu
checking whether to enable maintainer-specific portions of Makefiles... no
checking whether make supports nested variables... yes
checking for a BSD-compatible install... /usr/lib/portage/python2.7/ebuild-helpers/xattr/install -c

@behlendorf
Copy link
Contributor

@loli10K since we're not sure who may be using this functionality lets play it safe and keep it for the moment. @ryao may be able to shed some additional light on if this is still needed/useful for Gentoo.

@loli10K
Copy link
Contributor Author

loli10K commented Dec 15, 2017

@behlendorf i'll update the PR accordingly to honor --with-mounthelperdir= in other places as well (contrib/initramfs comes to mind).

@loli10K loli10K changed the title contrib/dracut: fix mount.zfs helper install path Honor --with-mounthelperdir where applicable Dec 15, 2017
@@ -5,7 +5,7 @@ check() {
[ "${1}" = "-d" ] && return 0

# Verify the zfs tool chain
for tool in "@sbindir@/zpool" "@sbindir@/zfs" "@sbindir@/mount.zfs" ; do
for tool in "@sbindir@/zpool" "@sbindir@/zfs" "@mounthelperdir@/mount.zfs" ; do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you may need to add -e 's,@mounthelperdir\@,$(mounthelperdir),g' to the sed command in contrib/dracut/90zfs/Makefile.am.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dinatale2 you're right, thanks.

Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
@behlendorf behlendorf merged commit e2d936e into openzfs:master Dec 17, 2017
@loli10K loli10K deleted the fix-dracut-mount.zfs branch December 17, 2017 22:24
@codecov
Copy link

codecov bot commented Dec 17, 2017

Codecov Report

Merging #6962 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6962      +/-   ##
==========================================
+ Coverage   75.17%   75.19%   +0.01%     
==========================================
  Files         296      296              
  Lines       95453    95453              
==========================================
+ Hits        71759    71772      +13     
+ Misses      23694    23681      -13
Flag Coverage Δ
#kernel 74.67% <ø> (+0.18%) ⬆️
#user 67.39% <ø> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6940bb...b9927ec. Read the comment docs.

Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Jan 29, 2018
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#6962
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Feb 13, 2018
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#6962
Conan-Kudo pushed a commit to Conan-Kudo/zfs that referenced this pull request Jan 17, 2019
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#6962 
(cherry picked from commit e2d936e)
Conan-Kudo pushed a commit to Conan-Kudo/zfs that referenced this pull request Jan 17, 2019
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#6962

(cherry picked from commit e2d936e)

Signed-off-by: Neal Gompa <ngompa@datto.com>

Requires-spl: spl-0.7-release
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jan 30, 2019
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#6962
tonyhutter pushed a commit that referenced this pull request Mar 4, 2019
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes #6962
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants