On 07/02/2014 04:34 PM, Pádraig Brady wrote: > On 07/02/2014 01:44 AM, Alex Deymo wrote: >> Hi again, >> >> Sorry for the delay with this patch. Here is patch v7 attached. >> >> *** symlinks vs shebangs: >> >> * Other projects use symlinks to have a multicall binary; including busybox >> and toolbox which cover similar functionality as coreutils; but not limited >> to them: lvm (vgcreate, pvcreate, etc), xz (xzcat, unxz point to "xz") and >> others I'm not aware of. >> >> * People using symlinks do not protect against the symlink to symlink case; >> they just rely on argv[0]. symlink to symlink is not the only way to provide >> a different argv[0]; you can pass whatever you want as the first argument of >> argv when you call exec(). >> >> * The scheme to follow symlinks to determine which one was the last symlink >> requires to accurately reproduce the path search algorithm used by exec() on >> the platform we are running; and requires the make a couple of syscalls just >> to know what's the tool to call, yet still doesn't solve the whole case. >> >> * Shebangs allow you to have a symlink to them and it still works without >> extra work. On some systems, we can change the value of /proc/$PID/cmdline >> and other files to not break use cases like "killall foo" or the result of >> "ps". >> >> >> In general, if you want to use --enable-single-binary is because size is a >> concern; and in those cases you know your environment (you are deploying an >> embedded system with a particular kernel). You might not care about 4MB in a >> multi GB linux distro and there's people out there still doing symlink to >> symlink that eventually will get their scripts right; but not yet. >> >> So, I added an option where you can choose at configure time if you want to >> install symlinks or shebangs (--enable-single-binary=shebangs for example). >> I tested this patch on the ChromiumOS builders and it worked (we managed to >> build a builder environment and images for amd64, x86 and arm). >> >> In the shebang case, the program will attempt to modify /proc/$PID/cmdline >> by using prctl() if available. There are other ways to modify argv[] on >> other platforms (look at the sendmail or postgresql case for example), so >> eventually we should migrate that functionality to a gnulib file and make it >> work on all the supported platforms... but for now, you still have the >> choice to use symlinks if that works for you. >> >> Let me know if you have other questions about this patch and happy canada >> day for the canadians out there on this list. >> >> deymo. >> > > Excellent work. > > One small issue I noticed (in symlinks mode at least) > was that `make syntax-check` causes all the > tools to be remade as stand-alone binaries. > This is related to `make src/$binary` replacing > the symlink with a stand-alone binary. > > Note that isn't a supported end user mode anyway, > so it's not blocking, though nice to avoid. > > As an aside, the reason end users can not do: > make src/$binary > is due to our use of BUILT_SOURCES of which the automake manual says: > "you cannot use BUILT_SOURCES if the ability to run > ‘make foo’ on a clean tree is important to you." > coreutils uses BUILT_SOURCES, as the mandatory configure > step is expensive compared to the build itself, > so there is not much advantage to supporting that. > > A more problematic issue is that program prefix isn't supported. > i.e.: ./configure --enable-single-binary --program-prefix=g > > BTW I've attached some spelling fixes which I've rolled in here. > > thanks! > Pádraig. >
Getting ready to merge this. Attached are the following adjustments: commit 6a7418ca215f8d99107948b95ff5d3d7672d7a2d Author: Pádraig Brady <[email protected]> Date: Fri Jul 4 15:59:17 2014 +0100 build: use the installed program name in --help and /proc/$PID/comm Note we still use the internal coreutils names for --coreutils-prog, independent of what's used on the system due to --prefix-prog etc. So adjust the argv appropriately so that --help and /proc/$PID/comm use the correct command name for the current install. commit 1a7c3b46056f009624a2d1e18a7bbe70e5d67eff Author: Pádraig Brady <[email protected]> Date: Fri Jul 4 15:19:54 2014 +0100 tests: avoid false failure without a working strip * tests/install/basic-1.sh: Really avoid using ginstall strip functionality if there is an issue with the independent strip command. This avoids a false failure with --enable-single-binary=shebangs. commit c53e8a03df916c7757b371f95435380087afecd0 Author: Pádraig Brady <[email protected]> Date: Fri Jul 4 15:09:17 2014 +0100 build: support directly calling coreutils with shebangs enabled When called with a shebang, the path of the shebang script will be added to the command line. Therefore there is a different number of args to strip depending on whether we're calling through the shebang script or directly calling `coreutils --coreutils-prog`. Also adjust argv[0] to just the identified program name. I.E. without the path. This is usually correct, and allows error string matching in the tests to work as expected. commit 1ec17595c0c80757a794d9ffa68ed9f64c6a195f Author: Pádraig Brady <[email protected]> Date: Fri Jul 4 15:04:49 2014 +0100 build: support --program-prefix et. al. with --enable-single-binary * Makefile.am: Honor any user configured $transform. This also uses 'install' rather than 'ginstall' as the destinary path.
>From 1ec17595c0c80757a794d9ffa68ed9f64c6a195f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Fri, 4 Jul 2014 15:04:49 +0100 Subject: [PATCH 1/4] build: support --program-prefix et. al. with --enable-single-binary * Makefile.am: Honor any user configured $transform. This also uses 'install' rather than 'ginstall' as the destinary path. --- Makefile.am | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-) diff --git a/Makefile.am b/Makefile.am index f4d5fef..20de43b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -189,19 +189,19 @@ check-git-hook-script-sync: # If we are building a single-binary, create symlinks or shebangs for # the selected tools when installing. install-exec-hook: - $(AM_V_at)for i in $(single_binary_progs); do \ - rm -f $(DESTDIR)$(bindir)/$$i$(EXEEXT) || exit $$?; \ - if test "x$(single_binary_install_type)" = xshebangs; then \ - printf '#!%s --coreutils-prog=%s\n' \ - $(bindir)/coreutils$(EXEEXT) $$i \ - >$(DESTDIR)$(bindir)/$$i$(EXEEXT) || exit $$?; \ - chmod a+x,a-w $(DESTDIR)$(bindir)/$$i$(EXEEXT) \ - || exit $$?; \ - else \ - $(LN_S) -s coreutils$(EXEEXT) \ - $(DESTDIR)$(bindir)/$$i$(EXEEXT) \ - || exit $$?; \ - fi \ + $(AM_V_at)ctrans=$$(printf coreutils | sed -e "$(transform)"); \ + for p in $(single_binary_progs); do \ + ptrans=$$(printf '%s' "$$p" | sed -e "$(transform)"); \ + rm -f $(DESTDIR)$(bindir)/$$ptrans$(EXEEXT) || exit $$?; \ + if test "x$(single_binary_install_type)" = xshebangs; then \ + printf '#!%s --coreutils-prog=%s\n' \ + $(bindir)/$$ctrans$(EXEEXT) $$p \ + >$(DESTDIR)$(bindir)/$$ptrans$(EXEEXT) || exit $$?; \ + chmod a+x,a-w $(DESTDIR)$(bindir)/$$ptrans$(EXEEXT) || exit $$?;\ + else \ + $(LN_S) -s $$ctrans$(EXEEXT) \ + $(DESTDIR)$(bindir)/$$ptrans$(EXEEXT) || exit $$?; \ + fi \ done noinst_LIBRARIES = -- 1.7.7.6 >From c53e8a03df916c7757b371f95435380087afecd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Fri, 4 Jul 2014 15:09:17 +0100 Subject: [PATCH 2/4] build: support directly calling coreutils with shebangs enabled When called with a shebang, the path of the shebang script will be added to the command line. Therefore there is a different number of args to strip depending on whether we're calling through the shebang script or directly calling `coreutils --coreutils-prog`. Also adjust argv[0] to just the identified program name. I.E. without the path. This is usually correct, and allows error string matching in the tests to work as expected. --- Makefile.am | 2 +- src/coreutils.c | 26 ++++++++++++++++++++++---- src/local.mk | 2 +- tests/local.mk | 1 + tests/misc/coreutils.sh | 31 +++++++++++++++++++++++++++++++ 5 files changed, 56 insertions(+), 6 deletions(-) create mode 100755 tests/misc/coreutils.sh diff --git a/Makefile.am b/Makefile.am index 20de43b..fb4af27 100644 --- a/Makefile.am +++ b/Makefile.am @@ -194,7 +194,7 @@ install-exec-hook: ptrans=$$(printf '%s' "$$p" | sed -e "$(transform)"); \ rm -f $(DESTDIR)$(bindir)/$$ptrans$(EXEEXT) || exit $$?; \ if test "x$(single_binary_install_type)" = xshebangs; then \ - printf '#!%s --coreutils-prog=%s\n' \ + printf '#!%s --coreutils-prog-shebang=%s\n' \ $(bindir)/$$ctrans$(EXEEXT) $$p \ >$(DESTDIR)$(bindir)/$$ptrans$(EXEEXT) || exit $$?; \ chmod a+x,a-w $(DESTDIR)$(bindir)/$$ptrans$(EXEEXT) || exit $$?;\ diff --git a/src/coreutils.c b/src/coreutils.c index f54f85d..cda7eb3 100644 --- a/src/coreutils.c +++ b/src/coreutils.c @@ -102,7 +102,7 @@ launch_program (const char *prog_name, int prog_argc, char **prog_argv) #include "coreutils.h" #undef SINGLE_BINARY_PROGRAM - if (!prog_main) + if (! prog_main) return; #if HAVE_PRCTL && defined PR_SET_NAME @@ -135,10 +135,28 @@ main (int argc, char **argv) path/to/coreutils --coreutils-prog=someprog someprog ... The third argument is what the program will see as argv[0]. */ - if (argc >= 2 && STRPREFIX (argv[1], "--coreutils-prog=")) + if (argc >= 2) { - prog_name = argv[1] + strlen ("--coreutils-prog="); - launch_program (prog_name, argc - 2, argv + 2); + size_t nskip = 0; + + /* If calling coreutils directly, the "script" name isn't passed. + Distinguish the two cases with a -shebang suffix. */ + if (STRPREFIX (argv[1], "--coreutils-prog=")) + { + nskip = 1; + prog_name = argv[1] + strlen ("--coreutils-prog="); + } + else if (STRPREFIX (argv[1], "--coreutils-prog-shebang=")) + { + nskip = 2; + prog_name = argv[1] + strlen ("--coreutils-prog-shebang="); + } + + if (nskip) + { + argv[nskip] = prog_name; /* XXX: Discards any specified path. */ + launch_program (prog_name, argc - nskip, argv + nskip); + } } /* No known program was selected. From here on, we behave like any other diff --git a/src/local.mk b/src/local.mk index 134f00b..e1a5632 100644 --- a/src/local.mk +++ b/src/local.mk @@ -427,7 +427,7 @@ src/coreutils_shebangs: Makefile $(AM_V_GEN)touch $@ $(AM_V_at)for i in $(single_binary_progs); do \ rm -f src/$$i$(EXEEXT) || exit $$?; \ - printf '#!%s --coreutils-prog=%s\n' \ + printf '#!%s --coreutils-prog-shebang=%s\n' \ $(abs_top_builddir)/src/coreutils$(EXEEXT) $$i \ >src/$$i$(EXEEXT) || exit $$?; \ chmod a+x,a-w src/$$i$(EXEEXT) || exit $$?; \ diff --git a/tests/local.mk b/tests/local.mk index 86050dc..d1e6f59 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -249,6 +249,7 @@ all_tests = \ tests/pr/pr-tests.pl \ tests/misc/pwd-option.sh \ tests/misc/chcon-fail.sh \ + tests/misc/coreutils.sh \ tests/misc/cut.pl \ tests/misc/cut-huge-range.sh \ tests/misc/wc.pl \ diff --git a/tests/misc/coreutils.sh b/tests/misc/coreutils.sh new file mode 100755 index 0000000..ad39c8b --- /dev/null +++ b/tests/misc/coreutils.sh @@ -0,0 +1,31 @@ +#!/bin/sh +# Verify behavior of separate coreutils multicall binary + +# Copyright (C) 2014 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + + +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src +print_ver_ coreutils + +test -x "$abs_top_builddir}/src/coreutils" \ + || skip_ "multicall binary is not built" + +# Yes outputs all its params so is good to verify argv manipulations +echo 'y' > exp +coreutils --coreutils-prog=yes | head -n10 | uniq > out +compare_ out exp || fail=1 + +Exit $fail -- 1.7.7.6 >From 1a7c3b46056f009624a2d1e18a7bbe70e5d67eff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Fri, 4 Jul 2014 15:19:54 +0100 Subject: [PATCH 3/4] tests: avoid false failure without a working strip * tests/install/basic-1.sh: Really avoid using ginstall strip functionality if there is an issue with the independent strip command. This avoids a false failure with --enable-single-binary=shebangs. --- tests/install/basic-1.sh | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/install/basic-1.sh b/tests/install/basic-1.sh index 9835d46..0c0d1de 100755 --- a/tests/install/basic-1.sh +++ b/tests/install/basic-1.sh @@ -46,13 +46,17 @@ is not readable, so skipping the remaining tests in this file." cp "$just_built_dd" . || fail=1 cp $dd $dd2 || fail=1 -strip $dd2 \ - || warn_ "WARNING!!! Your strip command doesn't seem to work, +strip=-s +if ! strip $dd2; then + ! test -e $abs_top_builddir/src/coreutils \ + && warn_ "WARNING!!! Your strip command doesn't seem to work, so skipping the test of install's --strip option." + strip= +fi # This test would fail with 3.16s when using versions of strip that # don't work on read-only files (the one from binutils works fine). -ginstall -s -c -m 555 $dd $dir || fail=1 +ginstall $strip -c -m 555 $dd $dir || fail=1 # Make sure the source file is still around. test -f $dd || fail=1 -- 1.7.7.6 >From 6a7418ca215f8d99107948b95ff5d3d7672d7a2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Fri, 4 Jul 2014 15:59:17 +0100 Subject: [PATCH 4/4] build: use the installed program name in --help and /proc/$PID/comm Note we still use the internal coreutils names for --coreutils-prog, independent of what's used on the system due to --prefix-prog etc. So adjust the argv appropriately so that --help and /proc/$PID/comm use the correct command name for the current install. --- src/coreutils.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/coreutils.c b/src/coreutils.c index cda7eb3..9bb3ab4 100644 --- a/src/coreutils.c +++ b/src/coreutils.c @@ -107,7 +107,7 @@ launch_program (const char *prog_name, int prog_argc, char **prog_argv) #if HAVE_PRCTL && defined PR_SET_NAME /* Not being able to set the program name is not a fatal error. */ - prctl (PR_SET_NAME, prog_name); + prctl (PR_SET_NAME, prog_argv[0]); #endif #if HAVE_PRCTL && defined PR_SET_MM_ARG_START /* Shift the beginning of the command line to prog_argv[0] (if set) so @@ -138,23 +138,28 @@ main (int argc, char **argv) if (argc >= 2) { size_t nskip = 0; + char *arg_name = NULL; /* If calling coreutils directly, the "script" name isn't passed. Distinguish the two cases with a -shebang suffix. */ if (STRPREFIX (argv[1], "--coreutils-prog=")) { nskip = 1; - prog_name = argv[1] + strlen ("--coreutils-prog="); + arg_name = prog_name = argv[1] + strlen ("--coreutils-prog="); } else if (STRPREFIX (argv[1], "--coreutils-prog-shebang=")) { nskip = 2; prog_name = argv[1] + strlen ("--coreutils-prog-shebang="); + if (argc >= 3) + arg_name = last_component (argv[2]); + else + arg_name = prog_name; } if (nskip) { - argv[nskip] = prog_name; /* XXX: Discards any specified path. */ + argv[nskip] = arg_name; /* XXX: Discards any specified path. */ launch_program (prog_name, argc - nskip, argv + nskip); } } -- 1.7.7.6
