[pkg-go] Call for review - containerd and dependencies

[Michael Stapelberg]

Once you feel your packages are ready for review (possibly this reply
prompts some changes?), please send an up-to-date list of git URLs to what
you’d like reviewed. Thanks!

Replies inline:

On Fri, Feb 23, 2018 at 11:40 AM, Arnaud <arnaud.rebillout at collabora.com>
wrote:

>
>
> On 02/23/2018 05:10 PM, Michael Stapelberg wrote:
>
> I can’t have a look at the individual packages right now
>
>
> No rush !
>
> #### containerd
>> <https://salsa.debian.org/elboulangero-guest/containerd>
>> - There's a lintian warning about package-contains-vcs-control-file, but
>> until
>>   now I didn't find the right way to remove a file from installation. I
>> guess
>>   `override_dh_auto_install` is the way to go. Any hint welcome here.
>>
>
> Which file specifically is affected? The Files-Excluded directive in
> debian/copyright is a good way to exclude files.
>
>
> There's a `docs/.gitignore`, it's in a subdirectory. Files-Excluded will
> remove it from the orig tarball, right ? Is it really suitable here ?
>

Correct, and up to you (see the caveat which tincho expressed). Bug #812721
discusses a gbp import-ref command which would honor Files-Excluded, but
we’re not quite there yet. So, if you want to stick to orig tarballs for
the time being, Files-Excluded will work.


>
> For the moment the only package in which I exclude a file from
> installation is done this way:
>
> override_dh_auto_install:
>     dh_auto_install
>     find $(CURDIR)/debian -name '.tool' -type d -prune -exec rm -r '{}' +
>

nit: use find’s -delete action?


>
> Not really elegant, and it took me two hours to come up with that, after
> trying and failing every other way possible...
>
>
>
>>
>> #### golang-github-containerd-btrfs
>> <https://salsa.debian.org/elboulangero-guest/golang-github-c
>> ontainerd-btrfs>
>> <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890989>
>> - I disabled the test to avoid installing a btrfs-test binary, plus I
>> think the
>>   test failed for some reason, I'm not sure to remember though...
>>
>
> To avoid installing a binary, can you use the DH_GOLANG_EXCLUDES option
> please? See https://manpages.debian.org/testing/dh-golang/Debian::De
> bhelper::Buildsystem::golang.3pm.en.html
>
> Also, please add a comment stating why precisely the tests are disabled,
> and what needs to change so that we can re-enable them.
>
>
> Ok I'll try harder :)
>
> I noticed that DH_GOLANG_EXCLUDES will not save me from a test which fails
> though. For example, when I was working with containerd/cgroups, the test
> commmand invoked by dh looks like that:
>
>     go test -vet=off -v -p 8 github.com/containerd/cgroups
>
> DH_GOLANG_EXCLUDES is matched against github.com/containerd/cgroups, so
> if it contains a file inside the cgroups directory, it's not excluded from
> the files being tested.
>

DH_GOLANG_EXCLUDES, like the other directives, operates on Go packages, not
individual source code files. All files within github.com/containerd/cgroups
belong to the cgroups package, so you can only exclude it in its entirety,
which is not desired in this case.


>
> It means that if I want to prevent a file from the test, I still need to
> patch it with `// +build ignore`, if I understand properly.
>

Yes, if the whole file should be ignored. In case only individual tests are
affected, prefer using t.Skipf please:
https://golang.org/pkg/testing/#T.Skipf


>
>
>
>
>>
>> I see that most binaries come with the lintian warning
>> `statically-linked-binary`, I guess it's just the way it works in the
>> go world. Should I just add a lintian override ?
>>
>
> lintian does not emit the statically-linked-binary tag for Go packages.
> Can you point me to a specific example where you see it please?
>
>
> Yep, easy.
>
>   apt-get download gogoprotobuf
>   lintian gogoprotobuf_*_amd64.deb
>

Ah, I see what’s wrong here: you are supposed to run lintian on a .changes
file, so that it can inspect both the binary packages (*.deb) and the
source package (*.dsc). Indeed, when building gogoprotobuf and running
“lintian golang-gogoprotobuf_0.5-1_amd64.changes”, I don’t get any warnings
about statically linked binaries.


>
> Is this warning related to the field `Built-Using: ${misc:Built-Using}`
> for binary packages ? After reading the Debian Policy Manual, I seem to
> understand that this field is needed for go binary packages, am I correct ?
>

I don’t see a connection between the two fields. lintian examines the
Build-Depends field for detecting Go packages. Specifically, a package is a
Go package if golang-go or golang-any are present in Build-Depends.

Aside from that, yes, Built-Using must be set when building Go binary
packages, and must be unset for Go library packages. I have recently added
lintian warnings which cover these cases, but they’re not yet in a released
lintian version.


>
> However even with this field, there is still the warning in the
> `containerd` binary package that I built here, but only for *some* of the
> binaries in the package, not all of them.
>
>
>
>>
>> In the `-dev` packages, is `${shlibs:Depends}` needed ? During the builds
>> I see `unknown substitution variable ${shlibs:Depends}` passing by.
>>
>
> The messages implies it’s not required. I’m not entirely sure, but would
> suggest removing it for now.
>
>
> Ok. I think it should be also changed in dh-make-golang, should I look
> there and issue a PR, or do you prefer not to touch it for now ?
>

Please feel free to send a PR! :)


>
>
>
>
>>
>> In the `-dev` packages, do we really need to copy-paste all the
>> dependencies from the source package ?
>>
>
> Yes. They differ in some cases, for example when code generation needs
> more dependencies than the actual compilation/test (in which case the extra
> dependencies show up only in Build-Depends, not Depends).
>
>
> Ok...
>
> Thanks !
>
>   Arnaud
>



-- 
Best regards,
Michael
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pkg-go-maintainers/attachments/20180226/276e19b3/attachment-0001.html>