[dependency] Migrate to hyperledger/SmartBFT - #201
Conversation
b3a81c5 to
24a57f7
Compare
|
coverage: 82.023% (+0.02%) from 82.004% — tock-ibm:smartbft-migrate into hyperledger:main |
24a57f7 to
57fc0c2
Compare
Signed-off-by: Yoav Tock <TOCK@il.ibm.com>
57fc0c2 to
b78ea88
Compare
liran-funaro
changed the title
liran-funaro
changed the title
| @PATH="$(call go_tool_dir,counterfeiter):$(call go_tool_dir,protoc-gen-go):$(call go_tool_dir,protoc-gen-go-grpc):$$PATH" \ | ||
| COUNTERFEITER_NO_GENERATE_WARNING=true go generate ./... |
There was a problem hiding this comment.
minor: the PATH can be pre calculated as a variable, then this will be a single row.
This will improve readability.
There was a problem hiding this comment.
Done in 037e4dd: the PATH is now a mocks_path variable, so the recipe is a single line.
| project_dir := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST)))) | ||
| proto_flags ?= | ||
| fabric_protos_tag ?= $(shell go list -m -f '{{.Version}}' github.com/hyperledger/fabric-protos-go-apiv2) | ||
| protoc_version ?= $(shell protoc --version 2>/dev/null | awk '{print $$2}') |
There was a problem hiding this comment.
minor: this breaks for protoc 3.21.x, which is what Ubuntu 24.04 / Debian 12 ship via apt. It reports libprotoc 3.21.12, but the release tag is v21.12, so the includes download returns 404 (v3.21.12 → 404, v21.12 → 200). Older releases (v3.20.3, v3.19.4) keep the 3. prefix in the tag, and from v22 on the reported version matches the tag, so 3.21.x is the only mismatch.
Two options:
- Map 3.21.x to its tag:
| protoc_version ?= $(shell protoc --version 2>/dev/null | awk '{print $$2}') | |
| protoc_version ?= $(shell protoc --version 2>/dev/null | awk '{v=$$2; sub(/^3\.21\./,"21.",v); print v}') |
- Always download the includes for the protoc version pinned in
scripts/install-dev-dependencies.sh(35.1), instead of detecting the installed one. This removes the version mapping entirely, but the includes may not match the protoc that's actually installed.
There was a problem hiding this comment.
Confirmed (v3.21.12 → 404, v21.12 → 200). Went with option 1 in 037e4dd, plus a comment explaining the mapping. Option 2 could pair the includes with a different protoc than the one installed, so I kept the version detection.
| @mkdir -p ${BUILD_DIR} | ||
| @rm -rf ${PROTOC_INCLUDE_DIR} ${PROTOC_INCLUDE_DIR}.zip | ||
| @curl -fsSL -o ${PROTOC_INCLUDE_DIR}.zip ${PROTOC_INCLUDE_URL} | ||
| @unzip -q ${PROTOC_INCLUDE_DIR}.zip 'include/*' -d ${PROTOC_INCLUDE_DIR}.tmp | ||
| @mv ${PROTOC_INCLUDE_DIR}.tmp/include ${PROTOC_INCLUDE_DIR} | ||
| @rm -rf ${PROTOC_INCLUDE_DIR}.tmp ${PROTOC_INCLUDE_DIR}.zip |
There was a problem hiding this comment.
nit: we can simplify the download to two commands. curl --create-dirs replaces the mkdir, and extracting the zip in place removes the .tmp directory, the mv and both rm -rf. unzip -o overwrites any leftover files, and the zip stays in the git-ignored .build/protoc@<version>/ directory, which make clean-deps removes.
| @mkdir -p ${BUILD_DIR} | |
| @rm -rf ${PROTOC_INCLUDE_DIR} ${PROTOC_INCLUDE_DIR}.zip | |
| @curl -fsSL -o ${PROTOC_INCLUDE_DIR}.zip ${PROTOC_INCLUDE_URL} | |
| @unzip -q ${PROTOC_INCLUDE_DIR}.zip 'include/*' -d ${PROTOC_INCLUDE_DIR}.tmp | |
| @mv ${PROTOC_INCLUDE_DIR}.tmp/include ${PROTOC_INCLUDE_DIR} | |
| @rm -rf ${PROTOC_INCLUDE_DIR}.tmp ${PROTOC_INCLUDE_DIR}.zip | |
| @curl -fsSL --create-dirs -o ${PROTOC_INCLUDE_ROOT}/protoc.zip ${PROTOC_INCLUDE_URL} | |
| @unzip -qo ${PROTOC_INCLUDE_ROOT}/protoc.zip 'include/*' -d ${PROTOC_INCLUDE_ROOT} |
This needs the include dir to point at the include/ folder inside the extracted zip (lines 116-117):
PROTOC_INCLUDE_ROOT := ${BUILD_DIR}/protoc@${protoc_version}
PROTOC_INCLUDE_DIR := ${PROTOC_INCLUDE_ROOT}/include
PROTOC_INCLUDE_SENTINEL := ${PROTOC_INCLUDE_DIR}/google/protobuf/descriptor.protoTrade-off: we lose the atomic .tmp + mv. If unzip is interrupted after writing descriptor.proto, later runs skip the download until .build/ is removed. That is unlikely, since the extraction takes under a second.
There was a problem hiding this comment.
Done in 037e4dd, with the PROTOC_INCLUDE_ROOT / PROTOC_INCLUDE_DIR split you suggested. One note: make clean-deps only removed the fabric-protos clone before, so I added ${PROTOC_INCLUDE_ROOT} to it as well.
- Map protoc 3.21.x to its release tag (v21.N) when fetching the well-known types. - Simplify the download to `curl --create-dirs` + `unzip -o` in place. - `make clean-deps` also removes the downloaded protoc includes. - Pre-compute the `make mocks` PATH in a `mocks_path` variable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yoav Tock <TOCK@il.ibm.com>
3 participants
Type of change
Description
github.com/hyperledger-labs/SmartBFTv1.0.1 with its new homegithub.com/hyperledger/SmartBFTv1.0.2, and bump testify, yaml, x/sync andgoogle.golang.org/protobuf(v1.36.11 -> v1.36.12).*.pb.gofiles; only theprotoc-gen-goversion header changes.make protono longer depends on the host setup:google/protobuf/*.proto) matching the installedprotoc into
.build/, instead of relying on/usr/includefromlibprotobuf-dev(which is unavailable on non-Debian hosts, e.g. RHEL);
protoc-gen-go/protoc-gen-go-grpcfrom thego.modtool directives viago tool, instead of whatever is inPATH.make mocksputs thego.modversions ofcounterfeiter,protoc-gen-goandprotoc-gen-go-grpcfirst inPATH, sogo generateoutput matchesmake proto.make lint-protorunsapi-linterviago tool.libprotobuf-devfromscripts/install-dev-dependencies.sh.Additional details (Optional)
Verified in a clean Ubuntu 24.04 container by replaying the CI lint step
(
make proto,make mocks,git diff --exit-code): no diff. Also verified on RHEL 9.Related issues