diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index cd4906579e..e74973fe0d 100644 --- a/.github/workflows/build_and_test.yml +++ b/.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 \ diff --git a/Cargo.lock b/Cargo.lock index b163d4fe46..7b5539bdf5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4817,6 +4817,7 @@ dependencies = [ "byteorder", "bytes", "chrono", + "const_format", "criterion", "futures", "heapless", diff --git a/libs/utils/Cargo.toml b/libs/utils/Cargo.toml index 87b0082356..e7c8323c1d 100644 --- a/libs/utils/Cargo.toml +++ b/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 diff --git a/libs/utils/src/lib.rs b/libs/utils/src/lib.rs index 69d3a1b9f2..c3558443c3 100644 --- a/libs/utils/src/lib.rs +++ b/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 { diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index c3e9853978..d3d7d7f04e 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/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}'") diff --git a/test_runner/regress/test_compatibility.py b/test_runner/regress/test_compatibility.py index 51e7b01eba..00e916394b 100644 --- a/test_runner/regress/test_compatibility.py +++ b/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( diff --git a/test_runner/regress/test_neon_cli.py b/test_runner/regress/test_neon_cli.py index cd481e69eb..9d24594cb6 100644 --- a/test_runner/regress/test_neon_cli.py +++ b/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)