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

make: build without code gen by default #3911

Merged
merged 3 commits into from
Oct 22, 2020

Conversation

matzf
Copy link
Contributor

@matzf matzf commented Oct 21, 2020

By default, make (or make build) only builds.
An explicit make all will additionally run all the code-generation
steps for the checked-in generated code, in the right order.

For the protobuf code generation step, the multiple bazel invocations
are combined to both improve readablity of the Makefile and the output,
and to make make run quiter.

Note that the order of the code generation steps is different than
before; previously protobuf forced itself to run before generating
go_deps.bzl -- this doesn't seem to make sense as the protobuf code
generation will never automatically affect the go.mod file. The other
way around is more conceivable, changes to go.mod propagating to
go_deps.bzl and so affecting the protobuf code generation.
Additionally, all now also includes the licenses target, just for
good measure.


This change is Reviewable

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)


Makefile, line 32 at r1 (raw file):

protobuf:
	bazel build //go/pkg/proto/control_plane:proto_srcs \

ok since you started simplifying I think we can go further:

	bazel build --output_groups=go_generated_srcs //go/pkg/proto/...
	rm -f go/pkg/proto/*/*.pb.go
	cp -r bazel-bin/go/pkg/proto/*/go_default_library_/github.com/scionproto/scion/go/pkg/proto/* go/pkg/proto
	chmod 0644 go/pkg/proto/*/*.pb.go

and then in BUILD.bazel files you can delete the filegroup and pkg_tar invocations (also don't forget to remove the load(pkg_tar)


Makefile, line 50 at r1 (raw file):

mocks:
	./tools/gomocks
	bazel run //:gazelle -- update -mode=$(GAZELLE_MODE) -index=false -external=external -exclude go/vendor -exclude docker/_build $(GAZELLE_DIRS)

delete this line it's not needed anymore. tools/gomocks manages it correctly

@lukedirtwalker lukedirtwalker self-assigned this Oct 22, 2020
Copy link
Contributor Author

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 8 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)


Makefile, line 32 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

ok since you started simplifying I think we can go further:

	bazel build --output_groups=go_generated_srcs //go/pkg/proto/...
	rm -f go/pkg/proto/*/*.pb.go
	cp -r bazel-bin/go/pkg/proto/*/go_default_library_/github.com/scionproto/scion/go/pkg/proto/* go/pkg/proto
	chmod 0644 go/pkg/proto/*/*.pb.go

and then in BUILD.bazel files you can delete the filegroup and pkg_tar invocations (also don't forget to remove the load(pkg_tar)

Noice! Done.


Makefile, line 50 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

delete this line it's not needed anymore. tools/gomocks manages it correctly

Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukedirtwalker
Copy link
Collaborator

/rebase

matzf added 3 commits October 22, 2020 12:57
By default, `make` (or `make build`) only builds.
An explicit `make all` will additionally run all the code-generation
steps for the checked-in generated code, in the right order.

For the protobuf code generation step, the multiple bazel invocations
are combined to both improve readablity of the Makefile and the output,
and to make make run quiter.

Note that the order of the code generation steps is different than
before; previously `protobuf` forced itself to run before generating
`go_deps.bzl` -- this doesn't seem to make sense as the protobuf code
generation will never automatically affect the `go.mod` file. The other
way around is more conceivable, changes to `go.mod` propagating to
`go_deps.bzl` and so affecting the protobuf code generation.
Additionally, `all` now also includes the `licenses` target, just for
good measure.
@lukedirtwalker lukedirtwalker merged commit 9978730 into scionproto:master Oct 22, 2020
@matzf matzf deleted the makefile branch November 12, 2020 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants