-
Notifications
You must be signed in to change notification settings - Fork 18
Streamline CMake and build system, move to newer protobuf #150
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
Changes from all commits
d92cbf9
412ad2f
396f3a0
8997f73
1f037c9
05f5f85
7be1d19
95c324c
4d3ebce
fb77da1
1b31d39
053b3e8
d58a570
33f5074
b7f89cb
3371d1e
d52238e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,35 +1,43 @@ | ||
| BUILD_CHANNEL?=local | ||
| TOOL_BIN = bin/gotools/$(shell uname -s)-$(shell uname -m) | ||
| PATH_WITH_TOOLS="`pwd`/$(TOOL_BIN):$(HOME)/go/bin/:${PATH}" | ||
| GIT_REVISION = $(shell git rev-parse HEAD | tr -d '\n') | ||
| TAG_VERSION?=$(shell git tag --points-at | sort -Vr | head -n1) | ||
| GO_BUILD_LDFLAGS = -ldflags "-X 'main.Version=${TAG_VERSION}' -X 'main.GitRevision=${GIT_REVISION}'" | ||
| TOOL_BIN := $(shell pwd)/bin/tools/$(shell uname -s)-$(shell uname -m) | ||
| GIT_REVISION := $(shell git rev-parse HEAD | tr -d '\n') | ||
| TAG_VERSION ?= $(shell git tag --points-at | sort -Vr | head -n1) | ||
| GO_BUILD_LDFLAGS := -ldflags "-X 'main.Version=${TAG_VERSION}' -X 'main.GitRevision=${GIT_REVISION}'" | ||
| SHELL := /usr/bin/env bash | ||
nicksanford marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| export PATH := $(TOOL_BIN):$(PATH) | ||
| export GOBIN := $(TOOL_BIN) | ||
|
|
||
| ifneq (, $(shell which brew)) | ||
| EXTRA_CMAKE_FLAGS := -DCMAKE_PREFIX_PATH=$(shell brew --prefix) -DQt5_DIR=$(shell brew --prefix qt5)/lib/cmake/Qt5 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [note to self] from build scripts |
||
| export PKG_CONFIG_PATH := $(shell brew --prefix openssl@3)/lib/pkgconfig | ||
JohnN193 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| endif | ||
|
|
||
| default: build | ||
|
|
||
| ARTIFACT="~/go/bin/artifact" | ||
| artifact-pull: $(TOOL_BIN)/artifact | ||
| artifact pull | ||
|
|
||
| artifact-pull: | ||
| PATH=${PATH_WITH_TOOLS} artifact pull | ||
| $(TOOL_BIN)/artifact: | ||
| go install go.viam.com/utils/artifact/cmd/artifact | ||
|
|
||
| bufinstall: | ||
| sudo apt-get install -y protobuf-compiler-grpc libgrpc-dev libgrpc++-dev || brew install grpc openssl --quiet | ||
| $(TOOL_BIN)/buf: | ||
| go install github.com/bufbuild/buf/cmd/buf@v1.8.0 | ||
|
|
||
| bufsetup: | ||
| GOBIN=`pwd`/grpc/bin go install github.com/bufbuild/buf/cmd/buf@v1.8.0 | ||
| ln -sf `which grpc_cpp_plugin` grpc/bin/protoc-gen-grpc-cpp | ||
| $(TOOL_BIN)/protoc-gen-grpc-cpp: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [q] what is the benefit of moving to this more complex make target name?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make targets are supposed to be real things. It's the whole reason it's called "make" in the first place, because it makes the thing. E.g. if I type
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gotcha, so thats why other repos were updated to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, though in that case, it was using a slightly different pattern where the "dist" folder contains touchfiles (just empty files with specific names) to basically flag when a job had been "done." You can then reset everything by just wiping those files out, causing a clean build to happen. Sometimes when you're linking in more complex tooling (that might have dozens of things it generates, like the buf generate command does) that's an easier way than setting up targets for EVERY output file. Especially if the output files often change or are variable in number. |
||
| mkdir -p "$(TOOL_BIN)" | ||
| which grpc_cpp_plugin && ln -sf `which grpc_cpp_plugin` $(TOOL_BIN)/protoc-gen-grpc-cpp | ||
|
|
||
| buf: bufsetup | ||
| PATH="${PATH}:`pwd`/grpc/bin" buf generate --template ./buf/buf.gen.yaml buf.build/viamrobotics/api | ||
| PATH="${PATH}:`pwd`/grpc/bin" buf generate --template ./buf/buf.grpc.gen.yaml buf.build/viamrobotics/api | ||
| PATH="${PATH}:`pwd`/grpc/bin" buf generate --template ./buf/buf.gen.yaml buf.build/googleapis/googleapis | ||
| buf: $(TOOL_BIN)/buf $(TOOL_BIN)/protoc-gen-grpc-cpp | ||
| buf generate --template ./buf/buf.gen.yaml buf.build/viamrobotics/api | ||
| buf generate --template ./buf/buf.grpc.gen.yaml buf.build/viamrobotics/api | ||
| buf generate --template ./buf/buf.gen.yaml buf.build/googleapis/googleapis | ||
|
|
||
| clean: | ||
| rm -rf grpc | ||
| rm -rf bin | ||
| rm -rf viam-cartographer/build | ||
| rm -rf viam-cartographer/cartographer/build | ||
| rm -rf grpc bin viam-cartographer/build | ||
|
|
||
| clean-all: | ||
| git clean -fxd | ||
| cd viam-cartographer/cartographer && git checkout . && git clean -fxd | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [note to self] resets CMakeList |
||
|
|
||
| ensure-submodule-initialized: | ||
| @if [ ! -d "viam-cartographer/cartographer/cartographer" ]; then \ | ||
|
|
@@ -38,21 +46,8 @@ ensure-submodule-initialized: | |
| else \ | ||
| echo "Submodule found successfully"; \ | ||
| fi | ||
|
|
||
| lint-setup-cpp: | ||
| ifeq ("Darwin", "$(shell uname -s)") | ||
| brew install clang-format | ||
| else | ||
| sudo apt-get install -y clang-format | ||
| endif | ||
|
|
||
| lint-setup-go: | ||
| GOBIN=`pwd`/$(TOOL_BIN) go install \ | ||
| github.com/edaniels/golinters/cmd/combined \ | ||
| github.com/golangci/golangci-lint/cmd/golangci-lint \ | ||
| github.com/rhysd/actionlint/cmd/actionlint | ||
|
|
||
| lint-setup: lint-setup-cpp lint-setup-go | ||
| grep -q viam-patched viam-cartographer/cartographer/CMakeLists.txt || \ | ||
jeremyrhyde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| (cd viam-cartographer/cartographer && git checkout . && git apply ../cartographer_patches/carto.patch) | ||
|
|
||
| lint-cpp: | ||
| find . -type f -not -path \ | ||
|
|
@@ -63,42 +58,46 @@ lint-cpp: | |
| -and \( -iname '*.h' -o -iname '*.cpp' -o -iname '*.cc' \) \ | ||
| | xargs clang-format -i --style="{BasedOnStyle: Google, IndentWidth: 4}" | ||
|
|
||
| lint-go: | ||
| lint-go: $(TOOL_BIN)/combined $(TOOL_BIN)/golangci-lint $(TOOL_BIN)/actionlint | ||
| go vet -vettool=$(TOOL_BIN)/combined ./... | ||
| GOGC=50 $(TOOL_BIN)/golangci-lint run -v --fix --config=./etc/golangci.yaml | ||
| PATH=$(PATH_WITH_TOOLS) actionlint | ||
| GOGC=50 golangci-lint run -v --fix --config=./etc/golangci.yaml | ||
| actionlint | ||
|
|
||
| $(TOOL_BIN)/combined $(TOOL_BIN)/golangci-lint $(TOOL_BIN)/actionlint: | ||
nicksanford marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| go install \ | ||
| github.com/edaniels/golinters/cmd/combined \ | ||
| github.com/golangci/golangci-lint/cmd/golangci-lint \ | ||
| github.com/rhysd/actionlint/cmd/actionlint | ||
|
|
||
| lint: ensure-submodule-initialized lint-cpp lint-go | ||
|
|
||
| setup: ensure-submodule-initialized | ||
| ifeq ("Darwin", "$(shell uname -s)") | ||
| cd viam-cartographer/scripts && ./setup_cartographer_macos.sh | ||
| setup: install-dependencies ensure-submodule-initialized artifact-pull | ||
|
|
||
| install-dependencies: | ||
| ifneq (, $(shell which brew)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [note to self] from setup scripts |
||
| brew update | ||
| brew install abseil boost ceres-solver protobuf ninja cairo googletest lua@5.3 pkg-config cmake go@1.20 grpc clang-format | ||
| brew link lua@5.3 | ||
| brew install openssl@3 eigen gflags glog suite-sparse sphinx-doc pcl nlopt-static | ||
| else ifneq (, $(shell which apt-get)) | ||
| $(warning "Installing cartographer external dependencies via APT.") | ||
| $(warning "Packages may be too old to work with this project.") | ||
| sudo apt-get update | ||
| sudo apt-get install -y cmake ninja-build libgmock-dev libboost-iostreams-dev liblua5.3-dev libcairo2-dev python3-sphinx libnlopt-dev \ | ||
| libabsl-dev libceres-dev libprotobuf-dev protobuf-compiler protobuf-compiler-grpc libpcl-dev libgrpc-dev libgrpc++-dev clang-format | ||
| else | ||
| cd viam-cartographer/scripts && ./setup_cartographer_linux.sh | ||
| endif | ||
| @make artifact-pull | ||
|
|
||
| build: build-module | ||
| ifneq ($(wildcard viam-cartographer/cartographer/build/.),) | ||
| cd viam-cartographer && ./scripts/build_viam_cartographer.sh | ||
| else | ||
| cd viam-cartographer && ./scripts/build_cartographer.sh && ./scripts/build_viam_cartographer.sh | ||
| $(error "Unsupported system. Only apt and brew currently supported.") | ||
| endif | ||
|
|
||
| build-debug: build-module | ||
| ifneq ($(wildcard viam-cartographer/cartographer/build/.),) | ||
| cd viam-cartographer && ./scripts/build_viam_cartographer_debug.sh | ||
| else | ||
| cd viam-cartographer && ./scripts/build_cartographer.sh && ./scripts/build_viam_cartographer_debug.sh | ||
| endif | ||
| build: ensure-submodule-initialized buf build-module | ||
| cd viam-cartographer && cmake -Bbuild -G Ninja ${EXTRA_CMAKE_FLAGS} && cmake --build build | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we dont have to run the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [q] so if we wanted separate build directories for different architectures, we would modify [nit] if so, could we add a comment here for this, as that's something the slam team wants to look into doing in the future?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, in theory you can just change -Bbuild to -Bbuild-`uname -m` (or whatever name scheme you want, and it'll configure the output there. Then you can actually compile with the same thing just changing build to build-`uname -m` in the final command as well. More realistically, you'd just want to make them variables in Make, then you can set the BUILD_DIR variable once and use everywhere. Do note that some of the cartographer tests in go are written using cgo and depending on explicit paths (which is why I had to change one) so there's not as easy of a way to handle that. Likewise, it's not just the cmake part that would need to be per-architecture, as the golang stuff would be as well, so it's likely a bit of a more complex change you really want, to completely split out architectures if you really want to be able to build both in the same code space (without just doing "make clean" instead.) |
||
|
|
||
| build-debug: EXTRA_CMAKE_FLAGS += -DCMAKE_BUILD_TYPE=Debug -DFORCE_DEBUG_BUILD=True | ||
| build-debug: build | ||
jeremyrhyde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| build-module: | ||
| mkdir -p bin && go build $(GO_BUILD_LDFLAGS) -o bin/cartographer-module module/main.go | ||
|
|
||
| install-lua-files: | ||
| sudo mkdir -p /usr/local/share/cartographer/lua_files/ | ||
| sudo cp viam-cartographer/lua_files/* /usr/local/share/cartographer/lua_files/ | ||
|
|
||
| test-cpp: | ||
| viam-cartographer/build/unit_tests -p -l all | ||
|
|
||
|
|
@@ -122,7 +121,11 @@ test-go: | |
|
|
||
| test: test-cpp test-go | ||
|
|
||
| install: | ||
| install-lua-files: | ||
| sudo mkdir -p /usr/local/share/cartographer/lua_files/ | ||
| sudo cp viam-cartographer/lua_files/* /usr/local/share/cartographer/lua_files/ | ||
|
|
||
| install: install-lua-files | ||
| sudo rm -f /usr/local/bin/carto_grpc_server | ||
| sudo rm -f /usr/local/bin/cartographer-module | ||
| sudo cp viam-cartographer/build/carto_grpc_server /usr/local/bin/carto_grpc_server | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[q] Is removing this a benefit of the (TOOL_BIN) additions made to the Makefile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's actually due to removing the "path:" setting above. The normal paradigm is to checkout in the working directory. Only when checking out multiple repos side-by-side do you usually need explicit paths for them. This was just normalizing things to what we do (almost) everywhere else.