* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-04 16:41 ` Nick Rosbrook
@ 2020-08-05 6:35 ` Jan Beulich
2020-08-05 9:55 ` Ian Jackson
2020-08-24 13:11 ` Jan Beulich
2 siblings, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2020-08-05 6:35 UTC (permalink / raw)
To: Nick Rosbrook
Cc: Anthony Perard, xen-devel, Ian Jackson, George Dunlap, Wei Liu
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-04 16:41 ` Nick Rosbrook
2020-08-05 6:35 ` Jan Beulich
@ 2020-08-05 9:55 ` Ian Jackson
2020-08-10 15:17 ` George Dunlap
2020-08-24 13:11 ` Jan Beulich
2 siblings, 1 reply; 20+ messages in thread
From: Ian Jackson @ 2020-08-05 9:55 UTC (permalink / raw)
To: Nick Rosbrook
Cc: Anthony Perard, xen-devel, Wei Liu, Jan Beulich, George Dunlap
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.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-05 9:55 ` Ian Jackson
@ 2020-08-10 15:17 ` George Dunlap
0 siblings, 0 replies; 20+ messages in thread
From: George Dunlap @ 2020-08-10 15:17 UTC (permalink / raw)
To: Ian Jackson
Cc: Anthony Perard, Wei Liu, Nick Rosbrook, Jan Beulich, xen-devel
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-04 16:41 ` Nick Rosbrook
2020-08-05 6:35 ` Jan Beulich
2020-08-05 9:55 ` Ian Jackson
@ 2020-08-24 13:11 ` Jan Beulich
2020-08-24 14:58 ` Nick Rosbrook
2 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-08-24 13:11 UTC (permalink / raw)
To: Nick Rosbrook
Cc: Wei Liu, Anthony Perard, xen-devel, Ian Jackson, George Dunlap
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-24 13:11 ` Jan Beulich
@ 2020-08-24 14:58 ` Nick Rosbrook
2020-08-25 6:47 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Nick Rosbrook @ 2020-08-24 14:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Anthony Perard, xen-devel, Ian Jackson, George Dunlap
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-24 14:58 ` Nick Rosbrook
@ 2020-08-25 6:47 ` Jan Beulich
2020-08-25 10:37 ` George Dunlap
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-08-25 6:47 UTC (permalink / raw)
To: Nick Rosbrook
Cc: Wei Liu, Anthony Perard, xen-devel, Ian Jackson, George Dunlap
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-25 6:47 ` Jan Beulich
@ 2020-08-25 10:37 ` George Dunlap
2020-08-25 14:57 ` Nick Rosbrook
2020-08-26 7:41 ` Jan Beulich
0 siblings, 2 replies; 20+ messages in thread
From: George Dunlap @ 2020-08-25 10:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Nick Rosbrook, Wei Liu, Anthony Perard, xen-devel, Ian Jackson
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-25 10:37 ` George Dunlap
@ 2020-08-25 14:57 ` Nick Rosbrook
2020-08-26 7:41 ` Jan Beulich
1 sibling, 0 replies; 20+ messages in thread
From: Nick Rosbrook @ 2020-08-25 14:57 UTC (permalink / raw)
To: George Dunlap
Cc: Jan Beulich, Wei Liu, Anthony Perard, xen-devel, Ian Jackson
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-25 10:37 ` George Dunlap
2020-08-25 14:57 ` Nick Rosbrook
@ 2020-08-26 7:41 ` Jan Beulich
2020-08-26 10:33 ` George Dunlap
1 sibling, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-08-26 7:41 UTC (permalink / raw)
To: George Dunlap
Cc: Nick Rosbrook, Wei Liu, Anthony Perard, xen-devel, Ian Jackson
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-26 7:41 ` Jan Beulich
@ 2020-08-26 10:33 ` George Dunlap
2020-08-26 11:17 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: George Dunlap @ 2020-08-26 10:33 UTC (permalink / raw)
To: Jan Beulich
Cc: Nick Rosbrook, Wei Liu, Anthony Perard, xen-devel, Ian Jackson
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y
2020-08-26 10:33 ` George Dunlap
@ 2020-08-26 11:17 ` Jan Beulich
0 siblings, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2020-08-26 11:17 UTC (permalink / raw)
To: George Dunlap
Cc: Nick Rosbrook, Wei Liu, Anthony Perard, xen-devel, Ian Jackson
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
^ permalink raw reply [flat|nested] 20+ messages in thread