Fix git-env version for PRs (#4641)

## Problem

Binaries created from PRs (both in docker images and for tests) have 
wrong git-env versions, they point to phantom merge commits.

## Summary of changes
- Prefer GIT_VERSION env variable even if git information was accessible
- Use `${{ github.event.pull_request.head.sha || github.sha }}` instead
of `${{ github.sha }}` for `GIT_VERSION` in workflows

So the builds will still happen from this phantom commit, but we will
report the PR commit.

---------

Co-authored-by: Joonas Koivunen <joonas@neon.tech>

.github/workflows/build_and_test.yml

@@ -155,7 +155,7 @@ jobs:
         build_type: [ debug, release ]
     env:
       BUILD_TYPE: ${{ matrix.build_type }}
-      GIT_VERSION: ${{ github.sha }}
+      GIT_VERSION: ${{ github.event.pull_request.head.sha || github.sha }}
 
     steps:
       - name: Fix git ownership
@@ -614,7 +614,7 @@ jobs:
           /kaniko/executor --reproducible --snapshot-mode=redo --skip-unused-stages --cache=true
                            --cache-repo 369495373322.dkr.ecr.eu-central-1.amazonaws.com/cache
                            --context .
-                           --build-arg GIT_VERSION=${{ github.sha }}
+                           --build-arg GIT_VERSION=${{ github.event.pull_request.head.sha || github.sha }}
                            --build-arg REPOSITORY=369495373322.dkr.ecr.eu-central-1.amazonaws.com
                            --destination 369495373322.dkr.ecr.eu-central-1.amazonaws.com/neon:${{needs.tag.outputs.build-tag}}
                            --destination neondatabase/neon:${{needs.tag.outputs.build-tag}}
@@ -658,7 +658,7 @@ jobs:
           /kaniko/executor --reproducible --snapshot-mode=redo --skip-unused-stages --cache=true
                            --cache-repo 369495373322.dkr.ecr.eu-central-1.amazonaws.com/cache
                            --context .
-                           --build-arg GIT_VERSION=${{ github.sha }}
+                           --build-arg GIT_VERSION=${{ github.event.pull_request.head.sha || github.sha }}
                            --build-arg BUILD_TAG=${{needs.tag.outputs.build-tag}}
                            --build-arg REPOSITORY=369495373322.dkr.ecr.eu-central-1.amazonaws.com
                            --dockerfile Dockerfile.compute-tools
@@ -715,7 +715,7 @@ jobs:
           /kaniko/executor --reproducible --snapshot-mode=redo --skip-unused-stages --cache=true
                            --cache-repo 369495373322.dkr.ecr.eu-central-1.amazonaws.com/cache
                            --context .
-                           --build-arg GIT_VERSION=${{ github.sha }}
+                           --build-arg GIT_VERSION=${{ github.event.pull_request.head.sha || github.sha }}
                            --build-arg PG_VERSION=${{ matrix.version }}
                            --build-arg BUILD_TAG=${{needs.tag.outputs.build-tag}}
                            --build-arg REPOSITORY=369495373322.dkr.ecr.eu-central-1.amazonaws.com
@@ -742,7 +742,7 @@ jobs:
           /kaniko/executor --reproducible --snapshot-mode=redo --skip-unused-stages --cache=true \
                            --cache-repo 369495373322.dkr.ecr.eu-central-1.amazonaws.com/cache \
                            --context . \
-                           --build-arg GIT_VERSION=${{ github.sha }} \
+                           --build-arg GIT_VERSION=${{ github.event.pull_request.head.sha || github.sha }} \
                            --build-arg PG_VERSION=${{ matrix.version }} \
                            --build-arg BUILD_TAG=${{needs.tag.outputs.build-tag}} \
                            --build-arg REPOSITORY=369495373322.dkr.ecr.eu-central-1.amazonaws.com \

Cargo.lock

@@ -4817,6 +4817,7 @@ dependencies = [
  "byteorder",
  "bytes",
  "chrono",
+ "const_format",
  "criterion",
  "futures",
  "heapless",

libs/utils/Cargo.toml

@@ -40,6 +40,8 @@ pq_proto.workspace = true
 metrics.workspace = true
 workspace_hack.workspace = true
 
+const_format.workspace = true
+
 [dev-dependencies]
 byteorder.workspace = true
 bytes.workspace = true

libs/utils/src/lib.rs

@@ -109,10 +109,16 @@ pub use failpoint_macro_helpers::failpoint_sleep_helper;
 /// * building in docker (either in CI or locally)
 ///
 /// One thing to note is that .git is not available in docker (and it is bad to include it there).
-/// So everything becides docker build is covered by git_version crate, and docker uses a `GIT_VERSION` argument to get the value required.
-/// It takes variable from build process env and puts it to the rustc env. And then we can retrieve it here by using env! macro.
-/// Git version received from environment variable used as a fallback in git_version invocation.
-/// And to avoid running buildscript every recompilation, we use rerun-if-env-changed option.
+/// When building locally, the `git_version` is used to query .git. When building on CI and docker,
+/// we don't build the actual PR branch commits, but always a "phantom" would be merge commit to
+/// the target branch -- the actual PR commit from which we build from is supplied as GIT_VERSION
+/// environment variable.
+///
+/// We ended up with this compromise between phantom would be merge commits vs. pull request branch
+/// heads due to old logs becoming more reliable (github could gc the phantom merge commit
+/// anytime) in #4641.
+///
+/// To avoid running buildscript every recompilation, we use rerun-if-env-changed option.
 /// So the build script will be run only when GIT_VERSION envvar has changed.
 ///
 /// Why not to use buildscript to get git commit sha directly without procmacro from different crate?
@@ -132,17 +138,28 @@ pub use failpoint_macro_helpers::failpoint_sleep_helper;
 #[macro_export]
 macro_rules! project_git_version {
     ($const_identifier:ident) => {
-        const $const_identifier: &str = git_version::git_version!(
-            prefix = "git:",
-            fallback = concat!(
-                "git-env:",
-                env!("GIT_VERSION", "Missing GIT_VERSION envvar")
-            ),
-            args = ["--abbrev=40", "--always", "--dirty=-modified"] // always use full sha
-        );
+        // this should try GIT_VERSION first only then git_version::git_version!
+        const $const_identifier: &::core::primitive::str = {
+            const __COMMIT_FROM_GIT: &::core::primitive::str = git_version::git_version! {
+                prefix = "",
+                fallback = "unknown",
+                args = ["--abbrev=40", "--always", "--dirty=-modified"] // always use full sha
+            };
+
+            const __ARG: &[&::core::primitive::str; 2] = &match ::core::option_env!("GIT_VERSION") {
+                ::core::option::Option::Some(x) => ["git-env:", x],
+                ::core::option::Option::None => ["git:", __COMMIT_FROM_GIT],
+            };
+
+            $crate::__const_format::concatcp!(__ARG[0], __ARG[1])
+        };
     };
 }
 
+/// Re-export for `project_git_version` macro
+#[doc(hidden)]
+pub use const_format as __const_format;
+
 /// Same as `assert!`, but evaluated during compilation and gets optimized out in runtime.
 #[macro_export]
 macro_rules! const_assert {

test_runner/fixtures/neon_fixtures.py

@@ -3109,3 +3109,18 @@ def last_flush_lsn_upload(
     ps_http.timeline_checkpoint(tenant_id, timeline_id)
     wait_for_upload(ps_http, tenant_id, timeline_id, last_flush_lsn)
     return last_flush_lsn
+
+
+def parse_project_git_version_output(s: str) -> str:
+    """
+    Parses the git commit hash out of the --version output supported at least by neon_local.
+
+    The information is generated by utils::project_git_version!
+    """
+    import re
+
+    res = re.search(r"git(-env)?:([0-9a-fA-F]{8,40})(-\S+)?", s)
+    if res and (commit := res.group(2)):
+        return commit
+
+    raise ValueError(f"unable to parse --version output: '{s}'")

test_runner/regress/test_compatibility.py

@@ -14,6 +14,7 @@ from fixtures.neon_fixtures import (
     NeonEnvBuilder,
     PgBin,
     PortDistributor,
+    parse_project_git_version_output,
 )
 from fixtures.pageserver.http import PageserverHttpClient
 from fixtures.pageserver.utils import (
@@ -352,7 +353,7 @@ def prepare_snapshot(
 # get git SHA of neon binary
 def get_neon_version(neon_binpath: Path):
     out = subprocess.check_output([neon_binpath / "neon_local", "--version"]).decode("utf-8")
-    return out.split("git:", 1)[1].rstrip()
+    return parse_project_git_version_output(out)
 
 
 def check_neon_works(

test_runner/regress/test_neon_cli.py

@@ -1,12 +1,18 @@
+import os
+import subprocess
+from pathlib import Path
 from typing import cast
 
+import pytest
 import requests
 from fixtures.neon_fixtures import (
     DEFAULT_BRANCH_NAME,
     NeonEnv,
     NeonEnvBuilder,
+    parse_project_git_version_output,
 )
 from fixtures.pageserver.http import PageserverHttpClient
+from fixtures.pg_version import PgVersion, skip_on_postgres
 from fixtures.types import TenantId, TimelineId
 
 
@@ -131,3 +137,66 @@ def test_cli_start_stop(neon_env_builder: NeonEnvBuilder):
     # Default stop
     res = env.neon_cli.raw_cli(["stop"])
     res.check_returncode()
+
+
+@skip_on_postgres(PgVersion.V14, reason="does not use postgres")
+@pytest.mark.skipif(
+    os.environ.get("BUILD_TYPE") == "debug", reason="unit test for test support, either build works"
+)
+def test_parse_project_git_version_output_positive():
+    commit = "b6f77b5816cf1dba12a3bc8747941182ce220846"
+
+    positive = [
+        # most likely when developing locally
+        f"Neon CLI git:{commit}-modified",
+        # when developing locally
+        f"Neon CLI git:{commit}",
+        # this is not produced in practice, but the impl supports it
+        f"Neon CLI git-env:{commit}-modified",
+        # most likely from CI or docker build
+        f"Neon CLI git-env:{commit}",
+    ]
+
+    for example in positive:
+        assert parse_project_git_version_output(example) == commit
+
+
+@skip_on_postgres(PgVersion.V14, reason="does not use postgres")
+@pytest.mark.skipif(
+    os.environ.get("BUILD_TYPE") == "debug", reason="unit test for test support, either build works"
+)
+def test_parse_project_git_version_output_local_docker():
+    """
+    Makes sure the tests don't accept the default version in Dockerfile one gets without providing
+    a commit lookalike in --build-arg GIT_VERSION=XXX
+    """
+    input = "Neon CLI git-env:local"
+
+    with pytest.raises(ValueError) as e:
+        parse_project_git_version_output(input)
+
+    assert input in str(e)
+
+
+@skip_on_postgres(PgVersion.V14, reason="does not use postgres")
+@pytest.mark.skipif(
+    os.environ.get("BUILD_TYPE") == "debug", reason="cli api sanity, either build works"
+)
+def test_binaries_version_parses(neon_binpath: Path):
+    """
+    Ensures that we can parse the actual outputs of --version from a set of binaries.
+
+    The list is not meant to be exhaustive, and compute_ctl has a different way for example.
+    """
+
+    binaries = [
+        "neon_local",
+        "pageserver",
+        "safekeeper",
+        "proxy",
+        "pg_sni_router",
+        "storage_broker",
+    ]
+    for bin in binaries:
+        out = subprocess.check_output([neon_binpath / bin, "--version"]).decode("utf-8")
+        parse_project_git_version_output(out)