* [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` @ 2016-08-04 5:54 Kylie McClain 2016-08-04 5:54 ` [PATCH 2/2] histedit: Remove non-glibc fallback code Kylie McClain 2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk 0 siblings, 2 replies; 9+ messages in thread From: Kylie McClain @ 2016-08-04 5:54 UTC (permalink / raw) To: dash; +Cc: Kylie McClain From: Kylie McClain <somasis@exherbo.org> nl, while specified in POSIX, is rather obscure and isn't provided by small coreutils implementations such as `busybox`. This while loop works just as well for our purposes. --- src/mkbuiltins | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mkbuiltins b/src/mkbuiltins index b4d6f4e..a47bce8 100644 --- a/src/mkbuiltins +++ b/src/mkbuiltins @@ -101,7 +101,8 @@ cat <<\! */ ! -sed 's/ -[a-z]*//' $temp2 | nl -ba -v0 | +sed 's/ -[a-z]*//' $temp2 | while read line;do \ + i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done | LC_ALL= LC_COLLATE=C sort -u -k 3,3 | tr abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ | awk '{ printf "#define %s (builtincmd + %d)\n", $3, $1}' -- 2.9.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] histedit: Remove non-glibc fallback code 2016-08-04 5:54 [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Kylie McClain @ 2016-08-04 5:54 ` Kylie McClain 2016-08-04 15:59 ` Jilles Tjoelker 2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk 1 sibling, 1 reply; 9+ messages in thread From: Kylie McClain @ 2016-08-04 5:54 UTC (permalink / raw) To: dash; +Cc: Kylie McClain From: Kylie McClain <somasis@exherbo.org> This fallback code seems to go back the import of the repository back in 2005, it fails on musl libc, and there aren't any comments explaining why this difference is needed. Regardless, any compatibility ifdefs should probably be checking macros defined on systems that need a different code path, rather than just having fallback code for non-glibc. --- src/histedit.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/histedit.c b/src/histedit.c index 94465d7..4498cf4 100644 --- a/src/histedit.c +++ b/src/histedit.c @@ -214,11 +214,7 @@ histcmd(int argc, char **argv) if (argc == 1) sh_error("missing history argument"); -#ifdef __GLIBC__ optind = 0; -#else - optreset = 1; optind = 1; /* initialize getopt */ -#endif while (not_fcnumber(argv[optind]) && (ch = getopt(argc, argv, ":e:lnrs")) != -1) switch ((char)ch) { -- 2.9.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] histedit: Remove non-glibc fallback code 2016-08-04 5:54 ` [PATCH 2/2] histedit: Remove non-glibc fallback code Kylie McClain @ 2016-08-04 15:59 ` Jilles Tjoelker 2016-08-08 15:50 ` Jilles Tjoelker 0 siblings, 1 reply; 9+ messages in thread From: Jilles Tjoelker @ 2016-08-04 15:59 UTC (permalink / raw) To: Kylie McClain; +Cc: dash, Kylie McClain On Thu, Aug 04, 2016 at 01:54:11AM -0400, Kylie McClain wrote: > From: Kylie McClain <somasis@exherbo.org> > This fallback code seems to go back the import of the repository back > in 2005, it fails on musl libc, and there aren't any comments > explaining why this difference is needed. Regardless, any > compatibility ifdefs should probably be checking macros defined on > systems that need a different code path, rather than just having > fallback code for non-glibc. Setting optreset is required on FreeBSD and NetBSD which do not recognize setting optind to 0. POSIX specifies neither of these methods to reset getopt(). However, removing the dependency on this feature is simple. The shell already features a function nextopt() that can be used to parse options and is automatically set up for builtins. From 23da600dcff616662a93f307420d9142598e2cae Mon Sep 17 00:00:00 2001 From: Jilles Tjoelker <jilles@stack.nl> Date: Thu, 4 Aug 2016 17:51:12 +0200 Subject: [PATCH 1/2] [HISTEDIT] Stop depending on getopt reset feature. Instead, use our own nextopt() function. --- src/histedit.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/histedit.c b/src/histedit.c index 94465d7..ec45065 100644 --- a/src/histedit.c +++ b/src/histedit.c @@ -214,13 +214,8 @@ histcmd(int argc, char **argv) if (argc == 1) sh_error("missing history argument"); -#ifdef __GLIBC__ - optind = 0; -#else - optreset = 1; optind = 1; /* initialize getopt */ -#endif while (not_fcnumber(argv[optind]) && - (ch = getopt(argc, argv, ":e:lnrs")) != -1) + (ch = nextopt(":e:lnrs")) != '\0') switch ((char)ch) { case 'e': editor = optionarg; -- 2.9.2 -- Jilles Tjoelker ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] histedit: Remove non-glibc fallback code 2016-08-04 15:59 ` Jilles Tjoelker @ 2016-08-08 15:50 ` Jilles Tjoelker 0 siblings, 0 replies; 9+ messages in thread From: Jilles Tjoelker @ 2016-08-08 15:50 UTC (permalink / raw) To: Kylie McClain; +Cc: dash, Kylie McClain On Thu, Aug 04, 2016 at 05:59:08PM +0200, Jilles Tjoelker wrote: > >From 23da600dcff616662a93f307420d9142598e2cae Mon Sep 17 00:00:00 2001 > From: Jilles Tjoelker <jilles@stack.nl> > Date: Thu, 4 Aug 2016 17:51:12 +0200 > Subject: [PATCH 1/2] [HISTEDIT] Stop depending on getopt reset feature. > Instead, use our own nextopt() function. > --- > src/histedit.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/src/histedit.c b/src/histedit.c > index 94465d7..ec45065 100644 > --- a/src/histedit.c > +++ b/src/histedit.c > @@ -214,13 +214,8 @@ histcmd(int argc, char **argv) > if (argc == 1) > sh_error("missing history argument"); > > -#ifdef __GLIBC__ > - optind = 0; > -#else > - optreset = 1; optind = 1; /* initialize getopt */ > -#endif > while (not_fcnumber(argv[optind]) && > - (ch = getopt(argc, argv, ":e:lnrs")) != -1) > + (ch = nextopt(":e:lnrs")) != '\0') > switch ((char)ch) { > case 'e': > editor = optionarg; This is clearly wrong; not_fcnumber() should be passed *argptr instead of something bogus depending on optind. The fixed version is what FreeBSD sh has as of SVN r240541 but I have not tested it in dash. In any case, a side effect of this change is a small code size reduction. -- Jilles Tjoelker ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` 2016-08-04 5:54 [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Kylie McClain 2016-08-04 5:54 ` [PATCH 2/2] histedit: Remove non-glibc fallback code Kylie McClain @ 2016-08-04 17:17 ` Harald van Dijk 2016-08-06 9:02 ` Seb ` (2 more replies) 1 sibling, 3 replies; 9+ messages in thread From: Harald van Dijk @ 2016-08-04 17:17 UTC (permalink / raw) To: Kylie McClain, dash; +Cc: Kylie McClain On 04/08/2016 07:54, Kylie McClain wrote: > From: Kylie McClain <somasis@exherbo.org> > > nl, while specified in POSIX, is rather obscure and isn't provided by small > coreutils implementations such as `busybox`. This while loop works just as > well for our purposes. ... > -sed 's/ -[a-z]*//' $temp2 | nl -ba -v0 | > +sed 's/ -[a-z]*//' $temp2 | while read line;do \ > + i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done | $(( ... )) is mentioned in <https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Shell-Substitutions.html> as not universally supported, notably Solaris 10 /bin/sh does not have it. Given that dash was fairly recently changed to make it build on Solaris 9, it seems like a mistake to break that again. Aside from that, i is such a common variable name that it seems risky to assume it is unset. I know I've set it myself in shell sessions that I ended up using for building dash. I never exported it, so it wouldn't break here, but it doesn't seem like a stretch that someone else does export it. Cheers, Harald van Dijk ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` 2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk @ 2016-08-06 9:02 ` Seb 2016-08-07 10:17 ` Seb 2016-08-06 16:51 ` Harald van Dijk 2016-08-08 15:33 ` Herbert Xu 2 siblings, 1 reply; 9+ messages in thread From: Seb @ 2016-08-06 9:02 UTC (permalink / raw) To: dash; +Cc: Harald van Dijk [-- Attachment #1: Type: text/plain, Size: 645 bytes --] On Thu, Aug 04, 2016 at 07:17:47PM +0200, Harald van Dijk wrote: > Given that dash was fairly recently changed to make it build on > Solaris 9, it seems like a mistake to break that again. Hello, This is an attempt to simplify the current implementation. This one does not require any temporary file anymore and relies only on sort and (a rather basic usage of) awk. I've checked the the Opensolaris' awk manual, trying to not introduce some unsupported syntax. I still wonder about the escaped newlines and the (currently commented) close() statements, but both can be easily addressed. So, if someone thinks it's worth testing... ++ Seb. [-- Attachment #2: mkbuiltins --] [-- Type: text/plain, Size: 2382 bytes --] #!/bin/sh LC_ALL=C # Force the collate order of the builtins. export LC_ALL # some shells may not support the "export FOO=bar" form. awk ' (NF && ($1 !~ /^#/)) { # command [options] alias1 [[options] alias2] ... for (i = 2; i <= NF; i++) { mask = 0 cmd = $1 if ($i ~ /^-/) { if ($i ~ /n/) cmd = "NULL" if ($i ~ /s/) mask += 1 if ($i ~ /[su]/) mask += 2 if ($i ~ /a/) mask += 4 i++ } print $i, cmd, mask, $1 } }' $1 | sort -k1,1 | awk ' BEGIN { BUILTINS_H = "./builtins.h" BUILTINS_C = "./builtins.c" warn = "/*\n * This file was generated by the mkbuiltins program.\n */\n" print warn >BUILTINS_H print warn "\n#include \"shell.h\"" \ "\n#include \"builtins.h\"\n" >BUILTINS_C } (!($NF in DEFINE)) { up = $NF # /bin/awk has no toupper() on Solaris. gsub(/a/, "A", up); gsub(/j/, "J", up); gsub(/s/, "S", up) gsub(/b/, "B", up); gsub(/k/, "K", up); gsub(/t/, "T", up) gsub(/c/, "C", up); gsub(/l/, "L", up); gsub(/u/, "U", up) gsub(/d/, "D", up); gsub(/m/, "M", up); gsub(/v/, "V", up) gsub(/e/, "E", up); gsub(/n/, "N", up); gsub(/w/, "W", up) gsub(/f/, "F", up); gsub(/o/, "O", up); gsub(/x/, "X", up) gsub(/g/, "G", up); gsub(/p/, "P", up); gsub(/y/, "Y", up) gsub(/h/, "H", up); gsub(/q/, "Q", up); gsub(/z/, "Z", up) gsub(/i/, "I", up); gsub(/r/, "R", up) print "#define "up" (builtincmd + "(NR-1)")" >BUILTINS_H print "int "$NF"(int, char **);" >BUILTINS_C DEFINE[$NF] } { CMD[NR] = "\""$1"\", "$2", "$3 } END { print "\n#define NUMBUILTINS "NR"\n" \ "\n#define BUILTIN_SPECIAL 0x1" \ "\n#define BUILTIN_REGULAR 0x2" \ "\n#define BUILTIN_ASSIGN 0x4\n" \ "\nstruct builtincmd {" \ "\n const char *name;" \ "\n int (*builtin)(int, char **);" \ "\n unsigned flags;" \ "\n};" \ "\n\nextern const struct builtincmd builtincmd[];" >BUILTINS_H # close(BUILTINS_H) # not supported on Solaris ? print "\nconst struct builtincmd builtincmd[] = {" >BUILTINS_C for (i = 1; i <= NR; i++) print "\t{ "CMD[i]" }," >BUILTINS_C print "};" >BUILTINS_C # close(BUILTINS_C) }' # EoF ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` 2016-08-06 9:02 ` Seb @ 2016-08-07 10:17 ` Seb 0 siblings, 0 replies; 9+ messages in thread From: Seb @ 2016-08-07 10:17 UTC (permalink / raw) To: dash On Sat, Aug 06, 2016 at 11:02:42AM +0200, Seb wrote: > This is an attempt to simplify the current implementation [...] BTW, I wonder if there is not a (virtual) bug with the current mkbuiltins. Look at the TRUECMD macro in the generated builtins.h: #define TRUECMD (builtincmd + 1) It doesn't point on "true" (mask=2, "special builtin") but on ":" (mask=3, "standard builtin"), whatever the order of the aliases you set in builtins.def.in. Maybe it's a mere problem with the macro name ("TRUECMD" should rather be "COLONCMD"), but I don't know if it is really an intended behaviour as the result is finally a matter of sorting. ++ Seb. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` 2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk 2016-08-06 9:02 ` Seb @ 2016-08-06 16:51 ` Harald van Dijk 2016-08-08 15:33 ` Herbert Xu 2 siblings, 0 replies; 9+ messages in thread From: Harald van Dijk @ 2016-08-06 16:51 UTC (permalink / raw) To: Kylie McClain, dash; +Cc: Kylie McClain On 04/08/2016 19:17, Harald van Dijk wrote: > On 04/08/2016 07:54, Kylie McClain wrote: >> From: Kylie McClain <somasis@exherbo.org> >> >> nl, while specified in POSIX, is rather obscure and isn't provided by >> small >> coreutils implementations such as `busybox`. This while loop works >> just as >> well for our purposes. > ... >> -sed 's/ -[a-z]*//' $temp2 | nl -ba -v0 | >> +sed 's/ -[a-z]*//' $temp2 | while read line;do \ >> + i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done | > > $(( ... )) is mentioned in > <https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Shell-Substitutions.html> > as not universally supported, notably Solaris 10 /bin/sh does not have > it. Given that dash was fairly recently changed to make it build on > Solaris 9, it seems like a mistake to break that again. I just realised that that particular change to make it build on Solaris 9 involved avoiding running mkbuiltins using sh, choosing to use $SHELL instead, as $SHELL should already have been picked by configure as something more modern. So the use of $(( ... )) should probably not be a problem after all... > Aside from that, i is such a common variable name that it seems risky to > assume it is unset. I know I've set it myself in shell sessions that I > ended up using for building dash. I never exported it, so it wouldn't > break here, but it doesn't seem like a stretch that someone else does > export it. ...as long as i is simply initialised before the loop. Cheers, Harald van Dijk ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` 2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk 2016-08-06 9:02 ` Seb 2016-08-06 16:51 ` Harald van Dijk @ 2016-08-08 15:33 ` Herbert Xu 2 siblings, 0 replies; 9+ messages in thread From: Herbert Xu @ 2016-08-08 15:33 UTC (permalink / raw) To: Harald van Dijk; +Cc: somasissounds, dash, somasis Harald van Dijk <harald@gigawatt.nl> wrote: > On 04/08/2016 07:54, Kylie McClain wrote: >> From: Kylie McClain <somasis@exherbo.org> >> >> nl, while specified in POSIX, is rather obscure and isn't provided by small >> coreutils implementations such as `busybox`. This while loop works just as >> well for our purposes. > ... >> -sed 's/ -[a-z]*//' $temp2 | nl -ba -v0 | >> +sed 's/ -[a-z]*//' $temp2 | while read line;do \ >> + i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done | > > $(( ... )) is mentioned in > <https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Shell-Substitutions.html> > as not universally supported, notably Solaris 10 /bin/sh does not have > it. Given that dash was fairly recently changed to make it build on > Solaris 9, it seems like a mistake to break that again. > > Aside from that, i is such a common variable name that it seems risky to > assume it is unset. I know I've set it myself in shell sessions that I > ended up using for building dash. I never exported it, so it wouldn't > break here, but it doesn't seem like a stretch that someone else does > export it. nl has been around forever. I'm not taking this patch. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-08-08 15:50 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-04 5:54 [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Kylie McClain 2016-08-04 5:54 ` [PATCH 2/2] histedit: Remove non-glibc fallback code Kylie McClain 2016-08-04 15:59 ` Jilles Tjoelker 2016-08-08 15:50 ` Jilles Tjoelker 2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk 2016-08-06 9:02 ` Seb 2016-08-07 10:17 ` Seb 2016-08-06 16:51 ` Harald van Dijk 2016-08-08 15:33 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).