Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add go module & set import path to capnproto.org/go/capnp/v3 #169

Merged
merged 6 commits into from
May 12, 2021

Conversation

lthibault
Copy link
Collaborator

Fixes #166.

  • Add go.mod with import path as capnproto.org/go/capnp/v3
  • Update import paths in all files

@lthibault lthibault added this to the 3.0 milestone May 12, 2021
@lthibault lthibault self-assigned this May 12, 2021
@lthibault
Copy link
Collaborator Author

$ go test ./...
--- FAIL: TestMarshalPackedShouldMatchTextWhenDecoded (0.14s)
    integration_test.go:762: zdataFilledMessage(20): decoded to:
        "(zdata = (data = \"\\000\\001\\002\\003\\004\\005\\006\\a\\b\\t\\n\\v\\f\\r\\016\\017\\020\\021\\022\\023\"))\n"; want:
        "(zdata = (data = \"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\a\\b\\t\\n\\v\\f\\r\\x0e\\x0f\\x10\\x11\\x12\\x13\"))\n"
--- FAIL: TestMarshalShouldMatchTextWhenDecoded (0.15s)
    integration_test.go:739: zdataFilledMessage(20): decoded to:
        "(zdata = (data = \"\\000\\001\\002\\003\\004\\005\\006\\a\\b\\t\\n\\v\\f\\r\\016\\017\\020\\021\\022\\023\"))\n"; want:
        "(zdata = (data = \"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\a\\b\\t\\n\\v\\f\\r\\x0e\\x0f\\x10\\x11\\x12\\x13\"))\n"
FAIL
FAIL    capnproto.org/go/capnp/v3       0.336s
--- FAIL: TestRemoteScope (0.00s)
    capnpc-go_test.go:217: different file struct: g.RemoteTypeName(nodes[0x836faf1834d91729].Const().Type(), nodes[0x836faf1834d91729]); g.imports = otherscopes "zombiezen.com/go/capnproto2/capnpc-go/testdata/otherscopes"; want otherscopes "capnproto.org/go/capnp/v3/capnpc-go/testdata/otherscopes"
    capnpc-go_test.go:217: different file struct list: g.RemoteTypeName(nodes[0x83e7e1b3cd1be338].Const().Type(), nodes[0x83e7e1b3cd1be338]); g.imports = otherscopes "zombiezen.com/go/capnproto2/capnpc-go/testdata/otherscopes"; want otherscopes "capnproto.org/go/capnp/v3/capnpc-go/testdata/otherscopes"
    capnpc-go_test.go:236: different file struct: g.RemoteTypeNew(nodes[0x836faf1834d91729].Const().Type(), nodes[0x836faf1834d91729]); g.imports = otherscopes "zombiezen.com/go/capnproto2/capnpc-go/testdata/otherscopes"; want otherscopes "capnproto.org/go/capnp/v3/capnpc-go/testdata/otherscopes"
    capnpc-go_test.go:236: different file struct list: g.RemoteTypeNew(nodes[0x83e7e1b3cd1be338].Const().Type(), nodes[0x83e7e1b3cd1be338]); g.imports = otherscopes "zombiezen.com/go/capnproto2/capnpc-go/testdata/otherscopes"; want otherscopes "capnproto.org/go/capnp/v3/capnpc-go/testdata/otherscopes"
FAIL
FAIL    capnproto.org/go/capnp/v3/capnpc-go     0.636s
FAIL

@zenhack Is this a known issue? (i.e. should I worry about it?)

@lthibault lthibault linked an issue May 12, 2021 that may be closed by this pull request
@lthibault lthibault marked this pull request as ready for review May 12, 2021 16:07
@zenhack
Copy link
Contributor

zenhack commented May 12, 2021

@lthibault, I think you forgot to re-generate some test data:

[isd@elf go-capnproto2]$ git grep zombiezen\.com/go
Binary file capnpc-go/testdata/aircraft.capnp.out matches
Binary file capnpc-go/testdata/const.capnp.out matches
Binary file capnpc-go/testdata/group.capnp.out matches
Binary file capnpc-go/testdata/rpc.capnp.out matches
Binary file capnpc-go/testdata/scopes.capnp.out matches
Binary file capnpc-go/testdata/util.capnp.out matches

@lthibault
Copy link
Collaborator Author

@lthibault, I think you forgot to re-generate some test data:

@zenhack Right on. What's the process for regenerating test data? I can't seem to find anything 🤔

@zenhack
Copy link
Contributor

zenhack commented May 12, 2021

I'm not sure what the official way to regenerate those is; @zombiezen?

I assume they're just the output of capnp compile -o- /path/to/schema.capnp > schema.capnp.out, with appropriate flags.

@lthibault
Copy link
Collaborator Author

lthibault commented May 12, 2021

Alright, I've manually regenerated the test data for capnpc-go/testdata, and that seems to work well.

Concerning the errors in integration_test.go, the test data appears to be valid.

{
    name: "zdataFilledMessage(20)",
    msg:  zdataFilledMessage(t, 20),
    typ:  "Z",
    text: `(zdata = (data = "\x00\x01\x02\x03\x04\x05\x06\a\b\t\n\v\f\r\x0e\x0f\x10\x11\x12\x13"))` + "\n",
    data: []byte{
        0, 0, 0, 0, 9, 0, 0, 0,
        0, 0, 0, 0, 3, 0, 1, 0,
        28, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 1, 0,
        1, 0, 0, 0, 162, 0, 0, 0,
	0, 1, 2, 3, 4, 5, 6, 7,
	8, 9, 10, 11, 12, 13, 14, 15,
	16, 17, 18, 19, 0, 0, 0, 0,
    },
},

Updating the text field appears to solve the issue.

Old: \x00\x01\x02\x03\x04\x05\x06\a\b\t\n\v\f\r\x0e\x0f\x10\x11\x12\x13
New: \000\001\002\003\004\005\006\a\b\t\n\v\f\r\016\017\020\021\022\023

I'm not sure I understand the root cause. Has Cap'n Proto's representation of binary data recently changed or something (I'm using versoin 0.8.0)?

@zenhack
Copy link
Contributor

zenhack commented May 12, 2021

The text field is what the test expects capnp decode to output, so my guess is that at some point since this test was written, that command starting using octal escaping instead of hex when formatting (@kentonv?)

Anyway, LGTM.

@lthibault
Copy link
Collaborator Author

lthibault commented May 12, 2021

@zenhack Yeah, that was my instinct as well and I've managed to reproduce the output at the command line:

$ cat /tmp/zdataFilled20.out | capnp convert binary:text internal/aircraftlib/aircraft.capnp Z
( zdata = (
    data = "\000\001\002\003\004\005\006\a\b\t\n\v\f\r\016\017\020\021\022\023" ) )

Merging now. Thanks for your help!

@lthibault lthibault merged commit efc3abd into v3 May 12, 2021
@kentonv
Copy link
Member

kentonv commented May 13, 2021

Looks like the switch from hex to octal happened here: capnproto/capnproto@03800df

Looks like it was not entirely advertent. But, hex escapes do have the drawback that in many languages (including C/C++), they are technically not limited to two digits, and so a string like "\x012" is technically equivalent to "\x12". This makes it really hard to output an arbitrary string with hex escapes correctly, since any valid hex digit appearing immediately after an escape has to be escaped, too. Octal escapes, on the other hand, are never more than three digits, so that's much easier to deal with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change import path to capnproto.org/go/capnp
3 participants