Discussion:
Merge getopt from gnulib to glibc and vice versa, eliminate __need_getopt
(too old to reply)
Zack Weinberg
2017-04-06 16:11:58 UTC
Permalink
Raw Message
The implementation of 'getopt' is shared between gnulib and glibc, but
they have not been kept consistent in quite some time and there has
been quite a bit of divergence. They also haven't really been
*maintained* in either project.

I set out simply to eliminate __need_getopt in both projects (see
previous discussion in threads at
https://sourceware.org/ml/libc-alpha/2017-03/msg00059.html,
https://sourceware.org/ml/libc-alpha/2017-03/msg00511.html, and
https://sourceware.org/ml/libc-alpha/2017-03/msg00512.html) but it
immediately became clear that a full bidirectional merge and quite a
few cleanups would be necessary to keep from going nuts.

Attached are a tarball of patches for glibc and a tarball of patches
for gnulib. I apologize for making reviewers unpack them, but this
seemed the best way of preventing the patch-sets from getting mixed
up. What they accomplish is:

* getopt.c, getopt1.c, and getopt_int.h are now identical in both projects.
* getopt.h has been exploded into many files. The most important
two, getopt_core.h and getopt_ext.h, are also identical in both
projects. The other two (in glibc) or four (in gnulib) are not shared
at all.
* __need_getopt is eliminated. In glibc, the replacement is #include
<bits/getopt_core.h>. In gnulib it's slightly more complicated, see
unistd.in.h.

* The code to handle _GNU_nonoption_argv_flags_ has been removed.
This was an experimental feature in bash 2.0.0, withdrawn in 2.0.1 and
never reconsidered. All of the removed code has been ifdef-ed out
since 2001.
* Error reporting in getopt.c no longer involves an #ifdef _LIBC
block for every single call to fprintf.
* There is now only one copy of the code to process long options; an
off-by-one error in that code has been corrected, and memory-handling
while reporting ambiguous abbreviated options simplified.

Most of these changes should be completely invisible to applications.

zw
Paul Eggert
2017-04-06 22:48:26 UTC
Permalink
Raw Message
Thanks for doing this, as Gnulib and Glibc should be
better-synchronized. In the interests of helping this move along, I
installed your proposed changes into Gnulib, with the attached
additional minor patches, the first of which I hope you can incorporate
into your Glibc proposal (the second patch is Gnulib-only). Perhaps
other reviewers will come up with other Gnulib and/or Glibc
improvements, but one step at a time. Three thoughts:

* Gnulib lib/getopt_cdefs.in.h, lib/getopt_core.h, lib/getopt_ext.h,
lib/getopt_pfx_core.h, and lib/getopt_pfx_ext.h now all have comments
saying "Unlike most bits headers, it does not have a protective #error,
because the guard macro for getopt.h in gnulib is not fixed." What sort
of Gnulib fix did you have in mind? Is this something we can easily fix
in Gnulib now?

* From Gnulib's point of view, perhaps it would be better to define a
module for sys/cdefs.h, and have the Gnulib getopt-posix module depend
on this new module. That way, other Gnulib code could assume a standard
GNU system which has sys/cdefs.h. Does this seem like a reasonable way
forward to you? It shouldn't affect glibc code, other than perhaps to
simplify it a bit.

* Although the Gnulib project prefers to omit leading tabs in .c and .h
files, it's OK to put tabs in files shared from glibc, at least while
we're doing this merge. I suppose Gnulib developers who prefer to avoid
tabs can expand them later, once the merging is done. Personally I would
rather let sleeping dogs lie.
Zack Weinberg
2017-04-07 12:09:01 UTC
Permalink
Raw Message
Thanks for doing this, as Gnulib and Glibc should be better-synchronized. In
the interests of helping this move along, I installed your proposed changes
into Gnulib, with the attached additional minor patches, the first of which
I hope you can incorporate into your Glibc proposal (the second patch is
Gnulib-only). Perhaps other reviewers will come up with other Gnulib and/or
Glibc improvements, but one step at a time.
Great! I'm going to take this as sufficient review to go ahead and
merge the entire patchset on the glibc side as well. I don't think
there's anyone else who knows getopt itself as well as you do, I'm
confident that any problems will be minor and addressable in follow-up
patches, and I don't want to be left with a situation where there is
*massive* divergence between gnulib and glibc here for an extended
period.

I've also pushed your follow-on change to getopt1.c.
* Gnulib lib/getopt_cdefs.in.h, lib/getopt_core.h, lib/getopt_ext.h,
lib/getopt_pfx_core.h, and lib/getopt_pfx_ext.h now all have comments saying
"Unlike most bits headers, it does not have a protective #error, because the
guard macro for getopt.h in gnulib is not fixed." What sort of Gnulib fix
did you have in mind? Is this something we can easily fix in Gnulib now?
This is just meant to reassure people reading the code in glibc that
the omission of

#ifndef _GETOPT_H
# error "Don't include bits/whatever.h directly, include getopt.h instead"
#endif

from these headers is intentional and not a bug. Maybe we could
improve the wording of the comment, but I don't think there's anything
that actually needs fixing on the gnulib side.
* From Gnulib's point of view, perhaps it would be better to define a module
for sys/cdefs.h, and have the Gnulib getopt-posix module depend on this new
module. That way, other Gnulib code could assume a standard GNU system which
has sys/cdefs.h. Does this seem like a reasonable way forward to you? It
shouldn't affect glibc code, other than perhaps to simplify it a bit.
Yes. But note that sys/cdefs.h exists in BSD as well, with an
overlapping set of definitions -- see for instance
https://github.com/freebsd/freebsd/blob/master/sys/sys/cdefs.h. It's
not a GNU invention.
* Although the Gnulib project prefers to omit leading tabs in .c and .h
files, it's OK to put tabs in files shared from glibc, at least while we're
doing this merge. I suppose Gnulib developers who prefer to avoid tabs can
expand them later, once the merging is done. Personally I would rather let
sleeping dogs lie.
glibc still adheres to the strict leading-tabs policy that I think is
still written into the GNU coding standard. I suppose there might be
a case for relaxing it for files shared with gnulib, but I am also
content to let it lie. More important, in my opinion, that 'diff -u
gnulib/lib/getopt.c glibc/posix/getopt.c' reports no changes.

zw
Bruno Haible
2017-04-18 22:14:53 UTC
Permalink
Raw Message
Hi Paul,
0002-getopt-gnu-omit-some-duplicate-code.patch
It took me some time to convince myself that this patch is correct.
Here's a test case:
$ tar xvfz hello.tar.gz (attached)
$ cd hello
$ $GNULIB_TOOL --import --lib=libposix --source-base=libp --m4-base=m4p --macro-prefix=gp getopt-posix
$ $GNULIB_TOOL --import --lib=libgnu --source-base=libg --m4-base=m4g --macro-prefix=gg getopt-gnu
$ aclocal -I m4p -I m4g
$ autoconf
$ autoheader
$ automake --copy --add-missing --force-missing

Therefore (and because the getopt-gnu module works quite differently from other
gnulib modules) let me add some comments about how it works.


2017-04-18 Bruno Haible <***@clisp.org>

getopt-gnu: Add comments.
* m4/getopt.m4 (gl_FUNC_GETOPT_GNU): Add comments.
* modules/getopt-gnu (configure.ac): Likewise.

diff --git a/m4/getopt.m4 b/m4/getopt.m4
index ac3b38e..3ebc7b7 100644
--- a/m4/getopt.m4
+++ b/m4/getopt.m4
@@ -32,7 +32,16 @@ AC_DEFUN([gl_FUNC_GETOPT_POSIX],
# getopt_long_only.
AC_DEFUN([gl_FUNC_GETOPT_GNU],
[
+ dnl Set the variable gl_getopt_required, so that all invocations of
+ dnl gl_GETOPT_CHECK_HEADERS in the scope of the current configure file
+ dnl will check for getopt with GNU extensions.
+ dnl This means that if one gnulib-tool invocation requests getopt-posix
+ dnl and another gnulib-tool invocation requests getopt-gnu, it is as if
+ dnl both had requested getopt-gnu.
m4_divert_text([INIT_PREPARE], [gl_getopt_required=GNU])
+
+ dnl No need to invoke gl_FUNC_GETOPT_POSIX here; this is automatically
+ dnl done through the module dependency getopt-gnu -> getopt-posix.
])

# Determine whether to replace the entire getopt facility.
diff --git a/modules/getopt-gnu b/modules/getopt-gnu
index 974ce14..a148693 100644
--- a/modules/getopt-gnu
+++ b/modules/getopt-gnu
@@ -10,6 +10,9 @@ getopt-posix

configure.ac:
gl_FUNC_GETOPT_GNU
+dnl Because of the way gl_FUNC_GETOPT_GNU is implemented (the gl_getopt_required
+dnl mechanism), there is no need to do any AC_LIBOBJ or AC_SUBST here; they are
+dnl done in the getopt-posix module.

Makefile.am:

Loading...