<div dir="ltr">I'd like gccgo to pass reliably, but I think it falls under a CI test rather than a pre-commit test. (Because otherwise it roughly doubles the time it takes to know whether your patch will land.)<div> </div><div>I don't have specific feedback on the behavior differences, but the changes seem sane to me. An errors library is certainly something that I'd like to see shared between code bases.</div><div> John</div> <div>=:-></div></div><div class="gmail_extra"> <div class="gmail_quote">On Wed, May 14, 2014 at 1:24 PM, Tim Penhey <<a href="mailto:tim.penhey@canonical.com" target="_blank">tim.penhey@canonical.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi all, I took it upon myself to get Rog's errgo library used inside juju-core. Dimiter recently did a hunk of work in the juju-core/errors package to have functions to add context to some core error types while keeping their type. What I did was to move this errors package out of juju-core and into its own package, and update it to use the errgo as a base. For those that are interested the diff for the errors_test (to show that everything works as before) is as shown at the bottom of this email. I also added in simple methods based on my earlier error handling package, but changed them to be based on errgo. This adds in the functions: New, Errorf, Check, Trace, Annotate, Annotatef, Wrap, ErrorStack. One key difference in behaviour between these methods and the base errgo ones is that the cause is always passed through. Unless you call Wrap, in which case the cause changes, but that is the purpose of the call. I have a branch of juju-core up as work in progress that does the following: * removes the dependency on <a href="http://github.com/errgo/errgo" target="_blank">github.com/errgo/errgo</a> * changes uses of errgo/errgo to juju/errors * adds both juju/errgo and juju/errors to the dependency file * removes the juju-core/errors package * changes all the juju-core/errors import statement to use <a href="http://github.com/juju/errors" target="_blank">github.com/juju/errors</a> (and puts the import statement in the right block) <a href="https://code.launchpad.net/~thumper/juju-core/juju-errors/+merge/219311" target="_blank">https://code.launchpad.net/~thumper/juju-core/juju-errors/+merge/219311</a> 3602 lines (+252/-658) 156 files modified I have tested juju/errors with both gc and gccgo and the tests pass with both. Interestingly juju/errgo has a few test failures with gccgo, but Dave is looking at them and assures me they are simple fixes. As an aside, I do think that any new dependencies we add should pass all tests with both gc and gccgo. Cheers, Tim $ diff -u ~/src/juju-core/trunk/errors/errors_test.go errors_test.go --- /home/tim/src/juju-core/trunk/errors/errors_test.go 2014-05-14 19:18:53.376121000 +1200 +++ errors_test.go 2014-05-13 17:14:50.329051637 +1200 @@ -1,5 +1,5 @@ -// Copyright 2013 Canonical Ltd. -// Licensed under the AGPLv3, see LICENCE file for details. +// Copyright 2013, 2014 Canonical Ltd. +// Licensed under the GPLv3, see LICENCE file for details. package errors_test @@ -8,12 +8,10 @@ "fmt" "reflect" "runtime" - "testing" + "<a href="http://github.com/juju/errors" target="_blank">github.com/juju/errors</a>" jc "<a href="http://github.com/juju/testing/checkers" target="_blank">github.com/juju/testing/checkers</a>" gc "<a href="http://launchpad.net/gocheck" target="_blank">launchpad.net/gocheck</a>" - - "<a href="http://launchpad.net/juju-core/errors" target="_blank">launchpad.net/juju-core/errors</a>" ) // errorInfo holds information about a single error type: a satisfier @@ -40,10 +38,6 @@ var _ = gc.Suite(&errorsSuite{}) -func Test(t *testing.T) { - gc.TestingT(t) -} - func (t *errorInfo) satisfierName() string { value := reflect.ValueOf(t.satisfier) f := runtime.FuncForPC(value.Pointer()) @@ -175,6 +169,12 @@ runErrorTests(c, errorTests, true) } +func (*errorsSuite) TestContextfNotNewer(c *gc.C) { + err := fmt.Errorf("simple error") + errors.Contextf(&err, "annotate") + c.Assert(err, gc.ErrorMatches, "annotate: simple error") +} + func (*errorsSuite) TestAllErrors(c *gc.C) { errorTests := []errorTest{} for _, errInfo := range allErrors { -- Juju-dev mailing list <a href="mailto:Juju-dev@lists.ubuntu.com">Juju-dev@lists.ubuntu.com</a> Modify settings or unsubscribe at: <a href="https://lists.ubuntu.com/mailman/listinfo/juju-dev" target="_blank">https://lists.ubuntu.com/mailman/listinfo/juju-dev</a> </blockquote></div> </div>