Re: [PATCH v2 1/5] libxl: Generate golang bindings in libxl Makefile

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Jun 8, 2020, at 12:16 PM, Ian Jackson <ian.jackson@xxxxxxxxxx> 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