[Buildroot] [PATCH v6 1/2] e2fsprogs: refactor to fix conflicts with busybox and util-linux

Arnout Vandecappelle arnout at mind.be
Mon Apr 10 12:45:05 UTC 2017


 Hm, I had half-finished a review of v5 when this came in :-) And some of the
typos I was going to say are already fixed, so that's good.


On 10-04-17 13:49, Carlos Santos wrote:
> So far we attempted to solve the conflicts between busybox and e2fsprogs
> by removing busybox programs from /bin and /sbin, leaving the e2fsprogs
> ones at /usr/bin and /usr/sbin. This fails with BR2_ROOTFS_MERGED_USR=y,
> leading to situations like the one described in bug 9436.
> 
> We could provide a better solution by means of a fine-grained selection
> of programs, like util-linux does, but this would require big changes in
> e2fsprogs. So instead of resorting to dirty tricks we switch to a more
> pragmatic approach:
> 
> 1. Drop all configs to select/deselect utilities without corresponding
>    --enable/disable options to the configure script. In other words, we
>    always install the basic set of utilities.
> 2. Do not try to build UUID utilities, since they are disabled along
>    with libuuid (the util-linux ones must be used instead).
> 3. Install e2fsprogs utilities at /bin and /sbin, overriding the ones
>    eventually installed by busybox.
> 
> Notice that these changes do exactly the opposite of what is requested
> in bug 9436. On the other hand the policy for e2fsprogs becomes coherent
> with the one for util-linux: busybox never wins.
> 
> Since e2fsprogs depends on util-linux, it's built later. So in order to
> have e2fsck from e2fsprogs and the fsck wrapper from util-linux (which
> is better maintained and compatible with systemd) we disable e2fsprogs'
> fsck if the one from util-linux is selected.
> 
> Fixes:
>   https://bugs.busybox.net/show_bug.cgi?id=9436 (no fix, in fact)
> 
> Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian at gmail.com>

 This has changed so much that I doubt the Sob of Maxime is still correct...
Just turn it into a Cc. Cc makes sure that:

- Maxime is kept in Cc, obviously, but also
- the git log shows that Maxime was involved in this series.

 Regards,
 Arnout


> Signed-off-by: Carlos Santos <casantos at datacom.ind.br>
> ---
> Changes v1->v2
>   - Add depenndece on !BR2_PACKAGE_UTIL_LINUX_FSCK to
>     BR2_PACKAGE_E2FSPROGS_FSCK
>   - Add comment telling that the fsck from util-linux has preference.
>   - Pass "--enable-fsck" conditionally to configure
>   - Remove the guard around the removal of /usr/sbin/fsck, since the
>     fsck from busybox and util-linux are installed at /sbin
>   - Improve comments and commit message
> Changes v2->v3
>   - Pass "--disable-fsck" conditionaly to configure too, since enable
>     is the default.
>   - Do not attempt to remove $(TARGET_DIR)/usr/sbin/fsck, just the
>     $(TARGET_DIR)/sbin/fsck from busybox.
> Changes v3->v4
>    - Improve commit message
> Changes v4->v5
>    - Extreme overhauling, deeply simplifying the recipe and following
>      the suggestion from Arnout Vandecappelle to install e2fsprogs
>      utilities on /bin and /sbin.
> Changes v5->v6
>    - Fix typos and make small changes in commit message
> ---
>  package/e2fsprogs/Config.in      |  84 +++++++---------------
>  package/e2fsprogs/Config.in.host |   1 +
>  package/e2fsprogs/e2fsprogs.mk   | 149 +++++++++++----------------------------
>  3 files changed, 68 insertions(+), 166 deletions(-)
> 
> diff --git a/package/e2fsprogs/Config.in b/package/e2fsprogs/Config.in
> index d1914a9..776645f 100644
> --- a/package/e2fsprogs/Config.in
> +++ b/package/e2fsprogs/Config.in
> @@ -1,101 +1,69 @@
> -config BR2_PACKAGE_E2FSPROGS
> +menuconfig BR2_PACKAGE_E2FSPROGS

  Wow, you're changing way too much here in a single patch... Changing the
config into a menuconfig may be a good idea (we more or less decided that
starting from 5 suboptions it should be a menuconfig), but it certainly should
be a separate patch.

 I propose that you split up into:

- config -> menuconfig

- update upstream URL (with as commit log: "e2fsprogs.sf.net redirects to
e2fsprogs.sourceforge.net, so use that URL directly")

- don't build uuid tools

- remove options for basic set

- add help text for remaining options

- move from /usr/{bin,sbin} to /{bin,sbin}

- whatever you're doing with host-e2fsprogs and for which no explanation is
given in the commit message

 I may have missed something that also needs to be split off, but this patch is
too complicated to detect even that :-)


 I'll give a few other comments below still, but I'm afraid there will be more
in v7...


>  	bool "e2fsprogs"
>  	depends on BR2_USE_MMU # util-linux/libblkid
>  	select BR2_PACKAGE_UTIL_LINUX
>  	select BR2_PACKAGE_UTIL_LINUX_LIBBLKID
> -	select BR2_PACKAGE_UTIL_LINUX_LIBUUID
>  	help
>  	  The EXT2 file system utilities.
>  
> -	  http://e2fsprogs.sf.net
> +	  badblocks chattr debugfs dumpe2fs e2freefrag e2fsck e2image
> +	  e2undo e4crypt e4defrag filefrag fsck fuse2fs logsave lsattr
> +	  mke2fs mklost+found resize2fs tune2fs

 This could be a little more explicit:

	  The following programs are always built and installed:
	  badblocks ...

	  Other programs can be selected below.

>  
> -if BR2_PACKAGE_E2FSPROGS
>  
> -config BR2_PACKAGE_E2FSPROGS_BADBLOCKS
> -	bool "badblocks"
> -	default y
> +	  http://e2fsprogs.sourceforge.net/
>  
> -config BR2_PACKAGE_E2FSPROGS_CHATTR
> -	bool "chattr"
> -	default y
> +if BR2_PACKAGE_E2FSPROGS
> +
> +comment "Basic set: badblocks chattr dumpe2fs e2freefrag e2fsck e2undo"
> +comment "e4crypt e4defrag filefrag logsave lsattr mke2fs mklost+found"
> +comment "tune2fs"

 This comment is not needed. It's already said in the help text.

>  
>  config BR2_PACKAGE_E2FSPROGS_DEBUGFS
>  	bool "debugfs"
> -
> -config BR2_PACKAGE_E2FSPROGS_DUMPE2FS
> -	bool "dumpe2fs"
> -	default y
> -
> -config BR2_PACKAGE_E2FSPROGS_E2FREEFRAG
> -	bool "e2freefrag"
> -	default y
> -
> -config BR2_PACKAGE_E2FSPROGS_E2FSCK
> -	bool "e2fsck"
> -	default y
> +	help
> +	  ext2/ext3/ext4 file system debugger
>  
>  config BR2_PACKAGE_E2FSPROGS_E2IMAGE
>  	bool "e2image"
> -
> -config BR2_PACKAGE_E2FSPROGS_E2LABEL
> -	bool "e2label"
> -	default y
> -
> -config BR2_PACKAGE_E2FSPROGS_E2UNDO
> -	bool "e2undo"
> -	default y
> +	help
> +	  Save critical ext2/ext3/ext4 filesystem metadata to a file
>  
>  config BR2_PACKAGE_E2FSPROGS_E4DEFRAG
>  	bool "e4defrag"
>  	depends on !BR2_nios2 # fallocate not implemented
>  	depends on !BR2_TOOLCHAIN_USES_UCLIBC # sync_file_range not impl
> +	help
> +	  Online defragmenter for ext4 filesystem
>  
>  comment "e4defrag needs a glibc or musl toolchain"
>  	depends on BR2_TOOLCHAIN_USES_UCLIBC

 Hm, looks like there is a
	depends on !BR2_nios2
missing here...

>  
> -config BR2_PACKAGE_E2FSPROGS_FILEFRAG
> -	bool "filefrag"
> -	default y
> -
>  config BR2_PACKAGE_E2FSPROGS_FSCK
>  	bool "fsck"
> +	depends on !BR2_PACKAGE_UTIL_LINUX_FSCK
>  	default y
> +	help
> +	  Check and repair a Linux file system

 This sounds as if fsck does the actual filesystem check, which it doesn't. How
about:

	  Check and repair Linux file systems. This is a wrapper around
	  the filesystem-specific fsck tools.

 Yes, I do realize that makes it differ from the help text in util-linux, which
is not ideal. But when you add something, it's better to do the right thing :-)

> +
> +comment "the fsck from util-linux has preference over this one"
> +	depends on BR2_PACKAGE_UTIL_LINUX_FSCK
>  
>  config BR2_PACKAGE_E2FSPROGS_FUSE2FS
>  	bool "fuse2fs"
>  	depends on !BR2_STATIC_LIBS # libfuse
>  	depends on BR2_TOOLCHAIN_HAS_THREADS # libfuse
>  	select BR2_PACKAGE_LIBFUSE
> +	help
> +	  FUSE file system client for ext2/ext3/ext4 file systems
>  
>  comment "fuse2fs needs a toolchain w/ threads, dynamic library"
>  	depends on !BR2_TOOLCHAIN_HAS_THREADS || BR2_STATIC_LIBS
>  
> -config BR2_PACKAGE_E2FSPROGS_LOGSAVE
> -	bool "logsave"
> -	default y
> -
> -config BR2_PACKAGE_E2FSPROGS_LSATTR
> -	bool "lsattr"
> -	default y
> -
> -config BR2_PACKAGE_E2FSPROGS_MKE2FS
> -	bool "mke2fs"
> -	default y
> -
> -config BR2_PACKAGE_E2FSPROGS_MKLOSTFOUND
> -	bool "mklost+found"
> -	default y
> -
>  config BR2_PACKAGE_E2FSPROGS_RESIZE2FS
>  	bool "resize2fs"
> -
> -config BR2_PACKAGE_E2FSPROGS_TUNE2FS
> -	bool "tune2fs"
> -	default y
> -	select BR2_PACKAGE_E2FSPROGS_E2LABEL
> -
> -config BR2_PACKAGE_E2FSPROGS_UUIDGEN
> -	bool "uuidgen"
>  	default y
> +	help
> +	  ext2/ext3/ext4 file system resizer
>  
>  endif
> diff --git a/package/e2fsprogs/Config.in.host b/package/e2fsprogs/Config.in.host
> index de9148e..7591bbc 100644
> --- a/package/e2fsprogs/Config.in.host
> +++ b/package/e2fsprogs/Config.in.host
> @@ -1,5 +1,6 @@
>  config BR2_PACKAGE_HOST_E2FSPROGS
>  	bool "host e2fsprogs"
> +	select BR2_PACKAGE_HOST_UTIL_LINUX
>  	help
>  	  The EXT2/3/4 file system utilities.
>  
> diff --git a/package/e2fsprogs/e2fsprogs.mk b/package/e2fsprogs/e2fsprogs.mk
> index 3f235c5..e172914 100644
> --- a/package/e2fsprogs/e2fsprogs.mk
> +++ b/package/e2fsprogs/e2fsprogs.mk
> @@ -10,36 +10,58 @@ E2FSPROGS_SITE = $(BR2_KERNEL_MIRROR)/linux/kernel/people/tytso/e2fsprogs/v$(E2F
>  E2FSPROGS_LICENSE = GPL-2.0, BSD-3-Clause (libuuid), MIT-like with advertising clause (libss and libet)
>  E2FSPROGS_LICENSE_FILES = NOTICE lib/uuid/COPYING lib/ss/mit-sipb-copyright.h lib/et/internal.h
>  E2FSPROGS_INSTALL_STAGING = YES
> +
>  E2FSPROGS_DEPENDENCIES = host-pkgconf util-linux
> -# we don't have a host-util-linux
> -HOST_E2FSPROGS_DEPENDENCIES = host-pkgconf
> +
> +HOST_E2FSPROGS_DEPENDENCIES = host-pkgconf host-util-linux

 Actually I'm not sure if we really want to depend on host-util-linux... Why
should we?

> +
> +# If both e2fsprogs and busybox are selected, make certain e2fsprogs
> +# wins the fight over who gets to have their utils actually installed
> +ifeq ($(BR2_PACKAGE_BUSYBOX),y)
> +E2FSPROGS_DEPENDENCIES += busybox
> +endif
> +
> +ifeq ($(BR2_PACKAGE_E2FSPROGS_FUSE2FS),y)
> +E2FSPROGS_DEPENDENCIES += libfuse
> +endif
>  
>  # e4defrag doesn't build on older systems like RHEL5.x, and we don't
>  # need it on the host anyway.
>  # Disable fuse2fs as well to avoid carrying over deps, and it's unused
> -HOST_E2FSPROGS_CONF_OPTS += --disable-defrag --disable-fuse2fs
> +# Set the binary directories to "$(HOST_DIR)/bin" and "$(HOST_DIR)/sbin"
> +# to match the target paths (the $(HOST_DIR) prefix is required because
> +# passing DESTDIR=$(HOST_DIR) in the install command does not work).
> +HOST_E2FSPROGS_CONF_OPTS = \
> +	--disable-defrag \
> +	--disable-fuse2fs \
> +	--bindir=$(HOST_DIR)/bin \
> +	--sbindir=$(HOST_DIR)/sbin \
> +	--enable-symlink-install \
> +	--disable-libblkid \
> +	--disable-libuuid \
> +	--disable-e2initrd-helper \
> +	--disable-testio-debug \
> +	--disable-rpath
>  
> +# Set the binary directories to "/bin" and "/sbin" to override programs
> +# installed by busybox.
>  E2FSPROGS_CONF_OPTS = \
> -	$(if $(BR2_STATIC_LIBS),,--enable-elf-shlibs) \
> -	$(if $(BR2_PACKAGE_E2FSPROGS_DEBUGFS),,--disable-debugfs) \
> -	$(if $(BR2_PACKAGE_E2FSPROGS_E2IMAGE),,--disable-imager) \
> -	$(if $(BR2_PACKAGE_E2FSPROGS_E4DEFRAG),,--disable-defrag) \
> -	$(if $(BR2_PACKAGE_E2FSPROGS_RESIZE2FS),,--disable-resizer) \
> -	--disable-uuidd \
> +	$(if $(BR2_STATIC_LIBS),--disable-elf-shlibs,--enable-elf-shlibs) \
> +	$(if $(BR2_PACKAGE_E2FSPROGS_DEBUGFS),--enable-debugfs,--disable-debugfs) \
> +	$(if $(BR2_PACKAGE_E2FSPROGS_E2IMAGE),--enable-imager,--disable-imager) \
> +	$(if $(BR2_PACKAGE_E2FSPROGS_E4DEFRAG),--enable-defrag,--disable-defrag) \
> +	$(if $(BR2_PACKAGE_E2FSPROGS_FSCK),--enable-fsck,--disable-fsck) \
> +	$(if $(BR2_PACKAGE_E2FSPROGS_FUSE2FS),--enable-fuse2fs,--disable-fuse2fs) \
> +	$(if $(BR2_PACKAGE_E2FSPROGS_RESIZE2FS),--enable-resizer,--disable-resizer) \
> +	--bindir=/bin \
> +	--sbindir=/sbin \
> +	--enable-symlink-install \
>  	--disable-libblkid \
>  	--disable-libuuid \
> -	--enable-fsck \
>  	--disable-e2initrd-helper \
>  	--disable-testio-debug \
>  	--disable-rpath
>  
> -ifeq ($(BR2_PACKAGE_E2FSPROGS_FUSE2FS),y)
> -E2FSPROGS_CONF_OPTS += --enable-fuse2fs
> -E2FSPROGS_DEPENDENCIES += libfuse
> -else
> -E2FSPROGS_CONF_OPTS += --disable-fuse2fs
> -endif

 Don't change this, it was better like that.

> -
>  ifeq ($(BR2_nios2),y)
>  E2FSPROGS_CONF_ENV += ac_cv_func_fallocate=no
>  endif
> @@ -56,17 +78,13 @@ HOST_E2FSPROGS_CONF_ENV += \
>  	ac_cv_header_magic_h=no \
>  	ac_cv_lib_magic_magic_file=no
>  
> -ifeq ($(BR2_NEEDS_GETTEXT_IF_LOCALE),y)
> -# util-linux libuuid pulls in libintl if needed, so ensure we also
> -# link against it, otherwise static linking fails
> -E2FSPROGS_CONF_ENV += LIBS=-lintl
> -endif

 Why can this be removed? IIUC, e2fsprogs will now use the uuid from util-linux
if that is enabled, and will not use uuid at all if not. Correct? So if
util-linux libuuid is enabled, this is still needed, no?

> -
>  E2FSPROGS_MAKE_OPTS = LDCONFIG=true
> +
>  E2FSPROGS_INSTALL_STAGING_OPTS = \
>  	DESTDIR=$(STAGING_DIR) \
>  	LDCONFIG=true \
>  	install-libs
> +
>  E2FSPROGS_INSTALL_TARGET_OPTS = \
>  	DESTDIR=$(TARGET_DIR) \
>  	LDCONFIG=true \
> @@ -76,90 +94,5 @@ define HOST_E2FSPROGS_INSTALL_CMDS
>  	$(HOST_MAKE_ENV) $(MAKE) -C $(@D) install install-libs
>  endef
>  
> -# binaries to keep or remove
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_BADBLOCKS) += usr/sbin/badblocks
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_CHATTR) += usr/bin/chattr
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_DUMPE2FS) += usr/sbin/dumpe2fs
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_E2FREEFRAG) += usr/sbin/e2freefrag
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_E2FSCK) += usr/sbin/e2fsck
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_E2LABEL) += usr/sbin/e2label
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_E2UNDO) += usr/sbin/e2undo
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_E4DEFRAG) += usr/sbin/e4defrag
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_FILEFRAG) += usr/sbin/filefrag
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_FSCK) += usr/sbin/fsck
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_LOGSAVE) += usr/sbin/logsave
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_LSATTR) += usr/bin/lsattr
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_MKE2FS) += usr/sbin/mke2fs
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_MKLOSTFOUND) += usr/sbin/mklost+found
> -E2FSPROGS_BINTARGETS_$(BR2_PACKAGE_E2FSPROGS_UUIDGEN) += usr/bin/uuidgen
> -
> -# files to remove
> -E2FSPROGS_TXTTARGETS_ = \
> -	usr/sbin/mkfs.ext[234] \
> -	usr/sbin/mkfs.ext4dev \
> -	usr/sbin/fsck.ext[234] \
> -	usr/sbin/fsck.ext4dev \
> -	usr/sbin/tune2fs
> -
> -define E2FSPROGS_TARGET_REMOVE_UNNEEDED
> -	rm -f $(addprefix $(TARGET_DIR)/, $(E2FSPROGS_BINTARGETS_))
> -	rm -f $(addprefix $(TARGET_DIR)/, $(E2FSPROGS_TXTTARGETS_))
> -endef
> -
> -E2FSPROGS_POST_INSTALL_TARGET_HOOKS += E2FSPROGS_TARGET_REMOVE_UNNEEDED
> -
> -define E2FSPROGS_TARGET_MKE2FS_SYMLINKS
> -	ln -sf mke2fs $(TARGET_DIR)/usr/sbin/mkfs.ext2
> -	ln -sf mke2fs $(TARGET_DIR)/usr/sbin/mkfs.ext3
> -	ln -sf mke2fs $(TARGET_DIR)/usr/sbin/mkfs.ext4
> -	ln -sf mke2fs $(TARGET_DIR)/usr/sbin/mkfs.ext4dev

 Can you explain why this is no longer needed? I don't see how any of your other
changes could obviate this one...

 If it actually wasn't needed to begin with, please also make it a separate
patch with a good explanation.


 Regards,
 Arnout

> -endef
> -
> -ifeq ($(BR2_PACKAGE_E2FSPROGS_MKE2FS),y)
> -E2FSPROGS_POST_INSTALL_TARGET_HOOKS += E2FSPROGS_TARGET_MKE2FS_SYMLINKS
> -endif
> -
> -define E2FSPROGS_TARGET_E2FSCK_SYMLINKS
> -	ln -sf e2fsck $(TARGET_DIR)/usr/sbin/fsck.ext2
> -	ln -sf e2fsck $(TARGET_DIR)/usr/sbin/fsck.ext3
> -	ln -sf e2fsck $(TARGET_DIR)/usr/sbin/fsck.ext4
> -	ln -sf e2fsck $(TARGET_DIR)/usr/sbin/fsck.ext4dev
> -endef
> -
> -ifeq ($(BR2_PACKAGE_E2FSPROGS_E2FSCK),y)
> -E2FSPROGS_POST_INSTALL_TARGET_HOOKS += E2FSPROGS_TARGET_E2FSCK_SYMLINKS
> -endif
> -
> -# If BusyBox is included, its configuration may supply its own variant
> -# of ext2-related tools. Since Buildroot desires having full blown
> -# variants take precedence (in this case, e2fsprogs), we want to remove
> -# BusyBox's variant of e2fsprogs provided binaries. e2fsprogs targets
> -# /usr/{bin,sbin} where BusyBox targets /{bin,sbin}. We will attempt to
> -# remove BusyBox-generated ext2-related tools from /{bin,sbin}. We need
> -# to do this in the pre-install stage to ensure we do not accidentally
> -# remove e2fsprogs's binaries in usr-merged environments (ie. if they
> -# are removed, they would be re-installed in this package's install
> -# stage).
> -ifeq ($(BR2_PACKAGE_BUSYBOX),y)
> -E2FSPROGS_DEPENDENCIES += busybox
> -
> -define E2FSPROGS_REMOVE_BUSYBOX_APPLETS
> -	$(RM) -f $(TARGET_DIR)/bin/chattr
> -	$(RM) -f $(TARGET_DIR)/bin/lsattr
> -	$(RM) -f $(TARGET_DIR)/sbin/fsck
> -	$(RM) -f $(TARGET_DIR)/sbin/tune2fs
> -	$(RM) -f $(TARGET_DIR)/sbin/e2label
> -endef
> -E2FSPROGS_PRE_INSTALL_TARGET_HOOKS += E2FSPROGS_REMOVE_BUSYBOX_APPLETS
> -endif
> -
> -define E2FSPROGS_TARGET_TUNE2FS_SYMLINK
> -	ln -sf e2label $(TARGET_DIR)/usr/sbin/tune2fs
> -endef
> -
> -ifeq ($(BR2_PACKAGE_E2FSPROGS_TUNE2FS),y)
> -E2FSPROGS_POST_INSTALL_TARGET_HOOKS += E2FSPROGS_TARGET_TUNE2FS_SYMLINK
> -endif
> -
>  $(eval $(autotools-package))
>  $(eval $(host-autotools-package))
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF



More information about the buildroot mailing list