Re: gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
On 4/28/24 2:52 AM, Bruno Haible wrote: > Thanks! OK to push in 1 or 2 days. Sounds good. >> Though, the correct way to fix this would be making instance variables >> local when they are only used in one function. > > I agree that this kind of doing side effects on a GLConfig object that > is already held by another object is not super maintainable. Yeah, there are many things I dislike about that class. I'll probably have a look at *slowly* cleaning it up eventually. There is a bunch of dead code in there if I remember correctly. > However, this is not the only call to setLibTests. There is another one > at GLTestDir.py:392. And there is a call to setAuxDir in GLTestDir.py:401. > It seems that a conversion to use instance variables or local variables > would be quite intrusive. I'm not sure this refactoring would be worth it. Good point. I'll probably have to double check those too. Collin
Re: gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
Hi Collin,
> > The 'libtests' value read from the config in self.emitter is
> > incorrect.
>
> This patch seems to support what I was saying here.
>
> diff --git a/pygnulib/GLTestDir.py b/pygnulib/GLTestDir.py
> index 002eb30267..11b067e085 100644
> --- a/pygnulib/GLTestDir.py
> +++ b/pygnulib/GLTestDir.py
> @@ -244,8 +244,7 @@ def execute(self) -> None:
> if file.startswith('lib/'):
> libtests = True
> break
> -if libtests:
> -self.config.setLibtests(True)
> +self.emitter.config.setLibtests(libtests)
>
> if single_configure:
> # Add the dummy module to the main module list if needed.
Thanks! OK to push in 1 or 2 days.
> Though, the correct way to fix this would be making instance variables
> local when they are only used in one function.
I agree that this kind of doing side effects on a GLConfig object that
is already held by another object is not super maintainable.
However, this is not the only call to setLibTests. There is another one
at GLTestDir.py:392. And there is a call to setAuxDir in GLTestDir.py:401.
It seems that a conversion to use instance variables or local variables
would be quite intrusive. I'm not sure this refactoring would be worth it.
Bruno
Re: gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
Hi Bruno,
On 4/27/24 5:29 PM, Collin Funk wrote:
> I'm thinking that this is due to GLTestDir creating a GLEmiter in
> __init__() like this:
[...]
> The 'libtests' value read from the config in self.emitter is
> incorrect.
This patch seems to support what I was saying here.
diff --git a/pygnulib/GLTestDir.py b/pygnulib/GLTestDir.py
index 002eb30267..11b067e085 100644
--- a/pygnulib/GLTestDir.py
+++ b/pygnulib/GLTestDir.py
@@ -244,8 +244,7 @@ def execute(self) -> None:
if file.startswith('lib/'):
libtests = True
break
-if libtests:
-self.config.setLibtests(True)
+self.emitter.config.setLibtests(libtests)
if single_configure:
# Add the dummy module to the main module list if needed.
Though, the correct way to fix this would be making instance variables
local when they are only used in one function.
I was having a look at doing that when I submitted this patch:
https://lists.gnu.org/archive/html/bug-gnulib/2024-04/msg00444.html
Since I also noticed some in GLImport. Since we don't want to
introduce any major changes for a bit after your announcement, should
we just leave this for a few days?
Thanks.
Collin
Re: gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
On 4/27/24 4:06 PM, Bruno Haible wrote:
> The remaining problem now is:
>
> $ rm -rf ../testdir3; GNULIB_TOOL_IMPL=sh+py ./gnulib-tool
> --create-megatestdir --dir=../testdir3 --single-configure stdio
> sys_types
> ./gnulib-tool: *** gnulib-tool.py produced different files than
> gnulib-tool.sh! Compare ../testdir3 and ../testdir3-glpy3133646.
> ./gnulib-tool: *** Stop.
Thanks for figuring out the test case for me.
I'm thinking that this is due to GLTestDir creating a GLEmiter in
__init__() like this:
self.emitter = GLEmiter(self.config)
then in GLTestDir.execute():
# Determine whether a $testsbase/libtests.a is needed.
libtests = False
for module in tests_modules:
files = module.getFiles()
for file in files:
if file.startswith('lib/'):
libtests = True
break
if libtests:
self.config.setLibtests(True)
The emiter has no clue that libtests is changed. So following those
lines when we print the configure.ac:
if single_configure:
# Create $testsbase/Makefile.am.
destfile = joinpath(directory, 'Makefile.am')
witness_macro = '%stests_WITNESS' % macro_prefix
emit = self.emitter.tests_Makefile_am(destfile, tests_modules,
moduletable,
self.makefiletable,
witness_macro, for_test)
The 'libtests' value read from the config in self.emitter is
incorrect.
That's what it seems like at least. Going to eat dinner then work on a
patch. Hopefully my thoughts are correct...
Collin
Re: gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
Hi Collin, > > GNULIB_TOOL_IMPL=sh+py ./gnulib-tool --create-megatestdir > > --dir=testdir2 --single-configure sys_types stdio > > > > Can you double check on your machine? > > I confirm, it passes for me as well now. The remaining problem now is: $ rm -rf ../testdir3; GNULIB_TOOL_IMPL=sh+py ./gnulib-tool --create-megatestdir --dir=../testdir3 --single-configure stdio sys_types ./gnulib-tool: *** gnulib-tool.py produced different files than gnulib-tool.sh! Compare ../testdir3 and ../testdir3-glpy3133646. ./gnulib-tool: *** Stop. $ diff -r -q ../testdir3 ../testdir3-glpy3133646 Files ../testdir3/sys_types/gltests/Makefile.am and ../testdir3-glpy3133646/sys_types/gltests/Makefile.am differ Files ../testdir3/sys_types/gltests/Makefile.in and ../testdir3-glpy3133646/sys_types/gltests/Makefile.in differ $ diff -u ../testdir3/sys_types/gltests/Makefile.am ../testdir3-glpy3133646/sys_types/gltests/Makefile.am --- ../testdir3/sys_types/gltests/Makefile.am 2024-04-28 00:59:01.911451722 +0200 +++ ../testdir3-glpy3133646/sys_types/gltests/Makefile.am 2024-04-28 00:58:50.951373470 +0200 @@ -32,6 +32,7 @@ EXTRA_PROGRAMS = noinst_HEADERS = noinst_LIBRARIES = +noinst_LIBRARIES += libtests.a pkgdata_DATA = EXTRA_DIST = BUILT_SOURCES = @@ -52,7 +53,13 @@ -I.. -I$(srcdir)/.. \ -I../gllib -I$(srcdir)/../gllib -LDADD = ../gllib/libgnu.a +LDADD = libtests.a ../gllib/libgnu.a libtests.a ../gllib/libgnu.a libtests.a $(LIBTESTS_LIBDEPS) + +libtests_a_SOURCES = +libtests_a_LIBADD = $(gltests_LIBOBJS) +libtests_a_DEPENDENCIES = $(gltests_LIBOBJS) +EXTRA_libtests_a_SOURCES = +AM_LIBTOOLFLAGS = --preserve-dup-deps TESTS_ENVIRONMENT += EXEEXT='@EXEEXT@' srcdir='$(srcdir)'
Re: gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
On 4/27/24 3:22 PM, Bruno Haible wrote: > Thanks. There's a nit, though: These lines do not remove duplicates. > > module_set = set(modules) > modules = [ module > for module in modules > if module in module_set ] Oops... Today isn't my day I guess. I think the least annoying way to remove duplicates while preserving order is this trick I mentioned a few months ago: print(list(dict.fromkeys([5, 1, 3, 100, 3, 3, 5, 5, 5]))) [5, 1, 3, 100] This works since we require Python 3.7 [1]: Changed in version 3.7: Dictionary order is guaranteed to be insertion order. This behavior was an implementation detail of CPython from 3.6. There is also collections.OrderedDict for previous versions though. > I confirm, it passes for me as well now. > > But I'm confused: I thought part of the problem was that a libtests was being > generated. Putting the modules in a different order makes the libtests > disappear?? What is going on? Likewise. The sorting of gnulib-tool.py was still incorrect, so that commit was needed minus my silly set mistake. Sounds like it is time for me to learn how the Python debuggers work and step through it. :) [1] https://docs.python.org/3/library/stdtypes.html#mapping-types-dict Collin
Re: gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
Hi Collin, > I've pushed the attached patch Thanks. There's a nit, though: These lines do not remove duplicates. module_set = set(modules) modules = [ module for module in modules if module in module_set ] See: >>> modules = ['a', 'b', 'a', 'c', 'b'] >>> module_set = set(modules) >>> modules = [ module for module in modules if module in module_set ] >>> modules ['a', 'b', 'a', 'c', 'b'] > since it allows me to run this and pass: > > GNULIB_TOOL_IMPL=sh+py ./gnulib-tool --create-megatestdir --dir=testdir2 > --single-configure sys_types stdio > > Can you double check on your machine? I confirm, it passes for me as well now. But I'm confused: I thought part of the problem was that a libtests was being generated. Putting the modules in a different order makes the libtests disappear?? What is going on? Bruno
Re: gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
On 4/27/24 2:29 PM, Bruno Haible wrote: > Also, the SUBDIRS variable in a Makefile.am determines the order in which > the subdirectories are traversed during a build. If a subdirectory has some > chances to fail the build or the tests, it should be mentioned last, so > that the other (more reliable) subdirectories can at least be built and > tested. Thus, sorting SUBDIRS in alphabetical order is generally unwelcome. > > It is gnulib-tool.py which needs to adapt. Thanks for the explanation. I agree, I overlooked the purpose of SUBDIRS in the original message. I've pushed the attached patch since it allows me to run this and pass: GNULIB_TOOL_IMPL=sh+py ./gnulib-tool --create-megatestdir --dir=testdir2 --single-configure sys_types stdio Can you double check on your machine? CollinFrom dba810f623ad02476401faddccfdcaf234db7b5e Mon Sep 17 00:00:00 2001 From: Collin Funk Date: Sat, 27 Apr 2024 15:01:24 -0700 Subject: [PATCH] gnulib-tool.py: Preserve module ordering in --create-megatestdir. * pygnulib/GLTestDir.py (GLMegaTestDir.execute): Use a separate set to remove duplicates from the original list without sorting. --- ChangeLog | 6 ++ pygnulib/GLTestDir.py | 7 ++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 935ddbd1ba..34893ebff1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2024-04-27 Collin Funk + + gnulib-tool.py: Preserve module ordering in --create-megatestdir. + * pygnulib/GLTestDir.py (GLMegaTestDir.execute): Use a separate set to + remove duplicates from the original list without sorting. + 2024-04-27 Bruno Haible fcntl-h, stdio, unistd: Ensure off64_t is defined on all platforms. diff --git a/pygnulib/GLTestDir.py b/pygnulib/GLTestDir.py index 758a65168e..002eb30267 100644 --- a/pygnulib/GLTestDir.py +++ b/pygnulib/GLTestDir.py @@ -876,7 +876,12 @@ def execute(self) -> None: modules = self.modulesystem.list() modules = [ self.modulesystem.find(m) for m in modules ] -modules = sorted(set(modules)) +# Preserve ordering from the command-line, but remove duplicates. +# This allows control over the SUBDIRS variable in the top-level Makefile.am. +module_set = set(modules) +modules = [ module +for module in modules +if module in module_set ] # First, all modules one by one. for module in modules: -- 2.44.0
Re: gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
Hi Collin, > diff -ru testdir2/Makefile.am testdir2-glpy85706/Makefile.am > --- testdir2/Makefile.am 2024-04-27 13:53:51.443692945 -0700 > +++ testdir2-glpy85706/Makefile.am2024-04-27 13:53:33.063660555 -0700 > @@ -2,6 +2,6 @@ > > AUTOMAKE_OPTIONS = 1.14 foreign > > -SUBDIRS = sys_types stdio ALL > +SUBDIRS = stdio sys_types ALL Indeed, this part of the failure is a question of sorting. > The behavior of gnulib-tool.py is to sort the modules and > gnulib-tool.sh uses them as passed. > > I feel like the best solution here is just to sort the modules in > gnulib-tool.sh. I disagree. The route that we have been following over the last two months — and that you can verify through 'gitk gnulib-tool.sh' — is that we change gnulib-tool.sh only when it is clearly a bug fix or an improvement. Adding sorting is neither a bug fix nor an improvement. It is arbitrary. Also, the SUBDIRS variable in a Makefile.am determines the order in which the subdirectories are traversed during a build. If a subdirectory has some chances to fail the build or the tests, it should be mentioned last, so that the other (more reliable) subdirectories can at least be built and tested. Thus, sorting SUBDIRS in alphabetical order is generally unwelcome. It is gnulib-tool.py which needs to adapt. Bruno
gnulib-tool.sh: Match sorting of gnulib-tool.py in output.
Hi Bruno, When looking at your bug report I noticed a few lines of the diff are just different orderings. Running: GNULIB_TOOL_IMPL=sh+py ./gnulib-tool --create-megatestdir --dir=testdir2 --single-configure sys_types stdio We see in the diff: diff -ru testdir2/Makefile.am testdir2-glpy85706/Makefile.am --- testdir2/Makefile.am2024-04-27 13:53:51.443692945 -0700 +++ testdir2-glpy85706/Makefile.am 2024-04-27 13:53:33.063660555 -0700 @@ -2,6 +2,6 @@ AUTOMAKE_OPTIONS = 1.14 foreign -SUBDIRS = sys_types stdio ALL +SUBDIRS = stdio sys_types ALL This can also be seen in configure.ac and the generated configure script. The behavior of gnulib-tool.py is to sort the modules and gnulib-tool.sh uses them as passed. I feel like the best solution here is just to sort the modules in gnulib-tool.sh. Since gnulib-tool.py turns the strings into a set of GLModules and then sorts them. Can I apply the attached patch? Asking since I feel less comfortable making changes outside of pygnulib/* and I'm not sure if/how to apply them to stable branches. CollinFrom c6877b8f0fcd34a96f8874199eafae14b3241d77 Mon Sep 17 00:00:00 2001 From: Collin Funk Date: Sat, 27 Apr 2024 14:03:27 -0700 Subject: [PATCH] gnulib-tool.sh: Match sorting of gnulib-tool.py in output. * gnulib-tool.sh (func_create_megatestdir): Sort the modules so AC_CONFIG_SUBDIRS in configure.ac and SUBDIRS in Makefile.am are also sorted. --- ChangeLog | 7 +++ gnulib-tool.sh | 1 + 2 files changed, 8 insertions(+) diff --git a/ChangeLog b/ChangeLog index 935ddbd1ba..61518a200e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2024-04-27 Collin Funk + + gnulib-tool.sh: Match sorting of gnulib-tool.py in output. + * gnulib-tool.sh (func_create_megatestdir): Sort the modules so + AC_CONFIG_SUBDIRS in configure.ac and SUBDIRS in Makefile.am are also + sorted. + 2024-04-27 Bruno Haible fcntl-h, stdio, unistd: Ensure off64_t is defined on all platforms. diff --git a/gnulib-tool.sh b/gnulib-tool.sh index b486e99b1e..13a9bdf535 100755 --- a/gnulib-tool.sh +++ b/gnulib-tool.sh @@ -7135,6 +7135,7 @@ func_create_megatestdir () if test -z "$allmodules"; then allmodules=`func_all_modules` fi + allmodules=`for module in $allmodules; do echo $module; done | sort -u` megasubdirs= # First, all modules one by one. -- 2.44.0
