| Message ID | 20221013130513.52440-17-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Toolstack build system improvement, toward non-recursive makefiles | expand |
On 13/10/2022 14:05, Anthony Perard wrote: > diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py > index ac1cf060dd..ff4c2ad216 100644 > --- a/tools/golang/xenlight/gengotypes.py > +++ b/tools/golang/xenlight/gengotypes.py > @@ -723,7 +723,13 @@ def xenlight_golang_fmt_name(name, exported = True): > return words[0] + ''.join(x.title() for x in words[1:]) > > if __name__ == '__main__': > + if len(sys.argv) != 4: > + print("Usage: gengotypes.py <idl> <types.gen.go> <helpers.gen.go>", file=sys.stderr) This breaks with Py2.7. Needs a from __future__ import print_function inserting at the top. ~Andrew
> On 13 Oct 2022, at 16:00, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 13/10/2022 14:05, Anthony Perard wrote: >> diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py >> index ac1cf060dd..ff4c2ad216 100644 >> --- a/tools/golang/xenlight/gengotypes.py >> +++ b/tools/golang/xenlight/gengotypes.py >> @@ -723,7 +723,13 @@ def xenlight_golang_fmt_name(name, exported = True): >> return words[0] + ''.join(x.title() for x in words[1:]) >> >> if __name__ == '__main__': >> + if len(sys.argv) != 4: >> + print("Usage: gengotypes.py <idl> <types.gen.go> <helpers.gen.go>", file=sys.stderr) > > This breaks with Py2.7. Needs a > > from __future__ import print_function > > inserting at the top. Out of curiosity, did you notice this by inspection, or because you specifically tested Python 2.7, or because a system you were using is still actually using Python 2.7? -George
> On 13 Oct 2022, at 14:05, Anthony PERARD <anthony.perard@citrix.com> wrote: > > gengotypes.py creates both "types.gen.go" and "helpers.gen.go", but > make can start gengotypes.py twice. Rework the rules so that > gengotypes.py is executed only once. > > Also, add the ability to provide a path to tell gengotypes.py where to > put the files. This doesn't matter yet but it will when for example > the script will be run from tools/ to generate the targets. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > Release-acked-by: Henry Wang <Henry.Wang@arm.com> Acked-by: George Dunlap <george.dunlap@citrix.com> And you can keep the ack when you address the Python 2.7 problem. -George
On 14/10/2022 12:24, George Dunlap wrote: >> On 13 Oct 2022, at 16:00, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: >> >> On 13/10/2022 14:05, Anthony Perard wrote: >>> diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py >>> index ac1cf060dd..ff4c2ad216 100644 >>> --- a/tools/golang/xenlight/gengotypes.py >>> +++ b/tools/golang/xenlight/gengotypes.py >>> @@ -723,7 +723,13 @@ def xenlight_golang_fmt_name(name, exported = True): >>> return words[0] + ''.join(x.title() for x in words[1:]) >>> >>> if __name__ == '__main__': >>> + if len(sys.argv) != 4: >>> + print("Usage: gengotypes.py <idl> <types.gen.go> <helpers.gen.go>", file=sys.stderr) >> This breaks with Py2.7. Needs a >> >> from __future__ import print_function >> >> inserting at the top. > Out of curiosity, did you notice this by inspection, or because you specifically tested Python 2.7, or because a system you were using is still actually using Python 2.7? Xen's build system can't actually create a build which supports Py2 and Py3, because xen.lowlevel.{xc,xs} only get built once. It would be nice to fix this, but -ETUITS, so we state a specific version in the specfile and mock ensures there is no trace of the other one. XenServer is in the process of trying to retire Py2, but it turns out that Xen isn't actually fully Py3 clean yet, so we use Py2 for Xen. The build breaks because the libxl build writes the .go files even when we don't actually want go bindings in the end. ~Andrew
> On 14 Oct 2022, at 12:37, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 14/10/2022 12:24, George Dunlap wrote: >>> On 13 Oct 2022, at 16:00, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: >>> >>> On 13/10/2022 14:05, Anthony Perard wrote: >>>> diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py >>>> index ac1cf060dd..ff4c2ad216 100644 >>>> --- a/tools/golang/xenlight/gengotypes.py >>>> +++ b/tools/golang/xenlight/gengotypes.py >>>> @@ -723,7 +723,13 @@ def xenlight_golang_fmt_name(name, exported = True): >>>> return words[0] + ''.join(x.title() for x in words[1:]) >>>> >>>> if __name__ == '__main__': >>>> + if len(sys.argv) != 4: >>>> + print("Usage: gengotypes.py <idl> <types.gen.go> <helpers.gen.go>", file=sys.stderr) >>> This breaks with Py2.7. Needs a >>> >>> from __future__ import print_function >>> >>> inserting at the top. >> Out of curiosity, did you notice this by inspection, or because you specifically tested Python 2.7, or because a system you were using is still actually using Python 2.7? > > Xen's build system can't actually create a build which supports Py2 and > Py3, because xen.lowlevel.{xc,xs} only get built once. It would be nice > to fix this, but -ETUITS, so we state a specific version in the specfile > and mock ensures there is no trace of the other one. > > XenServer is in the process of trying to retire Py2, but it turns out > that Xen isn't actually fully Py3 clean yet, so we use Py2 for Xen. > > The build breaks because the libxl build writes the .go files even when > we don't actually want go bindings in the end. I think the generation code is looped in even when golang is disabled so that we can detect IDL changes during development, even on systems which don’t have golang installed. In theory if libxl.idl doesn’t change, it shouldn’t trigger the build? Alternately we could consider skipping the code generation on non-debug builds, since we only really need to detect changes during development. -George
On 14/10/2022 12:42, George Dunlap wrote:
On 14 Oct 2022, at 12:37, Andrew Cooper <Andrew.Cooper3@citrix.com<mailto:Andrew.Cooper3@citrix.com>> wrote:
On 14/10/2022 12:24, George Dunlap wrote:
On 13 Oct 2022, at 16:00, Andrew Cooper <Andrew.Cooper3@citrix.com<mailto:Andrew.Cooper3@citrix.com>> wrote:
On 13/10/2022 14:05, Anthony Perard wrote:
diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py
index ac1cf060dd..ff4c2ad216 100644
--- a/tools/golang/xenlight/gengotypes.py
+++ b/tools/golang/xenlight/gengotypes.py
@@ -723,7 +723,13 @@ def xenlight_golang_fmt_name(name, exported = True):
return words[0] + ''.join(x.title() for x in words[1:])
if __name__ == '__main__':
+ if len(sys.argv) != 4:
+ print("Usage: gengotypes.py <idl> <types.gen.go> <helpers.gen.go>", file=sys.stderr)
This breaks with Py2.7. Needs a
from __future__ import print_function
inserting at the top.
Out of curiosity, did you notice this by inspection, or because you specifically tested Python 2.7, or because a system you were using is still actually using Python 2.7?
Xen's build system can't actually create a build which supports Py2 and
Py3, because xen.lowlevel.{xc,xs} only get built once. It would be nice
to fix this, but -ETUITS, so we state a specific version in the specfile
and mock ensures there is no trace of the other one.
XenServer is in the process of trying to retire Py2, but it turns out
that Xen isn't actually fully Py3 clean yet, so we use Py2 for Xen.
The build breaks because the libxl build writes the .go files even when
we don't actually want go bindings in the end.
I think the generation code is looped in even when golang is disabled so that we can detect IDL changes during development, even on systems which don’t have golang installed. In theory if libxl.idl doesn’t change, it shouldn’t trigger the build? Alternately we could consider skipping the code generation on non-debug builds, since we only really need to detect changes during development.
This is a "clean" mock build, from a git archive'd tarball. Any logic like this is going to trigger unconditionally.
It's not a problem IMO - writing out one extra text file is not the slow bit of a Xen build.
~Andrew
diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile index 00e6d17f2b..c5bb6b94a8 100644 --- a/tools/golang/xenlight/Makefile +++ b/tools/golang/xenlight/Makefile @@ -15,8 +15,10 @@ all: build GOXL_GEN_FILES = types.gen.go helpers.gen.go -%.gen.go: gengotypes.py $(LIBXL_SRC_DIR)/libxl_types.idl $(LIBXL_SRC_DIR)/idl.py - LIBXL_SRC_DIR=$(LIBXL_SRC_DIR) $(PYTHON) gengotypes.py $(LIBXL_SRC_DIR)/libxl_types.idl +# This exploits the 'multi-target pattern rule' trick. +# gentypes.py should be executed only once to make all the targets. +$(subst .gen.,.%.,$(GOXL_GEN_FILES)): gengotypes.py $(LIBXL_SRC_DIR)/libxl_types.idl $(LIBXL_SRC_DIR)/idl.py + LIBXL_SRC_DIR=$(LIBXL_SRC_DIR) $(PYTHON) gengotypes.py $(LIBXL_SRC_DIR)/libxl_types.idl $(@D)/types.gen.go $(@D)/helpers.gen.go # Go will do its own dependency checking, and not actuall go through # with the build if none of the input files have changed. diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py index ac1cf060dd..ff4c2ad216 100644 --- a/tools/golang/xenlight/gengotypes.py +++ b/tools/golang/xenlight/gengotypes.py @@ -723,7 +723,13 @@ def xenlight_golang_fmt_name(name, exported = True): return words[0] + ''.join(x.title() for x in words[1:]) if __name__ == '__main__': + if len(sys.argv) != 4: + print("Usage: gengotypes.py <idl> <types.gen.go> <helpers.gen.go>", file=sys.stderr) + sys.exit(1) + idlname = sys.argv[1] + path_types = sys.argv[2] + path_helpers = sys.argv[3] (builtins, types) = idl.parse(idlname) @@ -735,9 +741,11 @@ if __name__ == '__main__': // source: {} """.format(os.path.basename(sys.argv[0]), - ' '.join([os.path.basename(a) for a in sys.argv[1:]])) + os.path.basename(sys.argv[1])) xenlight_golang_generate_types(types=types, + path=path_types, comment=header_comment) xenlight_golang_generate_helpers(types=types, + path=path_helpers, comment=header_comment)
gengotypes.py creates both "types.gen.go" and "helpers.gen.go", but make can start gengotypes.py twice. Rework the rules so that gengotypes.py is executed only once. Also, add the ability to provide a path to tell gengotypes.py where to put the files. This doesn't matter yet but it will when for example the script will be run from tools/ to generate the targets. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> Release-acked-by: Henry Wang <Henry.Wang@arm.com> --- Notes: v5: - released-acked to fix occasional CI build issue v4: - new patch tools/golang/xenlight/Makefile | 6 ++++-- tools/golang/xenlight/gengotypes.py | 10 +++++++++- 2 files changed, 13 insertions(+), 3 deletions(-)