Check protobuf compatibility of main and PR commits w.r.t. previous…

… stable release [KVL-1109] (digital-asset#10950) * Check protobuf compatibility of release commits w.r.t. previous stable release CHANGELOG_BEGIN CHANGELOG_END * Remove blank line * Don't persist credentials Co-authored-by: Gary Verhaegen <gary.verhaegen@digitalasset.com> * check-protobuf-against-stable.sh: SRC_DIR -> PROJECT ROOT + simplify * Don't set LATEST_STABLE global in a function * Simplify by using only the main work tree * Simplify further as the check will be only run from `main` * Move the check to `ci/build.yml` so that it is also run on PRs * Enter the development environment to use tools * Make variables read-only * Support release branches and PRs targeting them * Fix and document the reference tag finding logic * Fix SYSTEM_PULLREQUEST_TARGETBRANCH and print it * Don't log the source branch * Fix comment formatting Co-authored-by: Gary Verhaegen <gary.verhaegen@digitalasset.com> * Enable Slack integration Co-authored-by: Gary Verhaegen <gary.verhaegen@digitalasset.com> * Don't check if the branch is a release one ...as the check won't be run on release branches. * Add compatibility_stable_protobuf to collect_build_data * Do not activate dev-env globally but only in sub-shells * Add an explanation about why the check is not run on release branch commits * Simplify further by leveraging `buf`'s ability to compare against branches * Use `buf`'s `tag` locator instead of `branch` * Split buf checks by module and remove previous manual check * Explain how to run locally * Use more future-proof WIRE_JSON for participant-integration-api Co-authored-by: Simon Meier <meiersi-da@users.noreply.github.com> * Use stricter FILE for the ledger gRPC API * Propose an explanation for WIRE in kvutils * Fix comment typo * Re-introduce linting configuration for kvutils * Simplify explanation for KVUtils' breaking check rule * Remove extra (C) header from 3rd-party proto * Don't touch the copyright of google/rpc/status.proto Co-authored-by: Gary Verhaegen <gary.verhaegen@digitalasset.com> Co-authored-by: Simon Meier <meiersi-da@users.noreply.github.com>
kgeonw · Sep 23, 2021 · f5d2135 · f5d2135
1 parent bf8b75d
commit f5d2135
Show file tree

Hide file tree

Showing 9 changed files with 150 additions and 42 deletions.
diff --git a/3rdparty/protobuf/google/rpc/status.proto b/3rdparty/protobuf/google/rpc/status.proto
@@ -0,0 +1,47 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+syntax = "proto3";
+
+package google.rpc;
+
+import "google/protobuf/any.proto";
+
+option cc_enable_arenas = true;
+option go_package = "google.golang.org/genproto/googleapis/rpc/status;status";
+option java_multiple_files = true;
+option java_outer_classname = "StatusProto";
+option java_package = "com.google.rpc";
+option objc_class_prefix = "RPC";
+
+// The `Status` type defines a logical error model that is suitable for
+// different programming environments, including REST APIs and RPC APIs. It is
+// used by [gRPC](https://github.com/grpc). Each `Status` message contains
+// three pieces of data: error code, error message, and error details.
+//
+// You can find out more about this error model and how to work with it in the
+// [API Design Guide](https://cloud.google.com/apis/design/errors).
+message Status {
+  // The status code, which should be an enum value of [google.rpc.Code][google.rpc.Code].
+  int32 code = 1;
+
+  // A developer-facing error message, which should be in English. Any
+  // user-facing error message should be localized and sent in the
+  // [google.rpc.Status.details][google.rpc.Status.details] field, or localized by the client.
+  string message = 2;
+
+  // A list of messages that carry the error details.  There is a common set of
+  // message types for APIs to use.
+  repeated google.protobuf.Any details = 3;
+}
diff --git a/buf.yaml → buf-kvutils.yaml b/buf.yaml → buf-kvutils.yaml
@@ -3,18 +3,15 @@
 
 version: v1beta1
 
-deps:
-  - buf.build/googleapis/googleapis
-
 build:
   roots:
-    - daml-lf/archive/src/main/protobuf/
-    - daml-lf/transaction/src/main/protobuf/
-    - ledger/ledger-configuration/protobuf/
-    - ledger/participant-state/kvutils/src/main/protobuf/
-    - ledger-api/grpc-definitions
+    - daml-lf/archive/src/main/protobuf
+    - daml-lf/transaction/src/main/protobuf
+    - ledger/ledger-configuration/protobuf
+    - ledger/participant-state/kvutils/src/main/protobuf
   excludes:
-    # We have to exclude these to avoid duplicate definitions of e.g. Unit within the same package.
+    # We have to exclude these to avoid duplicate definitions of e.g. Unit
+    # within the same package.
     - daml-lf/archive/src/main/protobuf/com/daml/daml_lf_1_11/
     - daml-lf/archive/src/main/protobuf/com/daml/daml_lf_1_12/
     - daml-lf/archive/src/main/protobuf/com/daml/daml_lf_1_13/
@@ -29,4 +26,10 @@ lint:
 
 breaking:
   use:
-    - WIRE_JSON
+    # WIRE is enough here as KVUtils only needs to be able to read data persisted
+    # by previous versions of the code.
+    - WIRE
+  except:
+    # Moving an individual field into a new `one_of` retains binary backwards
+    # compatibility.
+    - FIELD_SAME_ONEOF
diff --git a/buf-ledger-api.yaml b/buf-ledger-api.yaml
@@ -0,0 +1,17 @@
+# Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+
+version: v1beta1
+
+build:
+  roots:
+    - ledger-api/grpc-definitions
+    - 3rdparty/protobuf
+
+breaking:
+  use:
+    # Set to FILE to ensure that all breakages of the gRPC Ledger API are explicitly
+    # discussed with Engineering and Product Leadership.
+    - FILE
+  except:
+    - FILE_NO_DELETE # Avoids errors due to refactored `buf` modules.
diff --git a/buf-participant-integration-api.yaml b/buf-participant-integration-api.yaml
@@ -0,0 +1,14 @@
+# Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+
+version: v1beta1
+
+build:
+  roots:
+    - ledger/participant-integration-api/src/main/protobuf
+
+breaking:
+  use:
+    # Using WIRE_JSON here to ensure we have the option of also using JSON encodings of the
+    # .proto values stored in the IndexDB.
+    - WIRE_JSON
diff --git a/buf-stable-protos-image.bin b/buf-stable-protos-image.bin
diff --git a/buf.lock b/buf.lock
diff --git a/ci/build.yml b/ci/build.yml
@@ -413,6 +413,15 @@ jobs:
     - template: tell-slack-failed.yml
     - template: report-end.yml
 
+- job: compatibility_stable_protobuf
+  pool:
+    name: ubuntu_20_04
+    demands: assignment -equals default
+  steps:
+    - checkout: self
+    - bash: ci/check-protobuf-against-stable.sh
+    - template: tell-slack-failed.yml
+
 - job: check_for_release
   dependsOn:
     - git_sha
@@ -486,6 +495,7 @@ jobs:
     - release
     - git_sha
     - compatibility_linux
+    - compatibility_stable_protobuf
     - check_for_release
   pool:
     name: "ubuntu_20_04"

diff --git a/ci/check-protobuf-against-stable.sh b/ci/check-protobuf-against-stable.sh
@@ -0,0 +1,49 @@
+#!/usr/bin/env bash
+# Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+set -euo pipefail
+
+readonly RELEASE_BRANCH_REGEX="^release/.*"
+
+declare -a BUF_MODULES=(
+  "buf-kvutils.yaml"
+  "buf-ledger-api.yaml"
+  "buf-participant-integration-api.yaml"
+)
+
+readonly BUF_IMAGE_TMPDIR="$(mktemp -d)"
+trap 'rm -rf ${BUF_IMAGE_TMPDIR}' EXIT
+
+# The `SYSTEM_PULLREQUEST_TARGETBRANCH` environment variable is defined by
+# Azure Pipelines; in order to run this script locally, define it beforehand
+# as the branch being targeted. For example:
+#
+#   SYSTEM_PULLREQUEST_TARGETBRANCH=main bash -x ci/check-protobuf-against-stable.sh
+#
+echo "The target branch is '${SYSTEM_PULLREQUEST_TARGETBRANCH}'."
+
+# For `main` and PRs targeting `main`, we simply check against the most recent
+# stable tag.
+#
+# For PRs targeting release branches, we should really check against
+# all the most recent stable tags reachable from either the current branch or
+# from previous release branches (say, against both `1.17.1` and `1.16.2`
+# created after the `release/1.17.x` branch).
+# Instead, we approximate by checking only against the most recent stable tag
+# reachable from the current branch, under the assumption that if a lesser
+# release branch contains a protobuf change, then it will also be present in
+# higher ones either through a shared commit or a back-port from `main`.
+#
+# Finally, this check does not need to run on release branch commits because
+# they are built sequentially, so no conflicts are possible and the per-PR
+# check is enough.
+GIT_TAG_SCOPE=""
+if [[ "${SYSTEM_PULLREQUEST_TARGETBRANCH}" =~ ${RELEASE_BRANCH_REGEX} ]]; then
+  GIT_TAG_SCOPE="--merged"
+fi
+
+readonly LATEST_STABLE_TAG="$(git tag ${GIT_TAG_SCOPE} | grep -v "snapshot" | sort -V | tail -1)"
+echo "Checking protobufs against tag '${LATEST_STABLE_TAG}'"
+for buf_module in "${BUF_MODULES[@]}"; do
+  (eval "$(dev-env/bin/dade assist)" ; buf breaking --config "${buf_module}" --against ".git#tag=${LATEST_STABLE_TAG}")
+done
diff --git a/fmt.sh b/fmt.sh
@@ -11,19 +11,6 @@ cd "$(dirname "$0")"
 # load the dev-env
 eval "$(dev-env/bin/dade-assist)"
 
-# Location of reference image used for buf breaking check.
-#
-# It should be re-built by running './fmt.sh --rebuild-buf-image' and then committed:
-#
-# 1. as part of a commit also containing breaking proto changes (that have been agreed to), and/or
-# 2. as part of a subsequent commit (e.g. in the same PR, in a later PR or as part of the SDK release process),
-#    when already committed proto changes become stable.
-#
-# Proto changes that are part of an SDK release (especially RC and stable ones) should also be included in
-# the buf image.
-
-buf_image="buf-stable-protos-image.bin"
-
 ## Config ##
 is_test=
 scalafmt_args=()
@@ -75,7 +62,6 @@ Usage: ./fmt.sh [options]
 Options:
   -h, --help: shows this help
   --test:     only test for formatting changes, used by CI
-  --rebuild-buf-image:  rebuilds reference image used for buf breaking checks
 USAGE
       exit
       ;;
@@ -93,11 +79,6 @@ USAGE
       scalafmt_args+=('--mode=diff' "--diff-branch=${merge_base}")
       diff_mode=true
       ;;
-    --rebuild-buf-image)
-      shift
-      run buf build -o "${buf_image}"
-      exit
-      ;;
     *)
       echo "fmt.sh: unknown argument $1" >&2
       exit 1
@@ -163,9 +144,6 @@ run scalafmt "${scalafmt_args[@]:-}"
 # check for Bazel build files code formatting
 run bazel run "$buildifier_target"
 
-# Check for breaking proto changes.
-run buf breaking --against "${buf_image}"
-
 # Note that we cannot use a symlink here because Windows.
 if ! diff .bazelrc compatibility/.bazelrc >/dev/null; then
     echo ".bazelrc and  compatibility/.bazelrc are out of sync:"