Keep generated go files in the git repo #2907
Conversation
matzf
force-pushed
the
goprotogen
branch
8 times, most recently
from
bc0f73c
to
c01085c
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 26 files reviewed, 1 unresolved discussion (waiting on @matzf)
a discussion (no related file):
On my workstation, running make gogen on this commit causes all capnp.go files' schema_XXX vars to change.
capnproto/go-capnp#145 implies that this shouldn't happen, so long as the env is the ~same.
Here's what i have:
$ ./bazel-scion/external/go_sdk/bin/go version
go version go1.11.5 linux/amd64
$ uname -a
Linux <redacted> 4.4.0-151-generic #178-Ubuntu SMP Tue Jun 11 08:30:22 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
$ dpkg -l capnproto
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name Version Architecture Description
+++-==============================================-============================-============================-==================================================================================================
ii capnproto 0.5.3-2ubuntu1.1 amd64 tool for working with the Cap'n Proto data interchange format
There was a problem hiding this comment.
Reviewable status: 0 of 26 files reviewed, 1 unresolved discussion (waiting on @matzf)
a discussion (no related file):
Previously, kormat (Stephen Shirley) wrote…
On my workstation, running
make gogenon this commit causes allcapnp.gofiles' schema_XXX vars to change.capnproto/go-capnp#145 implies that this shouldn't happen, so long as the env is the ~same.
Here's what i have:
$ ./bazel-scion/external/go_sdk/bin/go version go version go1.11.5 linux/amd64 $ uname -a Linux <redacted> 4.4.0-151-generic #178-Ubuntu SMP Tue Jun 11 08:30:22 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux $ dpkg -l capnproto Desired=Unknown/Install/Remove/Purge/Hold | Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad) ||/ Name Version Architecture Description +++-==============================================-============================-============================-================================================================================================== ii capnproto 0.5.3-2ubuntu1.1 amd64 tool for working with the Cap'n Proto data interchange format
$ capnp compile -ocapnp proto/zkid.capnp
# proto/zkid.capnp
@0xc4f0db62ff503b7d;
$import "/proto/go.capnp".package("proto");
$import "/proto/go.capnp".import("github.com/scionproto/scion/go/proto");
struct ZkId @0xfaadb66890d8d665 { # 8 bytes, 2 ptrs
isdas @0 :UInt64; # bits[0, 64)
id @1 :Text; # ptr[0]
addrs @2 :List(Addr); # ptr[1]
}
struct Addr @0x8982abce7c93c140 { # 8 bytes, 1 ptrs
type @0 :UInt8; # bits[0, 8)
addr @1 :Data; # ptr[0]
port @2 :UInt16; # bits[16, 32)
}
There was a problem hiding this comment.
Reviewable status: 0 of 26 files reviewed, 1 unresolved discussion (waiting on @matzf)
a discussion (no related file):
Previously, kormat (Stephen Shirley) wrote…
$ capnp compile -ocapnp proto/zkid.capnp # proto/zkid.capnp @0xc4f0db62ff503b7d; $import "/proto/go.capnp".package("proto"); $import "/proto/go.capnp".import("github.com/scionproto/scion/go/proto"); struct ZkId @0xfaadb66890d8d665 { # 8 bytes, 2 ptrs isdas @0 :UInt64; # bits[0, 64) id @1 :Text; # ptr[0] addrs @2 :List(Addr); # ptr[1] } struct Addr @0x8982abce7c93c140 { # 8 bytes, 1 ptrs type @0 :UInt8; # bits[0, 8) addr @1 :Data; # ptr[0] port @2 :UInt16; # bits[16, 32) }
$ which capnp
/usr/bin/capnp
$ capnp --version
Cap'n Proto version 0.5.3
There was a problem hiding this comment.
Reviewable status: 0 of 26 files reviewed, 1 unresolved discussion (waiting on @matzf)
a discussion (no related file):
Previously, kormat (Stephen Shirley) wrote…
$ which capnp /usr/bin/capnp $ capnp --version Cap'n Proto version 0.5.3
PR on hold until capnproto/go-capnp#145 is resolved.
There was a problem hiding this comment.
Reviewed 5 of 26 files at r1, 21 of 21 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @matzf)
Makefile, line 9 at r2 (raw file):
SRC_DIRS = c/lib/scion c/lib/filter c/dispatcher all: tags clibs dispatcher bazel gogen
The order here is wrong. If i modify a .capnp file, the changes won't make it to binaries until i run make twice. gogen must come before bazel.
scion.sh, line 349 at r2 (raw file):
if [ -n "$out" ]; then echo "$out"; ret=1; fi lint_step "misspell" xargs -a $TMPDIR/gofiles.list $TMPDIR/misspell -error || ret=1
This sucks a bit, as it can fail out on the first batch of files, and therefore fail to flag errors in subsequent batches in a single run.
docker/perapp/base/Dockerfile.builder, line 2 at r2 (raw file):
FROM scion:latest RUN make -s all setcap GOGEN_SKIP=1 && bazel clean
make vars usually come before targets.
go/proto/BUILD.bazel, line 26 at r2 (raw file):
) # Note: :structs and :capnp are only used to manually regenerate git-controlled
I'm wondering whether there's any benefit at all to having these as bazel targets, seeing as bazel can't use them, and they're only triggered from make. It would make it a lot easier to understand if they were generated outside of bazel.
go/proto/BUILD.bazel, line 27 at r2 (raw file):
# Note: :structs and :capnp are only used to manually regenerate git-controlled # source files. These targets are no dependencies of other bazel targets.
"are not"
tools/ci/checkgogen, line 5 at r2 (raw file):
set -ex ./docker.sh exec "set -eo pipefail; ( cp -R go/proto/ /tmp/; make gogen; diff /tmp/proto/ go/proto/ ) |& tee logs/checkgogen.run"
Use diff -ur (-u to make the output human-parsable, -r in case there are ever subdirs added)
There was a problem hiding this comment.
Reviewable status: 21 of 26 files reviewed, 6 unresolved discussions (waiting on @kormat)
Makefile, line 9 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
The order here is wrong. If i modify a .capnp file, the changes won't make it to binaries until i run
maketwice.gogenmust come beforebazel.
Done.
scion.sh, line 349 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This sucks a bit, as it can fail out on the first batch of files, and therefore fail to flag errors in subsequent batches in a single run.
Fixed, but it's gotten a bit obscure.
docker/perapp/base/Dockerfile.builder, line 2 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
make vars usually come before targets.
Done.
go/proto/BUILD.bazel, line 26 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'm wondering whether there's any benefit at all to having these as bazel targets, seeing as bazel can't use them, and they're only triggered from
make. It would make it a lot easier to understand if they were generated outside of bazel.
The idea was to (re-)use bazel for managing the required tools, i.e. install capnp-go. Also, I thought it might be a good to keep the diff in the BUILD file small to be able to change back to generating files during the bazel build with only small changes.
Now I just saw that for the linting, managing the tools is done using a special bazel target that creates a tar with the required binaries. Would you prefer this approach for the capnp-go binaries?
go/proto/BUILD.bazel, line 27 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
"are not"
Done.
tools/ci/checkgogen, line 5 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Use
diff -ur(-u to make the output human-parsable, -r in case there are ever subdirs added)
Done.
matzf
force-pushed
the
goprotogen
branch
2 times, most recently
from
7c1eb32
to
0ad706f
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)
scion.sh, line 349 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
Fixed, but it's gotten a bit obscure.
Apologies - i just re-read the manpage for xargs. It will run everything (unless something exits with 255), and then return a result at the end: http://man7.org/linux/man-pages/man1/xargs.1.html#EXIT_STATUS
So your original version was fine. Sorry :)
go/proto/BUILD.bazel, line 26 at r2 (raw file):
The idea was to (re-)use bazel for managing the required tools, i.e. install capnp-go.
For sure (i wasn't suggesting otherwise).
Would you prefer this approach for the capnp-go binaries?
That actually makes a lot of sense. We could have a build-tool-type tarball with capnpc-go, mockgen etc. That would be a lot cleaner than where we are currently.
There was a problem hiding this comment.
Reviewable status: 23 of 28 files reviewed, 1 unresolved discussion (waiting on @kormat)
scion.sh, line 349 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Apologies - i just re-read the manpage for
xargs. It will run everything (unless something exits with255), and then return a result at the end: http://man7.org/linux/man-pages/man1/xargs.1.html#EXIT_STATUSSo your original version was fine. Sorry :)
No worries. Reverted.
go/proto/BUILD.bazel, line 26 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
The idea was to (re-)use bazel for managing the required tools, i.e. install capnp-go.
For sure (i wasn't suggesting otherwise).
Would you prefer this approach for the capnp-go binaries?
That actually makes a lot of sense. We could have a build-tool-type tarball with
capnpc-go,mockgenetc. That would be a lot cleaner than where we are currently.
Done (not for mockgen).
There was a problem hiding this comment.
Reviewed 4 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)
BUILD.bazel, line 69 at r5 (raw file):
) # This is a package of tools used for generating code that is checked in to git (i.e. outside of bazels reach).
Wrap comment.
go/proto/Makefile, line 10 at r5 (raw file):
capnp: $(eval $@_TMPDIR := $(shell mktemp -d -t scion-build-tools.XXXXXXXXXX))
It's more normal to use a multi-line step. Something like:
set -eu; \
TMPDIR=$$(mktemp -d -t scion-build-tools.XXXXXXXXXX); \
bazel build ...; \
...
rm -r "$TMPDIR"
See https://github.com/scionproto/scion/blob/master/docker/perapp/Makefile#L14 for an example of where we do this currently.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)
go/proto/Makefile, line 10 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
It's more normal to use a multi-line step. Something like:
set -eu; \ TMPDIR=$$(mktemp -d -t scion-build-tools.XXXXXXXXXX); \ bazel build ...; \ ... rm -r "$TMPDIR"See https://github.com/scionproto/scion/blob/master/docker/perapp/Makefile#L14 for an example of where we do this currently.
(That should have been rm -r "$$TMPDIR")
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf)
Makefile, line 9 at r5 (raw file):
SRC_DIRS = c/lib/scion c/lib/filter c/dispatcher all: tags clibs dispatcher gogen bazel
Thinking about this further, i'd prefer to have gogen be a dependency of the bazel: rule. That way running make bazel would still guarantee a consistent tree.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kormat)
BUILD.bazel, line 69 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Wrap comment.
Done.
Makefile, line 9 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Thinking about this further, i'd prefer to have
gogenbe a dependency of thebazel:rule. That way runningmake bazelwould still guarantee a consistent tree.
Done.
go/proto/Makefile, line 10 at r5 (raw file):
Previously, kormat (Stephen Shirley) wrote…
(That should have been
rm -r "$$TMPDIR")
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)
Makefile, line 30 at r6 (raw file):
bzlcompat -vendorBase=go bazel: gogen vendor bazel_bin_clean
Let's move it after vendor, as future gogen tools might rely on this being up-to-date.
There was a problem hiding this comment.
Reviewable status: 27 of 28 files reviewed, 1 unresolved discussion (waiting on @kormat)
Makefile, line 30 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Let's move it after
vendor, as future gogen tools might rely on this being up-to-date.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7.
Reviewable status:complete! all files reviewed, all discussions resolved
Add the generated *.capnp.go and structs.gen.go files in go/proto to version control. This intends to simplify the build process when using the go toolchain, especially for go projects that consume scion as a library. Add make target 'gogen' to update the generated files in go/proto. Use new bazel pkg_tar rule 'build-tools' to compile canpnpc-go for generating the *.capnpc.go files. Add CI check for generated go/proto files; detect uncommitted changes to generated go/proto files by cleaning and regenerating the files.
#2878 again, was reverted in #2902.
Ignore non-deterministic schema_ in *.capnp.go files when comparing with generated files.
This change is