| Message ID | 20200522161240.3748320-6-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Golang build fixes | expand |
> Signed-off-by: George Dunlap <george.dunlap@citrix.com> Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>
George Dunlap writes ("[PATCH 5/5] gitignore: Ignore golang package directory"):
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
I have to say that finding a directory src/ in gitignore is very
startling.
This directory src/ contains only output files ?
Is there not a risk that humans will try to edit it ?
Ian.
> On May 26, 2020, at 2:54 PM, Ian Jackson <ian.jackson@citrix.com> wrote: > > George Dunlap writes ("[PATCH 5/5] gitignore: Ignore golang package directory"): >> Signed-off-by: George Dunlap <george.dunlap@citrix.com> > > I have to say that finding a directory src/ in gitignore is very > startling. > > This directory src/ contains only output files ? With golang, you don’t really distribute package binaries; you only distribute source files. However, we don’t want to wait until someone tries to clone the package to see if we’ve broken the build; so the current makefile does a “build test” of the package files. Before golang’s “modules” feature, the only way to do this was to have the code to build inside $GOPATH/src/$PACKAGENAME. We can set GOPATH but we can’t change the “src” component of that. So we used to set GOPATH to $XENROOT/tools/golang, put the files in $XENROOT/tools/golang/src/$PACKAGENAME, and With the “modules” feature, code can be built anywhere; the build at the moment doesn’t require GOPATH. If we’re willing to limit ourselves to using versions of golang which support modules by default (1.12+), then we can probably get rid of this bit instead. (And if we do want to support older versions, we should really add some code in the configure script to determine whether to build with modules or GOPATH.) Nick, any opinions? > Is there not a risk that humans will try to edit it ? I suppose someone might. If we decide we want to support older versions of go, we probably want to figure something out there. Options: 1. Copy the files to a temp directory instead. This is complicated because we have to find a good temp directory, and we’d have to copy them every time, slowing down the incremental build (though not that much). 2. Put a file in the generated directory like “GENERATED_DO_NOT_EDIT”. 3. Put them in tools/golang/GENERATED_DO_NOT_EDIT/src instead. -George
> With golang, you don’t really distribute package binaries; you only distribute source files. > > However, we don’t want to wait until someone tries to clone the package to see if we’ve broken the build; so the current makefile does a “build test” of the package files. > > Before golang’s “modules” feature, the only way to do this was to have the code to build inside $GOPATH/src/$PACKAGENAME. We can set GOPATH but we can’t change the “src” component of that. So we used to set GOPATH to $XENROOT/tools/golang, put the files in $XENROOT/tools/golang/src/$PACKAGENAME, and > > With the “modules” feature, code can be built anywhere; the build at the moment doesn’t require GOPATH. > > If we’re willing to limit ourselves to using versions of golang which support modules by default (1.12+), then we can probably get rid of this bit instead. (And if we do want to support older versions, we should really add some code in the configure script to determine whether to build with modules or GOPATH.) > > Nick, any opinions? I can't think of a reason we need to support anything older than go 1.11, so I think it would be fine to get rid of remnants of the GOPATH build. -NR
> On May 26, 2020, at 5:21 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote: > >> With golang, you don’t really distribute package binaries; you only distribute source files. >> >> However, we don’t want to wait until someone tries to clone the package to see if we’ve broken the build; so the current makefile does a “build test” of the package files. >> >> Before golang’s “modules” feature, the only way to do this was to have the code to build inside $GOPATH/src/$PACKAGENAME. We can set GOPATH but we can’t change the “src” component of that. So we used to set GOPATH to $XENROOT/tools/golang, put the files in $XENROOT/tools/golang/src/$PACKAGENAME, and >> >> With the “modules” feature, code can be built anywhere; the build at the moment doesn’t require GOPATH. >> >> If we’re willing to limit ourselves to using versions of golang which support modules by default (1.12+), then we can probably get rid of this bit instead. (And if we do want to support older versions, we should really add some code in the configure script to determine whether to build with modules or GOPATH.) >> >> Nick, any opinions? > > I can't think of a reason we need to support anything older than go > 1.11, so I think it would be fine to get rid of remnants of the GOPATH > build. OK, I’ll send a patch to remove the “fake GOPATH build” support. -George
George Dunlap writes ("Re: [PATCH 5/5] gitignore: Ignore golang package directory"):
> [explanation]
Sounds quite tangled...
> Nick, any opinions?
...
> > Is there not a risk that humans will try to edit it ?
Anyway ISTM that you have definitely considered this so
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
assuming that and Nick convince yourselves you've addressed this
possible issue.
> I suppose someone might. If we decide we want to support older versions of go, we probably want to figure something out there. Options:
>
> 1. Copy the files to a temp directory instead. This is complicated because we have to find a good temp directory, and we’d have to copy them every time, slowing down the incremental build (though not that much).
I don't think that helps much.
> 2. Put a file in the generated directory like “GENERATED_DO_NOT_EDIT”.
>
> 3. Put them in tools/golang/GENERATED_DO_NOT_EDIT/src instead.
Do they not have a header comment saying DO NOT EDIT ?
3 is pretty ugly. I'll leave it up to you whether to bother with 2.
Thanks,
Ian.
> On May 26, 2020, at 5:59 PM, Ian Jackson <ian.jackson@citrix.com> wrote: > > George Dunlap writes ("Re: [PATCH 5/5] gitignore: Ignore golang package directory"): >> [explanation] > > Sounds quite tangled... > >> Nick, any opinions? > ... >>> Is there not a risk that humans will try to edit it ? > > Anyway ISTM that you have definitely considered this so > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > assuming that and Nick convince yourselves you've addressed this > possible issue. > >> I suppose someone might. If we decide we want to support older versions of go, we probably want to figure something out there. Options: >> >> 1. Copy the files to a temp directory instead. This is complicated because we have to find a good temp directory, and we’d have to copy them every time, slowing down the incremental build (though not that much). > > I don't think that helps much. > >> 2. Put a file in the generated directory like “GENERATED_DO_NOT_EDIT”. >> >> 3. Put them in tools/golang/GENERATED_DO_NOT_EDIT/src instead. > > Do they not have a header comment saying DO NOT EDIT ? The generated files do, but this copies all the files, including the non-generated ones. Anyway, it turns out is has nothing to do with go modules per se, but more to do with my quixotic attempt to make it possible to build from stuff installed locally in $PREFIX, rather than having to clone something over the internet. The current version of the “build test” doesn’t actually use this GOPATH stuff, and works even on versions of golang that don’t have module support. I’ve got a patch that removes this whole fake-GOPATH thing; I’ll send that along in lieu of patches 4 and 5. Thanks, -George
diff --git a/.gitignore b/.gitignore index 7418ce9829..caaaa15b49 100644 --- a/.gitignore +++ b/.gitignore @@ -406,6 +406,7 @@ tools/python/xen/lowlevel/xl/_pyxl_types.h tools/xenstore/xenstore-watch tools/xl/_paths.h tools/xl/xl +tools/golang/src docs/txt/misc/*.txt docs/txt/man/*.txt
Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- CC: Ian Jackson <ian.jackson@citrix.com> CC: Wei Liu <wl@xen.org> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Konrad Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> CC: Nick Rosbrook <rosbrookn@ainfosec.com> --- .gitignore | 1 + 1 file changed, 1 insertion(+)