| Message ID | e8dd70a7-bdde-e12a-3f4d-f52e58016234@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl: avoid golang building without CONFIG_GOLANG=y | expand |
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
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
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
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
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
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
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
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
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
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.
> 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
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
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
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
> 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
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
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
> 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
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
--- 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
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.