> On Jun 8, 2020, at 12:16 PM, Ian Jackson <ian.jack...@citrix.com> wrote:
>
> George Dunlap writes ("Re: [PATCH v2 1/5] libxl: Generate golang bindings in
> libxl Makefile"):
>> So as written this turns out not to work correctly: `gengotypes.py` spits
>> out syntactically valid but unformatted Go code, and then runs `go fmt` on
>> it to get the right style (including tabs, &c). But the error code of `go
>> fmt` isn’t checked; so on a system without go installed, if the build system
>> decides it needs to re-run `gengotypes.py` for whatever reason, the build
>> succeeds but the tree ends up “dirtied” with an unformatted version fo the
>> generated text.
>
> And `go fmt' overwrites its input file ?
Yes.
> The lost error is due to using `os.system' which does not raise an
> exception. The python 3 docs for `os.system' say
> | The subprocess module provides more powerful facilities for
> | spawning new processes and retrieving their results; using that
> | module is preferable to using this function. See the Replacing
> | Older Functions with the subprocess Module section in the
> | subprocess documentation for some helpful recipes.
> And the recipe suggests
> | sts = os.system("mycmd" + " myarg")
> | # becomes
> | sts = call("mycmd" + " myarg", shell=True)
> | Notes:
> | * Calling the program through the shell is usually not required.
>
> This is not particularly helpful advice because subprocess.call, like
> os.system, does not raise an exception when things go wrong. But it
> does have a "more realistic example" immediately afterwards which does
> actually check the error code.
>
> You wanted subprocess.check_call. IDK which Python versions have
> subprocess.check_call.
Since the golang code generation recipe is now called from libxl/Makefile
unconditionally, the effect of “fixing” the `go fmt` call in gengotypes.py to
fail if `go fmt` is not available will be to make golang a required dependency
for building the tools. I think it’s rather late in the day to be discussing
that for 4.14.
Nick has already submitted a patch which simply removes the `go fmt` call,
leaving the generated code looking very “generated”. That will do for this
release.
-George
