libxl: avoid golang building without CONFIG_GOLANG=y
diff mbox series

Message ID e8dd70a7-bdde-e12a-3f4d-f52e58016234@suse.com
State New
Headers show
Series
  • libxl: avoid golang building without CONFIG_GOLANG=y
Related show

Commit Message

Jan Beulich Aug. 3, 2020, 8:06 a.m. UTC
While this doesn't address the real problem I've run into (attempting to
update r/o source files), not recursing into tools/golang/xenlight/ is
enough to fix the build for me for the moment. I don't currently see why
60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
it necessary to invoke this build step unconditionally.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm also having trouble to see why, besides the idl-gen target in
tools/golang/xenlight/Makefile, the commit also adds such a target to
(and mentions it in [only] a comment in) tools/libxl/Makefile.

Comments

Wei Liu Aug. 4, 2020, 2:16 p.m. UTC | #1
On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
> While this doesn't address the real problem I've run into (attempting to
> update r/o source files), not recursing into tools/golang/xenlight/ is
> enough to fix the build for me for the moment. I don't currently see why
> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
> it necessary to invoke this build step unconditionally.
> 

Perhaps an oversight?

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Wei Liu <wl@xen.org>

> ---
> I'm also having trouble to see why, besides the idl-gen target in
> tools/golang/xenlight/Makefile, the commit also adds such a target to
> (and mentions it in [only] a comment in) tools/libxl/Makefile.
> 
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -282,7 +282,9 @@ _libxl_type%.h _libxl_type%_json.h _libx
>  # about races with tools/golang/xenlight/Makefile:all
>  .PHONY: idl-external
>  idl-external:
> +ifeq ($(CONFIG_GOLANG),y)
>  	$(MAKE) -C $(XEN_ROOT)/tools/golang/xenlight idl-gen
> +endif
>  
>  LIBXL_IDLGEN_FILES = _libxl_types.h _libxl_types_json.h _libxl_types_private.h _libxl_types.c \
>  	_libxl_types_internal.h _libxl_types_internal_json.h _libxl_types_internal_private.h _libxl_types_internal.c
Nick Rosbrook Aug. 4, 2020, 3:22 p.m. UTC | #2
On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
>
> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
> > While this doesn't address the real problem I've run into (attempting to
> > update r/o source files), not recursing into tools/golang/xenlight/ is
> > enough to fix the build for me for the moment. I don't currently see why
> > 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
> > it necessary to invoke this build step unconditionally.
> >
>
> Perhaps an oversight?

This is intentional, and I think the commit message in 60db5da62ac0
("libxl: Generate golang bindings in libxl Makefile") explains the
reasoning well. But, to summarize, CONFIG_GOLANG is only used to
control the bindings actually being compiled (i.e. with `go build`).
However, we always want the code generation script
(tools/golang/xenlight/gengotypes.py) to run if e.g.
tools/libxl/libxl_types.idl is modified.

I hope this helps.

-NR
Jan Beulich Aug. 4, 2020, 3:30 p.m. UTC | #3
On 04.08.2020 17:22, Nick Rosbrook wrote:
> On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
>>
>> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
>>> While this doesn't address the real problem I've run into (attempting to
>>> update r/o source files), not recursing into tools/golang/xenlight/ is
>>> enough to fix the build for me for the moment. I don't currently see why
>>> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
>>> it necessary to invoke this build step unconditionally.
>>>
>>
>> Perhaps an oversight?
> 
> This is intentional, and I think the commit message in 60db5da62ac0
> ("libxl: Generate golang bindings in libxl Makefile") explains the
> reasoning well. But, to summarize, CONFIG_GOLANG is only used to
> control the bindings actually being compiled (i.e. with `go build`).
> However, we always want the code generation script
> (tools/golang/xenlight/gengotypes.py) to run if e.g.
> tools/libxl/libxl_types.idl is modified.
> 
> I hope this helps.

Not really - I'm still not seeing the "why" behind this behavior. I.e.
why build _anything_ that's not used further in the build, nor getting
installed? Also if (aiui) you effectively object to the change that
Wei has given his ack for, would you mind providing an alternative fix
for the problem at hand?

Jan
Wei Liu Aug. 4, 2020, 3:50 p.m. UTC | #4
On Tue, Aug 04, 2020 at 05:30:40PM +0200, Jan Beulich wrote:
> On 04.08.2020 17:22, Nick Rosbrook wrote:
> > On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
> >>
> >> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
> >>> While this doesn't address the real problem I've run into (attempting to
> >>> update r/o source files), not recursing into tools/golang/xenlight/ is
> >>> enough to fix the build for me for the moment. I don't currently see why
> >>> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
> >>> it necessary to invoke this build step unconditionally.
> >>>
> >>
> >> Perhaps an oversight?
> > 
> > This is intentional, and I think the commit message in 60db5da62ac0
> > ("libxl: Generate golang bindings in libxl Makefile") explains the
> > reasoning well. But, to summarize, CONFIG_GOLANG is only used to
> > control the bindings actually being compiled (i.e. with `go build`).
> > However, we always want the code generation script
> > (tools/golang/xenlight/gengotypes.py) to run if e.g.
> > tools/libxl/libxl_types.idl is modified.
> > 
> > I hope this helps.
> 
> Not really - I'm still not seeing the "why" behind this behavior. I.e.
> why build _anything_ that's not used further in the build, nor getting
> installed? Also if (aiui) you effectively object to the change that
> Wei has given his ack for, would you mind providing an alternative fix
> for the problem at hand?

Is the solution here to make the target check if IDL definition file is
actually changed before regenerating the bindings?

Admittedly I had misunderstood what CONFIG_GOLANG meant.

Wei.

> 
> Jan
Jan Beulich Aug. 4, 2020, 3:53 p.m. UTC | #5
On 04.08.2020 17:50, Wei Liu wrote:
> On Tue, Aug 04, 2020 at 05:30:40PM +0200, Jan Beulich wrote:
>> On 04.08.2020 17:22, Nick Rosbrook wrote:
>>> On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
>>>>
>>>> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
>>>>> While this doesn't address the real problem I've run into (attempting to
>>>>> update r/o source files), not recursing into tools/golang/xenlight/ is
>>>>> enough to fix the build for me for the moment. I don't currently see why
>>>>> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
>>>>> it necessary to invoke this build step unconditionally.
>>>>>
>>>>
>>>> Perhaps an oversight?
>>>
>>> This is intentional, and I think the commit message in 60db5da62ac0
>>> ("libxl: Generate golang bindings in libxl Makefile") explains the
>>> reasoning well. But, to summarize, CONFIG_GOLANG is only used to
>>> control the bindings actually being compiled (i.e. with `go build`).
>>> However, we always want the code generation script
>>> (tools/golang/xenlight/gengotypes.py) to run if e.g.
>>> tools/libxl/libxl_types.idl is modified.
>>>
>>> I hope this helps.
>>
>> Not really - I'm still not seeing the "why" behind this behavior. I.e.
>> why build _anything_ that's not used further in the build, nor getting
>> installed? Also if (aiui) you effectively object to the change that
>> Wei has given his ack for, would you mind providing an alternative fix
>> for the problem at hand?
> 
> Is the solution here to make the target check if IDL definition file is
> actually changed before regenerating the bindings?

I don't know - Nick? A move-if-changed based approach would likely deal
with the r/o source problem at the same time (at least until such time
where the directory containing the file(s) is also r/o).

Jan
Wei Liu Aug. 4, 2020, 3:57 p.m. UTC | #6
On Tue, Aug 04, 2020 at 05:53:49PM +0200, Jan Beulich wrote:
> On 04.08.2020 17:50, Wei Liu wrote:
> > On Tue, Aug 04, 2020 at 05:30:40PM +0200, Jan Beulich wrote:
> >> On 04.08.2020 17:22, Nick Rosbrook wrote:
> >>> On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
> >>>>
> >>>> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
> >>>>> While this doesn't address the real problem I've run into (attempting to
> >>>>> update r/o source files), not recursing into tools/golang/xenlight/ is
> >>>>> enough to fix the build for me for the moment. I don't currently see why
> >>>>> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
> >>>>> it necessary to invoke this build step unconditionally.
> >>>>>
> >>>>
> >>>> Perhaps an oversight?
> >>>
> >>> This is intentional, and I think the commit message in 60db5da62ac0
> >>> ("libxl: Generate golang bindings in libxl Makefile") explains the
> >>> reasoning well. But, to summarize, CONFIG_GOLANG is only used to
> >>> control the bindings actually being compiled (i.e. with `go build`).
> >>> However, we always want the code generation script
> >>> (tools/golang/xenlight/gengotypes.py) to run if e.g.
> >>> tools/libxl/libxl_types.idl is modified.
> >>>
> >>> I hope this helps.
> >>
> >> Not really - I'm still not seeing the "why" behind this behavior. I.e.
> >> why build _anything_ that's not used further in the build, nor getting
> >> installed? Also if (aiui) you effectively object to the change that
> >> Wei has given his ack for, would you mind providing an alternative fix
> >> for the problem at hand?
> > 
> > Is the solution here to make the target check if IDL definition file is
> > actually changed before regenerating the bindings?
> 
> I don't know - Nick? A move-if-changed based approach would likely deal
> with the r/o source problem at the same time (at least until such time
> where the directory containing the file(s) is also r/o).

To make sure Nick and I understand your use case correct -- "r/o source
problem" means you want the tools source to be read-only? But you would
be fine recursing into tools directory to build all the libraries and
programs?

Wei.

> 
> Jan
Jan Beulich Aug. 4, 2020, 4:02 p.m. UTC | #7
On 04.08.2020 17:57, Wei Liu wrote:
> On Tue, Aug 04, 2020 at 05:53:49PM +0200, Jan Beulich wrote:
>> On 04.08.2020 17:50, Wei Liu wrote:
>>> On Tue, Aug 04, 2020 at 05:30:40PM +0200, Jan Beulich wrote:
>>>> On 04.08.2020 17:22, Nick Rosbrook wrote:
>>>>> On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
>>>>>>
>>>>>> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
>>>>>>> While this doesn't address the real problem I've run into (attempting to
>>>>>>> update r/o source files), not recursing into tools/golang/xenlight/ is
>>>>>>> enough to fix the build for me for the moment. I don't currently see why
>>>>>>> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
>>>>>>> it necessary to invoke this build step unconditionally.
>>>>>>>
>>>>>>
>>>>>> Perhaps an oversight?
>>>>>
>>>>> This is intentional, and I think the commit message in 60db5da62ac0
>>>>> ("libxl: Generate golang bindings in libxl Makefile") explains the
>>>>> reasoning well. But, to summarize, CONFIG_GOLANG is only used to
>>>>> control the bindings actually being compiled (i.e. with `go build`).
>>>>> However, we always want the code generation script
>>>>> (tools/golang/xenlight/gengotypes.py) to run if e.g.
>>>>> tools/libxl/libxl_types.idl is modified.
>>>>>
>>>>> I hope this helps.
>>>>
>>>> Not really - I'm still not seeing the "why" behind this behavior. I.e.
>>>> why build _anything_ that's not used further in the build, nor getting
>>>> installed? Also if (aiui) you effectively object to the change that
>>>> Wei has given his ack for, would you mind providing an alternative fix
>>>> for the problem at hand?
>>>
>>> Is the solution here to make the target check if IDL definition file is
>>> actually changed before regenerating the bindings?
>>
>> I don't know - Nick? A move-if-changed based approach would likely deal
>> with the r/o source problem at the same time (at least until such time
>> where the directory containing the file(s) is also r/o).
> 
> To make sure Nick and I understand your use case correct -- "r/o source
> problem" means you want the tools source to be read-only? But you would
> be fine recursing into tools directory to build all the libraries and
> programs?

Yes - until we support out-of-tree builds, nothing more can be expected
to work.

Jan
Nick Rosbrook Aug. 4, 2020, 4:41 p.m. UTC | #8
On Tue, Aug 4, 2020 at 12:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.08.2020 17:57, Wei Liu wrote:
> > On Tue, Aug 04, 2020 at 05:53:49PM +0200, Jan Beulich wrote:
> >> On 04.08.2020 17:50, Wei Liu wrote:
> >>> On Tue, Aug 04, 2020 at 05:30:40PM +0200, Jan Beulich wrote:
> >>>> On 04.08.2020 17:22, Nick Rosbrook wrote:
> >>>>> On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
> >>>>>>
> >>>>>> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
> >>>>>>> While this doesn't address the real problem I've run into (attempting to
> >>>>>>> update r/o source files), not recursing into tools/golang/xenlight/ is
> >>>>>>> enough to fix the build for me for the moment. I don't currently see why
> >>>>>>> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
> >>>>>>> it necessary to invoke this build step unconditionally.
> >>>>>>>
> >>>>>>
> >>>>>> Perhaps an oversight?
> >>>>>
> >>>>> This is intentional, and I think the commit message in 60db5da62ac0
> >>>>> ("libxl: Generate golang bindings in libxl Makefile") explains the
> >>>>> reasoning well. But, to summarize, CONFIG_GOLANG is only used to
> >>>>> control the bindings actually being compiled (i.e. with `go build`).
> >>>>> However, we always want the code generation script
> >>>>> (tools/golang/xenlight/gengotypes.py) to run if e.g.
> >>>>> tools/libxl/libxl_types.idl is modified.
> >>>>>
> >>>>> I hope this helps.
> >>>>
> >>>> Not really - I'm still not seeing the "why" behind this behavior. I.e.
> >>>> why build _anything_ that's not used further in the build, nor getting
> >>>> installed? Also if (aiui) you effectively object to the change that
> >>>> Wei has given his ack for, would you mind providing an alternative fix
> >>>> for the problem at hand?
> >>>
> >>> Is the solution here to make the target check if IDL definition file is
> >>> actually changed before regenerating the bindings?
> >>
> >> I don't know - Nick? A move-if-changed based approach would likely deal
> >> with the r/o source problem at the same time (at least until such time
> >> where the directory containing the file(s) is also r/o).
> >
> > To make sure Nick and I understand your use case correct -- "r/o source
> > problem" means you want the tools source to be read-only? But you would
> > be fine recursing into tools directory to build all the libraries and
> > programs?
>
> Yes - until we support out-of-tree builds, nothing more can be expected
> to work.
>

Jan - is the problem specifically that a fresh clone,  or `git
checkout`, etc. changes file timestamps in a way that triggers make to
rebuild those targets? I have not used the move-if-changed approach
before, but AFAICT that would be sufficient.

-NR
Jan Beulich Aug. 5, 2020, 6:35 a.m. UTC | #9
On 04.08.2020 18:41, Nick Rosbrook wrote:
> On Tue, Aug 4, 2020 at 12:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 04.08.2020 17:57, Wei Liu wrote:
>>> On Tue, Aug 04, 2020 at 05:53:49PM +0200, Jan Beulich wrote:
>>>> On 04.08.2020 17:50, Wei Liu wrote:
>>>>> On Tue, Aug 04, 2020 at 05:30:40PM +0200, Jan Beulich wrote:
>>>>>> On 04.08.2020 17:22, Nick Rosbrook wrote:
>>>>>>> On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
>>>>>>>>
>>>>>>>> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
>>>>>>>>> While this doesn't address the real problem I've run into (attempting to
>>>>>>>>> update r/o source files), not recursing into tools/golang/xenlight/ is
>>>>>>>>> enough to fix the build for me for the moment. I don't currently see why
>>>>>>>>> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
>>>>>>>>> it necessary to invoke this build step unconditionally.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Perhaps an oversight?
>>>>>>>
>>>>>>> This is intentional, and I think the commit message in 60db5da62ac0
>>>>>>> ("libxl: Generate golang bindings in libxl Makefile") explains the
>>>>>>> reasoning well. But, to summarize, CONFIG_GOLANG is only used to
>>>>>>> control the bindings actually being compiled (i.e. with `go build`).
>>>>>>> However, we always want the code generation script
>>>>>>> (tools/golang/xenlight/gengotypes.py) to run if e.g.
>>>>>>> tools/libxl/libxl_types.idl is modified.
>>>>>>>
>>>>>>> I hope this helps.
>>>>>>
>>>>>> Not really - I'm still not seeing the "why" behind this behavior. I.e.
>>>>>> why build _anything_ that's not used further in the build, nor getting
>>>>>> installed? Also if (aiui) you effectively object to the change that
>>>>>> Wei has given his ack for, would you mind providing an alternative fix
>>>>>> for the problem at hand?
>>>>>
>>>>> Is the solution here to make the target check if IDL definition file is
>>>>> actually changed before regenerating the bindings?
>>>>
>>>> I don't know - Nick? A move-if-changed based approach would likely deal
>>>> with the r/o source problem at the same time (at least until such time
>>>> where the directory containing the file(s) is also r/o).
>>>
>>> To make sure Nick and I understand your use case correct -- "r/o source
>>> problem" means you want the tools source to be read-only? But you would
>>> be fine recursing into tools directory to build all the libraries and
>>> programs?
>>
>> Yes - until we support out-of-tree builds, nothing more can be expected
>> to work.
>>
> 
> Jan - is the problem specifically that a fresh clone,  or `git
> checkout`, etc. changes file timestamps in a way that triggers make to
> rebuild those targets?

Afaict it's not deterministic in which order files get created / updated,
and hence the generated files may or may not appear older than their
dependencies.

Jan
Ian Jackson Aug. 5, 2020, 9:55 a.m. UTC | #10
Nick Rosbrook writes ("Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y"):
> Jan - is the problem specifically that a fresh clone,  or `git
> checkout`, etc. changes file timestamps in a way that triggers make to
> rebuild those targets? I have not used the move-if-changed approach
> before, but AFAICT that would be sufficient.

I don't think there is, from the point of view of the build system,
anything different about gengotypes than about any other in-tree
committed file which is updated using makefile rules based on only
other in-tree files and common utilities (eg, in this case, Python).

I guess using move-if-changed will probably fix this.

Jan: the reasons why this output file has to be committed are
complicated.  We've discussed them at length.  Ultimately the reason
is deliberate deficiencies[1] in golang.  Sadly this is the best of a
not-very-good set of options.

Ian.

[1] This is an extreme phrase, but justified I think.  The golang
designers have deliberately aimed at what they regard as "simplicity"
and one of the things they have "simplified" away (in their language
and in their package management and build tools) is the ability to
conveniently generate golang code at build time.  Committing the
generated code is the norm in the golang community.
George Dunlap Aug. 10, 2020, 3:17 p.m. UTC | #11
> On Aug 5, 2020, at 10:55 AM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> Nick Rosbrook writes ("Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y"):
>> Jan - is the problem specifically that a fresh clone,  or `git
>> checkout`, etc. changes file timestamps in a way that triggers make to
>> rebuild those targets? I have not used the move-if-changed approach
>> before, but AFAICT that would be sufficient.
> 
> I don't think there is, from the point of view of the build system,
> anything different about gengotypes than about any other in-tree
> committed file which is updated using makefile rules based on only
> other in-tree files and common utilities (eg, in this case, Python).
> 
> I guess using move-if-changed will probably fix this.

That’s probably the quickest fix ATM.

> Jan: the reasons why this output file has to be committed are
> complicated.  We've discussed them at length.  Ultimately the reason
> is deliberate deficiencies[1] in golang.  Sadly this is the best of a
> not-very-good set of options.

I think we decided at the Summit to make a separate repo for the generated code, didn’t we?

To expand on this, Jan:

1. A *useful* go package must be able to be downloaded and built by the go build tools from the URL of a git repo

2. The go build process is restricted in what it will do automatically for security reasons.

For 4.14, we chose to check the generated code into xen.git, so that the main Xen repo could fulfill #1.  To make sure that the generated code was kept up to date with changes in libxl_types.idl, we decided to re-generate the code even for systems which don’t have golang installed.

But this is another example of the annoying side effects of this approach.

The other approach, which I thought we’d agreed upon at the summit, is to have a separate repo with a more friendly URL which is programmatically generated on a regular basis.  That would obviate the need to run the generator, except to verify that the generated code still compiled (which wouldn’t be possible without having golang installed anyway).

> [1] This is an extreme phrase, but justified I think.  The golang
> designers have deliberately aimed at what they regard as "simplicity"
> and one of the things they have "simplified" away (in their language
> and in their package management and build tools) is the ability to
> conveniently generate golang code at build time.  Committing the
> generated code is the norm in the golang community.

It’s a bit less unreasonable than this. :-)  There actually is infrastructure for generating files — `go generate`.  It’s just not allowed to run as part of a build.

One of the core things they wanted to be able to do was to download and build dependencies, recursively and automatically, from arbitrary URLs, without the need for any curation.  For safety concerns, they don’t want any such dependencies to be able to run arbitrary commands as part of their build: Go builds can basically call compilers and linkers and that’s it.

Not sure I actually buy that this gives you a whole lot of safety, since there’s not a not of point in compiling something you’re not going to run; and it’s not clear to me that it’s terribly more risky to run code you haven’t vetted than to build code you haven’t vetted.  But at least it’s not obviously wrong.

 -George
Jan Beulich Aug. 24, 2020, 1:11 p.m. UTC | #12
On 04.08.2020 18:41, Nick Rosbrook wrote:
> On Tue, Aug 4, 2020 at 12:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 04.08.2020 17:57, Wei Liu wrote:
>>> On Tue, Aug 04, 2020 at 05:53:49PM +0200, Jan Beulich wrote:
>>>> On 04.08.2020 17:50, Wei Liu wrote:
>>>>> On Tue, Aug 04, 2020 at 05:30:40PM +0200, Jan Beulich wrote:
>>>>>> On 04.08.2020 17:22, Nick Rosbrook wrote:
>>>>>>> On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
>>>>>>>>
>>>>>>>> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
>>>>>>>>> While this doesn't address the real problem I've run into (attempting to
>>>>>>>>> update r/o source files), not recursing into tools/golang/xenlight/ is
>>>>>>>>> enough to fix the build for me for the moment. I don't currently see why
>>>>>>>>> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
>>>>>>>>> it necessary to invoke this build step unconditionally.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Perhaps an oversight?
>>>>>>>
>>>>>>> This is intentional, and I think the commit message in 60db5da62ac0
>>>>>>> ("libxl: Generate golang bindings in libxl Makefile") explains the
>>>>>>> reasoning well. But, to summarize, CONFIG_GOLANG is only used to
>>>>>>> control the bindings actually being compiled (i.e. with `go build`).
>>>>>>> However, we always want the code generation script
>>>>>>> (tools/golang/xenlight/gengotypes.py) to run if e.g.
>>>>>>> tools/libxl/libxl_types.idl is modified.
>>>>>>>
>>>>>>> I hope this helps.
>>>>>>
>>>>>> Not really - I'm still not seeing the "why" behind this behavior. I.e.
>>>>>> why build _anything_ that's not used further in the build, nor getting
>>>>>> installed? Also if (aiui) you effectively object to the change that
>>>>>> Wei has given his ack for, would you mind providing an alternative fix
>>>>>> for the problem at hand?
>>>>>
>>>>> Is the solution here to make the target check if IDL definition file is
>>>>> actually changed before regenerating the bindings?
>>>>
>>>> I don't know - Nick? A move-if-changed based approach would likely deal
>>>> with the r/o source problem at the same time (at least until such time
>>>> where the directory containing the file(s) is also r/o).
>>>
>>> To make sure Nick and I understand your use case correct -- "r/o source
>>> problem" means you want the tools source to be read-only? But you would
>>> be fine recursing into tools directory to build all the libraries and
>>> programs?
>>
>> Yes - until we support out-of-tree builds, nothing more can be expected
>> to work.
> 
> Jan - is the problem specifically that a fresh clone,  or `git
> checkout`, etc. changes file timestamps in a way that triggers make to
> rebuild those targets? I have not used the move-if-changed approach
> before, but AFAICT that would be sufficient.

Since about three weeks have passed and - unless I've missed something -
the issue is still there, I'd like to clarify who's going to address the
(how I would call it) regression. I thought I had expressed that if my
proposed version isn't acceptable, I'd rather see you deal with the issue.
Did you perhaps imply the opposite?

Jan
Nick Rosbrook Aug. 24, 2020, 2:58 p.m. UTC | #13
On Mon, Aug 24, 2020 at 03:11:41PM +0200, Jan Beulich wrote:
> On 04.08.2020 18:41, Nick Rosbrook wrote:
> > On Tue, Aug 4, 2020 at 12:02 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 04.08.2020 17:57, Wei Liu wrote:
> >>> On Tue, Aug 04, 2020 at 05:53:49PM +0200, Jan Beulich wrote:
> >>>> On 04.08.2020 17:50, Wei Liu wrote:
> >>>>> On Tue, Aug 04, 2020 at 05:30:40PM +0200, Jan Beulich wrote:
> >>>>>> On 04.08.2020 17:22, Nick Rosbrook wrote:
> >>>>>>> On Tue, Aug 4, 2020 at 10:17 AM Wei Liu <wl@xen.org> wrote:
> >>>>>>>>
> >>>>>>>> On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
> >>>>>>>>> While this doesn't address the real problem I've run into (attempting to
> >>>>>>>>> update r/o source files), not recursing into tools/golang/xenlight/ is
> >>>>>>>>> enough to fix the build for me for the moment. I don't currently see why
> >>>>>>>>> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") found
> >>>>>>>>> it necessary to invoke this build step unconditionally.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Perhaps an oversight?
> >>>>>>>
> >>>>>>> This is intentional, and I think the commit message in 60db5da62ac0
> >>>>>>> ("libxl: Generate golang bindings in libxl Makefile") explains the
> >>>>>>> reasoning well. But, to summarize, CONFIG_GOLANG is only used to
> >>>>>>> control the bindings actually being compiled (i.e. with `go build`).
> >>>>>>> However, we always want the code generation script
> >>>>>>> (tools/golang/xenlight/gengotypes.py) to run if e.g.
> >>>>>>> tools/libxl/libxl_types.idl is modified.
> >>>>>>>
> >>>>>>> I hope this helps.
> >>>>>>
> >>>>>> Not really - I'm still not seeing the "why" behind this behavior. I.e.
> >>>>>> why build _anything_ that's not used further in the build, nor getting
> >>>>>> installed? Also if (aiui) you effectively object to the change that
> >>>>>> Wei has given his ack for, would you mind providing an alternative fix
> >>>>>> for the problem at hand?
> >>>>>
> >>>>> Is the solution here to make the target check if IDL definition file is
> >>>>> actually changed before regenerating the bindings?
> >>>>
> >>>> I don't know - Nick? A move-if-changed based approach would likely deal
> >>>> with the r/o source problem at the same time (at least until such time
> >>>> where the directory containing the file(s) is also r/o).
> >>>
> >>> To make sure Nick and I understand your use case correct -- "r/o source
> >>> problem" means you want the tools source to be read-only? But you would
> >>> be fine recursing into tools directory to build all the libraries and
> >>> programs?
> >>
> >> Yes - until we support out-of-tree builds, nothing more can be expected
> >> to work.
> > 
> > Jan - is the problem specifically that a fresh clone,  or `git
> > checkout`, etc. changes file timestamps in a way that triggers make to
> > rebuild those targets? I have not used the move-if-changed approach
> > before, but AFAICT that would be sufficient.
> 
> Since about three weeks have passed and - unless I've missed something -
> the issue is still there, I'd like to clarify who's going to address the
> (how I would call it) regression. I thought I had expressed that if my
> proposed version isn't acceptable, I'd rather see you deal with the issue.
> Did you perhaps imply the opposite?

Hi Jan,

My understanding was that you were going to use move-if-changed to fix
this for now (it seemed everyone agreed this was the quickest short-term fix).

The long-term plan (as George mentioned earlier in this thread) is to have 
a separate repository where the generated Go code is checked-in, rather
than xen.git. While this will eventually fix your problem, we are not
actively working on this yet.

Sorry for any misunderstanding.

-NR
Jan Beulich Aug. 25, 2020, 6:47 a.m. UTC | #14
On 24.08.2020 16:58, Nick Rosbrook wrote:
> My understanding was that you were going to use move-if-changed to fix
> this for now (it seemed everyone agreed this was the quickest short-term fix).

A technical and a non-technical remark:

Thinking about this some more, I'm no longer convinced the
move-if-changed approach is appropriate here. It is typically
used to avoid updating files with a large set of dependents
(all of which would need re-building if the file in question
changed, even if merely in its time stamp), and where the
cost of re-generating (and comparing) is relatively low.
While I can't really assess the cost part here (I know too
little of Python to be able to compare its use with e.g. a
shell script), I don't think the "large set of dependencies"
aspect applies here at all.

On the non-technical side I have to admit that I find it,
well, unfriendly to have a person not only run into and
investigate a (recent) regression, but also make multiple
attempts at fixing (or at least working around) it. I'd
rather view this as preferably the responsibility of the
person having introduced an issue. In the case at hand it is
quite clear that I wasn't even remotely aware of the
requirements, and hence determination and testing of a more
adequate solution would far better be done by someone
familiar with all the influencing factors. (Things might
yet be different if an issue is difficult to reproduce, but
I don't see that being the case here.)

Jan
George Dunlap Aug. 25, 2020, 10:37 a.m. UTC | #15
> On Aug 25, 2020, at 7:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
> On 24.08.2020 16:58, Nick Rosbrook wrote:
>> My understanding was that you were going to use move-if-changed to fix
>> this for now (it seemed everyone agreed this was the quickest short-term fix).
> 
> A technical and a non-technical remark:
> 
> Thinking about this some more, I'm no longer convinced the
> move-if-changed approach is appropriate here. It is typically
> used to avoid updating files with a large set of dependents
> (all of which would need re-building if the file in question
> changed, even if merely in its time stamp), and where the
> cost of re-generating (and comparing) is relatively low.
> While I can't really assess the cost part here (I know too
> little of Python to be able to compare its use with e.g. a
> shell script), I don't think the "large set of dependencies"
> aspect applies here at all.
> 
> On the non-technical side I have to admit that I find it,
> well, unfriendly to have a person not only run into and
> investigate a (recent) regression, but also make multiple
> attempts at fixing (or at least working around) it. I'd
> rather view this as preferably the responsibility of the
> person having introduced an issue. In the case at hand it is
> quite clear that I wasn't even remotely aware of the
> requirements, and hence determination and testing of a more
> adequate solution would far better be done by someone
> familiar with all the influencing factors. (Things might
> yet be different if an issue is difficult to reproduce, but
> I don't see that being the case here.)

Yes, this has been sub-optimal for you to have your functionality broken for several weeks.

As an explanation, there are a combination of things. You proposed A (remove the dependency), Ian proposed B (use move-if-changed), but we’re hoping to do C (have an external tree) before the next release.  I haven’t had the time to look into either B or C (nor, unfortunately, to review Nick’s submissions to other parts of the code — sorry Nick!); but I’ve still been reluctant to go for A.

I think basically, unless someone is ready to tackle B or C immediately, we should just check in Jan’s fix (or probably better, just revert the patch that introduced the dependency).  It will be annoying to have to potentially fix up the generated golang bindings, but that puts the incentives in the right place.

 -George
Nick Rosbrook Aug. 25, 2020, 2:57 p.m. UTC | #16
On Tue, Aug 25, 2020 at 10:37:09AM +0000, George Dunlap wrote:
> 
> 
> > On Aug 25, 2020, at 7:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
> > 
> > On 24.08.2020 16:58, Nick Rosbrook wrote:
> >> My understanding was that you were going to use move-if-changed to fix
> >> this for now (it seemed everyone agreed this was the quickest short-term fix).
> > 
> > A technical and a non-technical remark:
> > 
> > Thinking about this some more, I'm no longer convinced the
> > move-if-changed approach is appropriate here. It is typically
> > used to avoid updating files with a large set of dependents
> > (all of which would need re-building if the file in question
> > changed, even if merely in its time stamp), and where the
> > cost of re-generating (and comparing) is relatively low.
> > While I can't really assess the cost part here (I know too
> > little of Python to be able to compare its use with e.g. a
> > shell script), I don't think the "large set of dependencies"
> > aspect applies here at all.
> > 
> > On the non-technical side I have to admit that I find it,
> > well, unfriendly to have a person not only run into and
> > investigate a (recent) regression, but also make multiple
> > attempts at fixing (or at least working around) it. I'd
> > rather view this as preferably the responsibility of the
> > person having introduced an issue. In the case at hand it is
> > quite clear that I wasn't even remotely aware of the
> > requirements, and hence determination and testing of a more
> > adequate solution would far better be done by someone
> > familiar with all the influencing factors. (Things might
> > yet be different if an issue is difficult to reproduce, but
> > I don't see that being the case here.)
> 
> Yes, this has been sub-optimal for you to have your functionality broken for several weeks.
> 
> As an explanation, there are a combination of things. You proposed A (remove the dependency), Ian proposed B (use move-if-changed), but we’re hoping to do C (have an external tree) before the next release.  I haven’t had the time to look into either B or C (nor, unfortunately, to review Nick’s submissions to other parts of the code — sorry Nick!); but I’ve still been reluctant to go for A.
> 
> I think basically, unless someone is ready to tackle B or C immediately, we should just check in Jan’s fix (or probably better, just revert the patch that introduced the dependency).  It will be annoying to have to potentially fix up the generated golang bindings, but that puts the incentives in the right place.

Yeah, that's probably best. I for one do not have any immediate plans
for working on option C.

Jan - I apologize for the confusion, I certainly did not mean to be
unfriendly or hold up your work.

-NR
Jan Beulich Aug. 26, 2020, 7:41 a.m. UTC | #17
On 25.08.2020 12:37, George Dunlap wrote:
> As an explanation, there are a combination of things. You proposed A (remove the dependency), Ian proposed B (use move-if-changed), but we’re hoping to do C (have an external tree) before the next release.  I haven’t had the time to look into either B or C (nor, unfortunately, to review Nick’s submissions to other parts of the code — sorry Nick!); but I’ve still been reluctant to go for A.
> 
> I think basically, unless someone is ready to tackle B or C immediately, we should just check in Jan’s fix (or probably better, just revert the patch that introduced the dependency).  It will be annoying to have to potentially fix up the generated golang bindings, but that puts the incentives in the right place.

One additional aspect to consider is that I ran into the issue actually
in a 4.14 tree (because it just so happened that the timestamps of the
involved files were "right" for the problem to be hit), i.e. whatever
we decide to do will also end up needing backporting. To me this looks
to make A less attractive.

Jan
George Dunlap Aug. 26, 2020, 10:33 a.m. UTC | #18
> On Aug 26, 2020, at 8:41 AM, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 25.08.2020 12:37, George Dunlap wrote:
>> As an explanation, there are a combination of things. You proposed A (remove the dependency), Ian proposed B (use move-if-changed), but we’re hoping to do C (have an external tree) before the next release.  I haven’t had the time to look into either B or C (nor, unfortunately, to review Nick’s submissions to other parts of the code — sorry Nick!); but I’ve still been reluctant to go for A.
>> 
>> I think basically, unless someone is ready to tackle B or C immediately, we should just check in Jan’s fix (or probably better, just revert the patch that introduced the dependency).  It will be annoying to have to potentially fix up the generated golang bindings, but that puts the incentives in the right place.
> 
> One additional aspect to consider is that I ran into the issue actually
> in a 4.14 tree (because it just so happened that the timestamps of the
> involved files were "right" for the problem to be hit), i.e. whatever
> we decide to do will also end up needing backporting. To me this looks
> to make A less attractive.

I don’t understand why?  If it’s a regression in 4.14 functionality, we have to backport something to fix it one way or another.

If we were going to leave the functionality the way it is, it might make sense to make it so that the dependency was triggered only on staging/master; the goal, after all, was to make sure that the generated files were updated when libxl_types.idl was updated during development.

BTW, one way to prevent this from happening would be to add a version of the build to the Gitlab CI loop which would build out-of-tree and fail in a similar manner.  If there had been such a test, this change would have been reverted immediately.

 -George
Jan Beulich Aug. 26, 2020, 11:17 a.m. UTC | #19
On 26.08.2020 12:33, George Dunlap wrote:
> 
> 
>> On Aug 26, 2020, at 8:41 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 25.08.2020 12:37, George Dunlap wrote:
>>> As an explanation, there are a combination of things. You proposed A (remove the dependency), Ian proposed B (use move-if-changed), but we’re hoping to do C (have an external tree) before the next release.  I haven’t had the time to look into either B or C (nor, unfortunately, to review Nick’s submissions to other parts of the code — sorry Nick!); but I’ve still been reluctant to go for A.
>>>
>>> I think basically, unless someone is ready to tackle B or C immediately, we should just check in Jan’s fix (or probably better, just revert the patch that introduced the dependency).  It will be annoying to have to potentially fix up the generated golang bindings, but that puts the incentives in the right place.
>>
>> One additional aspect to consider is that I ran into the issue actually
>> in a 4.14 tree (because it just so happened that the timestamps of the
>> involved files were "right" for the problem to be hit), i.e. whatever
>> we decide to do will also end up needing backporting. To me this looks
>> to make A less attractive.
> 
> I don’t understand why?

Because you and Nick made clear that it's not the right way to deal with
the situation, i.e. I only consider this a band aid.

>  If it’s a regression in 4.14 functionality, we have to backport something to fix it one way or another.

Oh, yes, something will need backporting. But perhaps a more permanent
and/or appropriate solution?

> If we were going to leave the functionality the way it is, it might make sense to make it so that the dependency was triggered only on staging/master; the goal, after all, was to make sure that the generated files were updated when libxl_types.idl was updated during development.
> 
> BTW, one way to prevent this from happening would be to add a version of the build to the Gitlab CI loop which would build out-of-tree and fail in a similar manner.  If there had been such a test, this change would have been reverted immediately.

Not sure about this. For one, the out-of-tree aspect has got nothing
to do with it, I think. It's instead the read-only-ness of the
source file which matters. IOW I assume things would have worked
fine if I didn't keep my original source trees r/o at almost all
times (and hence the symlinks, when followed, make the files be
viewed as r/o in the build tree, too).

And then the timestamps matter, too. On a fresh clone (which is what
osstest uses afaik, and I guess which is also what's done in gitlab
CI), the two files quite likely would have matching ones, in which
case no re-build attempt would occur.

Jan

Patch
diff mbox series

--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -282,7 +282,9 @@  _libxl_type%.h _libxl_type%_json.h _libx
 # about races with tools/golang/xenlight/Makefile:all
 .PHONY: idl-external
 idl-external:
+ifeq ($(CONFIG_GOLANG),y)
 	$(MAKE) -C $(XEN_ROOT)/tools/golang/xenlight idl-gen
+endif
 
 LIBXL_IDLGEN_FILES = _libxl_types.h _libxl_types_json.h _libxl_types_private.h _libxl_types.c \
 	_libxl_types_internal.h _libxl_types_internal_json.h _libxl_types_internal_private.h _libxl_types_internal.c