chore(linux): Enhance GHA API check

This change adds additional checks for the symbols file: - if a line with a method name gets changed, the package version number in that line also needs to be updated - for major API changes (methods renamed or removed) the API version numbers needs to be incremented - the API version numbers in the .symbols file and in `CORE_API_VERSION.md` need to match
keymanapp · Oct 26, 2023 · 7f166a6 · 7f166a6
1 parent 7665ab3
commit 7f166a6
Show file tree

Hide file tree

Showing 4 changed files with 174 additions and 28 deletions.
diff --git a/.github/workflows/deb-packaging.yml b/.github/workflows/deb-packaging.yml
@@ -1,14 +1,11 @@
 name: "Ubuntu packaging"
-run-name: "Ubuntu packaging - ${{ github.event.inputs.branch }} (branch ${{ github.head_ref }}), by @${{ github.actor }}"
+run-name: "Ubuntu packaging - ${{ github.event.inputs.branch }} (branch ${{ github.event.inputs.baseBranch }}), by @${{ github.event.inputs.user }}"
 on:
   workflow_dispatch:
     inputs:
       ref:
         description: 'For a PR `refs/pull/<PR-NUMBER>/merge`'
         required: true
-      headRef:
-        description: 'The ref of HEAD. For a PR `refs/pull/<PR-NUMBER>/head`.'
-        required: true
       branch:
         description: 'The branch. For a PR `PR-<PR-NUMBER>`.'
         required: true
@@ -248,6 +245,7 @@ jobs:
       uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c #v3.3.0
       with:
         ref: '${{ github.event.inputs.ref }}'
+        fetch-depth: 0
 
     - name: Download Artifacts
       uses: actions/download-artifact@9bc31d5ccc31df68ecc42ccf4149144866c47d8a # v3.0.2
@@ -263,10 +261,13 @@ jobs:
       run: |
         cd linux
         PKG_NAME=libkeymancore
-        SRC_PKG="${GITHUB_WORKSPACE}/artifacts/keyman-srcpkg/keyman_${{ needs.sourcepackage.outputs.VERSION }}-1.debian.tar.xz" \
-        BIN_PKG="${GITHUB_WORKSPACE}/artifacts/keyman-binarypkgs/${PKG_NAME}_${{ needs.sourcepackage.outputs.VERSION }}-1${{ needs.sourcepackage.outputs.PRERELEASE_TAG }}+jammy1_amd64.deb" \
-        PKG_VERSION="${{ needs.sourcepackage.outputs.VERSION }}" \
-        ./scripts/deb-packaging.sh --gha verify 2>> $GITHUB_STEP_SUMMARY
+        ./scripts/deb-packaging.sh \
+          --gha \
+          --bin-pkg "${GITHUB_WORKSPACE}/artifacts/keyman-binarypkgs/${PKG_NAME}_${{ needs.sourcepackage.outputs.VERSION }}-1${{ needs.sourcepackage.outputs.PRERELEASE_TAG }}+jammy1_amd64.deb" \
+          --pkg-version "${{ needs.sourcepackage.outputs.VERSION }}" \
+          --git-ref "${{ github.sha }}" \
+          --git-base "${{ github.event.inputs.baseRef }}" \
+          verify 2>> $GITHUB_STEP_SUMMARY
 
     - name: Archive .symbols file
       uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2

diff --git a/docs/linux/ibus-keyman.md b/docs/linux/ibus-keyman.md
@@ -6,7 +6,7 @@ Source code for Keyman engine for IBus is in [linux/ibus-keyman](../../linux/ibu
 
 ## Requirements
 
-meson (>= 0.53)
+meson (>= 0.57)
 
 ## Building
 

diff --git a/linux/scripts/deb-packaging.sh b/linux/scripts/deb-packaging.sh
@@ -1,7 +1,9 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2154 # (variables are set in build-utils.sh)
 # Actions for creating a Debian source package. Used by deb-packaging.yml GHA.
 
 set -eu
+shopt -s inherit_errexit
 
 ## START STANDARD BUILD SCRIPT INCLUDE
 # adjust relative paths as necessary
@@ -11,14 +13,18 @@ THIS_SCRIPT="$(readlink -f "${BASH_SOURCE[0]}")"
 
 builder_describe \
   "Helper for building Debian packages." \
-  "dependencies              Install dependencies as found in debian/control" \
-  "source+                   Build source package" \
-  "verify                    Verify API" \
-  "--gha                     Build from GitHub Action"
+  "dependencies               Install dependencies as found in debian/control" \
+  "source+                    Build source package" \
+  "verify                     Verify API" \
+  "--gha                      Build from GitHub Action" \
+  "--bin-pkg=BIN_PKG          Path and name of binary Debian package (for verify action)" \
+  "--pkg-version=PKG_VERSION  The version of the Debian package (for verify action)" \
+  "--git-ref=GIT_REF          The ref of the HEAD commit, e.g. HEAD of the PR branch (for verify action)" \
+  "--git-base=GIT_BASE        The ref of the base commit, e.g. HEAD of the master branch (for verify action)"
 
 builder_parse "$@"
 
-cd "$REPO_ROOT/linux"
+cd "${REPO_ROOT}/linux"
 
 if builder_has_option --gha; then
   START_STEP="::group::${COLOR_GREEN}"
@@ -50,19 +56,151 @@ source_action() {
   mv builddebs/* "${OUTPUT_PATH:-..}"
 }
 
+output_log() {
+  echo "$1" >&2
+  builder_echo "$1"
+}
+
+output_ok() {
+  echo ":heavy_check_mark: $1" >&2
+  builder_echo green "OK: $1"
+}
+
+output_warning() {
+  echo ":warning: $1" >&2
+  builder_echo warning "WARNING: $1"
+}
+
+output_error() {
+  echo ":x: $1" >&2
+  builder_echo error "ERROR: $1"
+}
+
+check_api_not_changed() {
+  # Checks that the API did not change compared to what's documented in the .symbols file
+  tmpDir=$(mktemp -d)
+  dpkg -x "${BIN_PKG}" "${tmpDir}"
+  cd debian
+  dpkg-gensymbols -v"${PKG_VERSION}" -p"${PKG_NAME}" -e"${tmpDir}"/usr/lib/x86_64-linux-gnu/"${LIB_NAME}".so* -O"${PKG_NAME}.symbols" -c4
+  output_ok "${LIB_NAME} API didn't change"
+  cd "${REPO_ROOT}/linux"
+}
+
+is_symbols_file_changed() {
+  local CHANGED_REF CHANGED_BASE
+  CHANGED_REF=$(git rev-parse "${GIT_REF}":"linux/debian/${PKG_NAME}.symbols")
+  CHANGED_BASE=$(git rev-parse "${GIT_BASE}":"linux/debian/${PKG_NAME}.symbols")
+  if [[ "${CHANGED_REF}" == "${CHANGED_BASE}" ]]; then
+    return 1
+  fi
+  return 0
+}
+
+check_updated_version_number() {
+  # Checks that the package version number got updated in the .symbols file if it got changed
+  # shellcheck disable=SC2310
+  if is_symbols_file_changed; then
+    # .symbols file changed, now check if the package version got updated as well
+    # Note: We don't check that ALL changes in that file have an updated package version -
+    # we hope this gets flagged in code review.
+    if ! git log -p -1 -- "debian/${PKG_NAME}.symbols" | grep -q "${PKG_VERSION}"; then
+      output_error "${PKG_NAME}.symbols file got changed without changing the package version number of the symbol"
+      EXIT_CODE=1
+    else
+      output_ok "${PKG_NAME}.symbols file got updated with package version number"
+    fi
+  else
+    output_ok "${PKG_NAME}.symbols file didn't change"
+  fi
+}
+
+get_api_version_in_symbols_file() {
+  # Extract 1 from "libkeymancore.so.1 libkeymancore #MINVER#"
+  local firstline
+  firstline=$(head -1 "debian/${PKG_NAME}.symbols")
+  firstline="${firstline#"${PKG_NAME}".so.}"
+  firstline="${firstline%% *}"
+  echo "${firstline}"
+}
+
+is_api_version_updated() {
+  local NEW_VERSION OLD_VERSION
+  git checkout "${GIT_REF}" -- "debian/${PKG_NAME}.symbols"
+  NEW_VERSION=$(get_api_version_in_symbols_file)
+  git checkout "${GIT_BASE}" -- "debian/${PKG_NAME}.symbols"
+  OLD_VERSION=$(get_api_version_in_symbols_file)
+  git checkout "${GIT_REF}" -- "debian/${PKG_NAME}.symbols"
+  if (( NEW_VERSION > OLD_VERSION )); then
+    return 0
+  fi
+  return 1
+}
+
+check_for_major_api_changes() {
+  # Checks that API version number gets updated if API changes
+  local WHAT_CHANGED CHANGES INSERTED DELETED MODIFIED
+
+  # shellcheck disable=2310
+  if ! is_symbols_file_changed; then
+    output_ok "No major API change"
+    return
+  fi
+
+  WHAT_CHANGED=$(git diff "${GIT_BASE}".."${GIT_REF}" -- "debian/${PKG_NAME}.symbols" | diffstat -m -t | tail -1)
+
+  IFS=',' read -r -a CHANGES <<< "${WHAT_CHANGED}"
+  INSERTED="${CHANGES[0]}"
+  DELETED="${CHANGES[1]}"
+  MODIFIED="${CHANGES[2]}"
+
+  if (( DELETED > 0 )) || (( MODIFIED > 0 )); then
+    builder_echo "Major API change: ${DELETED} lines deleted and ${MODIFIED} lines modified"
+    # shellcheck disable=2310
+    if ! is_api_version_updated; then
+      output_error "Major API change without updating API version number in ${PKG_NAME}.symbols file"
+      EXIT_CODE=2
+    else
+      output_ok "API version number got updated in ${PKG_NAME}.symbols file after major API change"
+    fi
+  elif (( INSERTED > 0 )); then
+    output_ok "Minor API change: ${INSERTED} lines added"
+    # We currently don't check version number for minor API changes
+  else
+    output_ok "No major API change"
+  fi
+}
+
+check_for_api_version_consistency() {
+  # Checks that the (major) API version number in the .symbols file and
+  # in CORE_API_VERSION.md are the same
+  local symbols_version api_version
+  symbols_version=$(get_api_version_in_symbols_file)
+  api_version=$(cat ../core/CORE_API_VERSION.md)
+  # Extract major version number from "1.0.0"
+  api_version=${api_version%%.*}
+
+  if (( symbols_version == api_version )); then
+    output_ok "API version in .symbols file and in CORE_API_VERSION.md is the same"
+  else
+    output_error "API version in .symbols file and in CORE_API_VERSION.md is different"
+    EXIT_CODE=3
+  fi
+}
+
 verify_action() {
-  tar xf "${SRC_PKG}"
   PKG_NAME=libkeymancore
   LIB_NAME=libkeymancore
-  if [ ! -f debian/${PKG_NAME}.symbols ]; then
-    echo ":warning: Missing ${PKG_NAME}.symbols file" >&2
-  else
-    tmpDir=$(mktemp -d)
-    dpkg -x "${BIN_PKG}" "$tmpDir"
-    cd debian
-    dpkg-gensymbols -v"${PKG_VERSION}" -p${PKG_NAME} -e"${tmpDir}"/usr/lib/x86_64-linux-gnu/${LIB_NAME}.so* -O${PKG_NAME}.symbols -c4
-    echo ":heavy_check_mark: ${LIB_NAME} API didn't change" >&2
+  if [[ ! -f debian/${PKG_NAME}.symbols ]]; then
+    output_warning "Missing ${PKG_NAME}.symbols file"
+    exit 0
   fi
+
+  EXIT_CODE=0
+  check_api_not_changed
+  check_updated_version_number
+  check_for_major_api_changes
+  check_for_api_version_consistency
+  exit "${EXIT_CODE}"
 }
 
 builder_run_action dependencies  dependencies_action

diff --git a/resources/build/trigger-builds.inc.sh b/resources/build/trigger-builds.inc.sh
@@ -69,32 +69,39 @@ function triggerGitHubActionsBuild() {
   local IS_TEST_BUILD="$1"
   local GITHUB_ACTION="$2"
   local GIT_BRANCH="${3:-master}"
-  local GIT_REF GIT_HEAD_REF
+  local GIT_BASE_BRANCH="${GIT_BRANCH}"
+  local GIT_USER="keyman-server"
+  local GIT_REF GIT_BASE_REF JSON
 
   local GITHUB_SERVER=https://api.github.com/repos/keymanapp/keyman
 
   if [ "${action:-""}" == "commit" ]; then
     # This will only be true if we created and pushed a tag
     GIT_REF="refs/tags/release@$VERSION_WITH_TAG"
-    GIT_HEAD_REF="${GIT_REF}"
+    GIT_BASE_REF="$(git rev-parse "${GIT_REF}^")"
     GIT_EVENT_TYPE="${GITHUB_ACTION}: release@${VERSION_WITH_TAG}"
   elif [[ $GIT_BRANCH != stable-* ]] && [[ $GIT_BRANCH =~ [0-9]+ ]]; then
     GIT_REF="refs/pull/${GIT_BRANCH}/merge"
-    GIT_HEAD_REF="refs/pull/${GIT_BRANCH}/head"
     GIT_EVENT_TYPE="${GITHUB_ACTION}: PR #${GIT_BRANCH}"
+    JSON=$(curl -s "${GITHUB_SERVER}/pulls/${GIT_BRANCH}")
+    GIT_USER="$(echo "$JSON" | jq -r '.user.login')"
+    GIT_BASE_REF="$(echo "$JSON" | jq -r '.base.ref')"
+    GIT_BASE_BRANCH="${GIT_BASE_REF}"
     GIT_BRANCH="PR-${GIT_BRANCH}"
   else
     GIT_REF="refs/heads/${GIT_BRANCH}"
-    GIT_HEAD_REF="${GIT_REF}"
+    GIT_BASE_REF="$(git rev-parse "${GIT_REF}^")"
     GIT_EVENT_TYPE="${GITHUB_ACTION}: ${GIT_BRANCH}"
   fi
 
   local DATA="{
     \"ref\": \"$GIT_REF\", \
     \"inputs\": { \
         \"ref\": \"$GIT_REF\", \
-        \"headRef\": \"$GIT_HEAD_REF\", \
         \"branch\": \"$GIT_BRANCH\", \
+        \"baseBranch\": \"$GIT_BASE_BRANCH\", \
+        \"baseRef\": \"$GIT_BASE_REF\", \
+        \"user\": \"$GIT_USER\", \
         \"isTestBuild\": \"$IS_TEST_BUILD\" \
     }}"