please upload new package to reenable go's race detector on wily

[Samuele Pedroni]

I've built a package that reenables the race detector on wily:

http://people.canonical.com/~mwh/go-race-detector-runtime/

This is the first time I've made a package from scratch so it'll probably need more than one round of review.

Previous description follows:

for example using lp:ubuntu-push/automatic ... and going into server/session I get (this was working in vivid and on wily before the 1.5rc1 upgrade)

$ go version
go version go1.5rc1 linux/amd64
$ go test -race
# testmain
runtime.raceinit: __tsan_init: not defined
runtime.raceinit: __tsan_map_shadow: not defined
runtime.racefini: __tsan_fini: not defined
runtime.racemapshadow: __tsan_map_shadow: not defined
runtime.racemalloc: __tsan_malloc: not defined
runtime.racegostart: __tsan_go_start: not defined
runtime.racegoend: __tsan_go_end: not defined
runtime.raceacquireg: __tsan_acquire: not defined
runtime.racereleaseg: __tsan_release: not defined
runtime.racereleasemergeg: __tsan_release_merge: not defined
runtime.racefingo: __tsan_finalizer_goroutine: not defined
runtime.RaceDisable: __tsan_go_ignore_sync_begin: not defined
runtime.RaceEnable: __tsan_go_ignore_sync_end: not defined
runtime.raceread: __tsan_read: not defined
runtime.racereadpc: __tsan_read_pc: not defined
runtime.racewrite: __tsan_write: not defined
runtime.racewritepc: __tsan_write_pc: not defined
runtime.racereadrange: __tsan_read_range: not defined
runtime.racereadrangepc1: __tsan_read_range: not defined
runtime.racewriterange: __tsan_write_range: not defined
runtime.racewriterangepc1: __tsan_write_range: not defined
/usr/lib/go/pkg/tool/linux_amd64/link: too many errors
FAIL launchpad.net/ubuntu-push/server/session [build failed]

[Michael Hudson-Doyle (mwhudson)]

I've just uploaded a fix that means the package now builds in a clean environment.

[Michael Hudson-Doyle (mwhudson)]

And another one fixed to avoid stripping the syso (which stops it working!)

[Martin Pitt (pitti)]

Packaging review:
 - Please use 3.0 (quilt) source format for clean and standard patch management.

 - Disabling "make check" (override_dh_auto_test) is a bit sad -- when running "make check" it fails with "unable to infer compiler target triple for clang". Is there any way to run the tests with gcc? If not, should this get a clang build dependency? If that doesn't work, please add a comment to debian/rules why tests can't/shouldn't be run.

 - W: go-race-detector-runtime source: dep5-copyright-license-name-not-unique (paragraph at line 35)
   Indeed you used "License: Expat" with two different license texts, AFAICS mostly due to including the copyright into it (which shouldn't happen -- that's what Copyright: is for). It's usually best to just say "License: Expat" in all Files: stanzas, and have a separate

  License: expat
    ..blabla long text

  stanza at the bottom, to avoid repetition.

 - There are several missing copyrights in debian/copyright, e. g. ./lib/BlocksRuntime/Block.h has "Copyright 2008-2010 Apple, Inc." and the license is BSD.

[Martin Pitt (pitti)]

Please re-subscribe ~ubuntu-sponsors once you have a fixed package. Thanks!

[Michael Hudson-Doyle (mwhudson)]

Thanks for the review! Part of the reason that this package is a bit
confusing is that it is only building a small part of the source it
includes. I added a debian/README.Debian file explaining what is going
on:

======================================================
Runtime support for Go's race detector
--------------------------------------

This package follows the instructions in the Go source:

    https://go.googlesource.com/go/+/master/src/runtime/race/README

that describe how to build the race detector runtime support from
source. The race detector depends on runtime support from the
"compiler-rt" runtime libraries (http://compiler-rt.llvm.org/),
specifically the ThreadSanitizer (aka "tsan") library. The Go source
distribution includes this support as a built object file which
obviously violates the principle of knowing that the binaries are
actually produced from the source they claim to be, hence this
package.

The compiler-rt source is included in other source packages (at least
gcc and llvm-toolchain) but I created one specifically for this task
so that we can use the same revision of the compiler-rt source as the
currently packaged version of Go.

This does mean that building this package only uses a relatively small
part of the source that it distributes -- for example, it doesn't
execute the full testsuite, just the small part that is run by the
buildgo.sh script.
======================================================

Do things make a bit more sense now?

On 18 September 2015 at 17:52, Martin Pitt <email address hidden> wrote:
> Packaging review:
> - Please use 3.0 (quilt) source format for clean and standard patch management.

I thought I had done that but indeed not. Fixed.

> - Disabling "make check" (override_dh_auto_test) is a bit sad -- when
> running "make check" it fails with "unable to infer compiler target
> triple for clang". Is there any way to run the tests with gcc? If not,
> should this get a clang build dependency? If that doesn't work, please
> add a comment to debian/rules why tests can't/shouldn't be run.

I hope README.Debian explains this now?

> - W: go-race-detector-runtime source: dep5-copyright-license-name-not-unique (paragraph at line 35)

I don't get this warning, is it new in wily?

> Indeed you used "License: Expat" with two different license texts, AFAICS mostly due to including the copyright into it (which shouldn't happen -- that's what Copyright: is for). It's usually best to just say "License: Expat" in all Files: stanzas, and have a separate
>
> License: expat
> ..blabla long text
>
> stanza at the bottom, to avoid repetition.

OK, updated that.

> - There are several missing copyrights in debian/copyright, e. g.
> ./lib/BlocksRuntime/Block.h has "Copyright 2008-2010 Apple, Inc."

Well I went looking and found that and one other (cpplint.py which is
(c) google). Is there a way to be sure you've got them all? (I grep
-riF for "(c)" and "copyright").

> and the license is BSD.

Nope, still Expat :-) (verified with wdiff)

Uploaded a new package, please take another look.

[Martin Pitt (pitti)]

debian/control was a bit messed up -- the Source: stanza can't have a "Description:" field. Also, short description shouldn't end with a dot. It should also add the LP bug ref to debian/changelog. Attaching a debdiff for this, please apply.

However these are blockers:

 - an upstream version of "229396" is huge, possibly not monotonous (is that a git commit ref?), and hard to make higher if this is more like a git commit hash than a monotonously increasing number.

 - There is also no indication where the orig.tar.gz came from -- this should either come from a documented upstream release tarball URL, usually with debian/watch; or should get a "debian/rules get-orig-source" rule or at least some debian/README.Source that documents how to get it or any newer version.

[Martin Pitt (pitti)]

Oh, if this is a non-monotonous git hash, please rather use time based versions, or a combination like 20151006+git123ABC

[Martin Pitt (pitti)]

Sorry, I mean 0.20151006+git123ABC, so that you can effortlessly go to "real" version numbers if upstream ever switches to those.

[Michael Hudson-Doyle (mwhudson)]

On 6 October 2015 at 22:34, Martin Pitt <email address hidden> wrote:
> debian/control was a bit messed up -- the Source: stanza can't have a
> "Description:" field. Also, short description shouldn't end with a dot.
> It should also add the LP bug ref to debian/changelog. Attaching a
> debdiff for this, please apply.

Will do thanks.

> However these are blockers:
>
> - an upstream version of "229396" is huge, possibly not monotonous (is
> that a git commit ref?), and hard to make higher if this is more like a
> git commit hash than a monotonously increasing number.

It's a subversion revision number. I guess 0.0+svn229396 would be better?

> - There is also no indication where the orig.tar.gz came from -- this
> should either come from a documented upstream release tarball URL,
> usually with debian/watch; or should get a "debian/rules get-orig-
> source" rule or at least some debian/README.Source that documents how to
> get it or any newer version.

Ah yes, fair points.

>
> ** Patch added: "packaging fixes"
> https://bugs.launchpad.net/ubuntu/+source/golang/+bug/1487010/+attachment/4485683/+files/go-race-detector-runtime.packagingfixes.debdiff
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1487010
>
> Title:
> please upload new package to reenable go's race detector on wily
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/ubuntu/+source/golang/+bug/1487010/+subscriptions

[Michael Hudson-Doyle (mwhudson)]

Uploaded a new version to http://people.canonical.com/~mwh/go-race-detector-runtime/ with the following changes:

1) Applied your debdiff
2) Changed the package name to golang-race-detector-runtime for consistency with, well, golang
3) Changed the upstream version number to 0.0+svn229396
4) Added a get-orig-source target and used it to repack the orig tarball (I verified that the file contents are the same as the previous version)

Thanks again for the reviews, feels close now?

[Martin Pitt (pitti)]

Thanks, looks good now! I uploaded this with dropping the subversion build dependency again, as it's only needed for get-orig-source, not the actual build.

[Launchpad Janitor (janitor)]

This bug was fixed in the package golang-race-detector-runtime - 0.0+svn229396-0ubuntu1

---------------
golang-race-detector-runtime (0.0+svn229396-0ubuntu1) wily; urgency=medium

  * Initial release. (LP: #1487010)

 -- Michael Hudson-Doyle <email address hidden> Thu, 10 Sep 2015 13:00:00 +1200