JingsongLi commented on code in PR #79: URL: https://github.com/apache/paimon-mosaic/pull/79#discussion_r3828952307
########## tools/verify_release_versions.py: ########## @@ -0,0 +1,687 @@ +#!/usr/bin/env python3 + +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You 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. + +"""Verify every published component uses the intended release version.""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys +import tempfile +import tomllib +import xml.etree.ElementTree as ET +from dataclasses import dataclass +from pathlib import Path + + +ROOT = Path(__file__).resolve().parent.parent +SEMVER = re.compile(r"[0-9]+\.[0-9]+\.[0-9]+") +DEPENDENCY_GROUPS = ("dependencies", "dev-dependencies", "build-dependencies") +RUST_PACKAGES = { + "core/Cargo.toml": "paimon-mosaic-core", + "ffi/Cargo.toml": "paimon-mosaic-ffi", + "jni/Cargo.toml": "paimon-mosaic-jni", + "cli/Cargo.toml": "paimon-mosaic-cli", +} + + +@dataclass(frozen=True) +class WorkspacePackage: + name: str + version: str + manifest: Path + + +@dataclass(frozen=True) +class PathDependency: + manifest: Path + group: tuple[str, ...] + alias: str + requirement: str | None + target: WorkspacePackage + + +@dataclass(frozen=True) +class TomlAssignment: + table: tuple[str, ...] + key: tuple[str, ...] + start: int + end: int + equals: int + + +def load_toml(relative_path: str, root: Path = ROOT) -> dict: + with (root / relative_path).open("rb") as file: + return tomllib.load(file) + + +def java_version(root: Path = ROOT) -> str: + pom = ET.parse(root / "java/pom.xml").getroot() + namespace = {"m": "http://maven.apache.org/POM/4.0.0"} + version = pom.find("m:version", namespace) + if version is None or not version.text: + raise ValueError("java/pom.xml has no direct project version") + return version.text.strip() + + +def cargo_metadata(root: Path) -> dict: + result = subprocess.run( + [ + "cargo", + "metadata", + "--format-version", + "1", + "--no-deps", + "--offline", + "--manifest-path", + str(root / "Cargo.toml"), + ], + cwd=root, + text=True, + capture_output=True, + check=False, + ) + if result.returncode != 0: + detail = result.stderr.strip() or result.stdout.strip() + raise ValueError(f"cargo metadata failed: {detail}") + try: + return json.loads(result.stdout) + except json.JSONDecodeError as error: + raise ValueError(f"cargo metadata returned invalid JSON: {error}") from error + + +def workspace_packages(root: Path) -> dict[Path, WorkspacePackage]: + metadata = cargo_metadata(root) + member_ids = set(metadata["workspace_members"]) + packages = {} + for package in metadata["packages"]: + if package["id"] not in member_ids: + continue + manifest = Path(package["manifest_path"]).resolve() + packages[manifest] = WorkspacePackage( + name=package["name"], + version=package["version"], + manifest=manifest, + ) + if not packages: + raise ValueError("Cargo workspace has no member packages") + return packages + + +def dependency_groups(data: dict) -> list[tuple[tuple[str, ...], dict]]: + groups = [] + for group_name in DEPENDENCY_GROUPS: + group = data.get(group_name) + if isinstance(group, dict): + groups.append(((group_name,), group)) + + workspace = data.get("workspace") + if isinstance(workspace, dict): + group = workspace.get("dependencies") + if isinstance(group, dict): + groups.append((("workspace", "dependencies"), group)) + + targets = data.get("target") + if isinstance(targets, dict): + for target_name, target in targets.items(): + if not isinstance(target, dict): + continue + for group_name in DEPENDENCY_GROUPS: + group = target.get(group_name) + if isinstance(group, dict): + groups.append((("target", target_name, group_name), group)) + return groups + + +def path_dependencies( + root: Path, packages: dict[Path, WorkspacePackage] +) -> list[PathDependency]: + manifests = {root.resolve() / "Cargo.toml", *packages.keys()} + dependencies = [] + for manifest in sorted(manifests): + with manifest.open("rb") as file: + data = tomllib.load(file) + for group_path, group in dependency_groups(data): + for alias, specification in group.items(): + if not isinstance(specification, dict) or "path" not in specification: + continue + dependency_path = specification["path"] + if not isinstance(dependency_path, str): + raise ValueError( + f"{manifest}: path dependency {alias} has a non-string path" + ) + target_manifest = ( + manifest.parent / dependency_path / "Cargo.toml" + ).resolve() + target = packages.get(target_manifest) + if target is None: + continue + requirement = specification.get("version") + if requirement is not None and not isinstance(requirement, str): + raise ValueError( + f"{manifest}: path dependency {alias} has a non-string version" + ) + dependencies.append( + PathDependency( + manifest=manifest, + group=group_path, + alias=alias, + requirement=requirement, + target=target, + ) + ) + return dependencies + + +def cargo_requirement_accepts(requirement: str, version: str) -> bool: + """Ask Cargo itself whether a version satisfies one of its requirements.""" + with tempfile.TemporaryDirectory(prefix="paimon-cargo-version-check-") as directory: + root = Path(directory) + target = root / "target-package" + consumer = root / "consumer" + (target / "src").mkdir(parents=True) + (consumer / "src").mkdir(parents=True) + (target / "src/lib.rs").write_text("", encoding="utf-8") + (consumer / "src/lib.rs").write_text("", encoding="utf-8") + (target / "Cargo.toml").write_text( + "[package]\n" + 'name = "path-version-target"\n' + f"version = {json.dumps(version)}\n" + 'edition = "2021"\n', + encoding="utf-8", + ) + (consumer / "Cargo.toml").write_text( + "[package]\n" + 'name = "path-version-consumer"\n' + 'version = "0.0.0"\n' + 'edition = "2021"\n' + "\n" + "[workspace]\n" + "\n" + "[dependencies]\n" + "target = { " + 'package = "path-version-target", ' + 'path = "../target-package", ' + f"version = {json.dumps(requirement)}" + " }\n", + encoding="utf-8", + ) + result = subprocess.run( + [ + "cargo", + "metadata", + "--format-version", + "1", + "--offline", + "--manifest-path", + str(consumer / "Cargo.toml"), + ], + text=True, + capture_output=True, + check=False, + ) + return result.returncode == 0 + + +def path_dependency_failures(root: Path) -> list[str]: + packages = workspace_packages(root) + failures = [] + compatibility = {} + for dependency in path_dependencies(root, packages): + if dependency.requirement is None: + continue + key = (dependency.requirement, dependency.target.version) + if key not in compatibility: + compatibility[key] = cargo_requirement_accepts(*key) + if compatibility[key]: + continue + manifest = dependency.manifest.relative_to(root) Review Comment: [P2] Canonicalize root before relative path comparisons Cargo returns canonical manifest paths, while root is used verbatim here. On macOS a temporary root commonly appears as /var/..., but Cargo reports /private/var/..., so relative_to(root) raises ValueError instead of returning version diagnostics. The same issue exists in update_cargo_versions. Please resolve root once at each public entry point before comparing Cargo paths. ########## tools/tests/test_validate_release_tag.py: ########## @@ -0,0 +1,276 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. + +from __future__ import annotations + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + + +TOOLS = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(TOOLS)) + +import validate_release_tag as validator + + +def run( + command: list[str], + *, + cwd: Path, + env: dict[str, str] | None = None, +) -> str: + result = subprocess.run( + command, + cwd=cwd, + env=env, + text=True, + capture_output=True, + check=False, + ) + assert result.returncode == 0, result.stderr or result.stdout + return result.stdout.strip() + + +def generate_key(tmp_path: Path, identity: str) -> tuple[Path, str, Path]: + home = tmp_path / identity.replace(" ", "-") + home.mkdir(mode=0o700) + env = os.environ.copy() + env["GNUPGHOME"] = str(home) + run( + [ + "gpg", + "--batch", + "--pinentry-mode", + "loopback", + "--passphrase", + "", + "--quick-generate-key", + f"{identity} <{identity.replace(' ', '.')}@example.test>", + "ed25519", + "sign", + "0", + ], + cwd=tmp_path, + env=env, + ) + listing = run( + ["gpg", "--batch", "--with-colons", "--list-secret-keys"], + cwd=tmp_path, + env=env, + ) + fingerprint = next( + line.split(":")[9] for line in listing.splitlines() if line.startswith("fpr:") + ) + keys = tmp_path / f"{identity.replace(' ', '-')}.keys" + exported = run( + ["gpg", "--batch", "--armor", "--export", fingerprint], + cwd=tmp_path, + env=env, + ) + keys.write_text(exported + "\n", encoding="utf-8") + return home, fingerprint, keys + + [email protected](scope="module") +def signing_keys(tmp_path_factory): + root = tmp_path_factory.mktemp("release-signing-keys") + trusted = generate_key(root, "Trusted Release") + untrusted = generate_key(root, "Untrusted Release") + return trusted, untrusted + + +def repository(tmp_path: Path) -> Path: + repo = tmp_path / "repo" + repo.mkdir() + run(["git", "init", "-q"], cwd=repo) + run(["git", "config", "user.name", "Release Test"], cwd=repo) + run(["git", "config", "user.email", "[email protected]"], cwd=repo) + commit(repo, "first") + return repo + + +def commit(repo: Path, contents: str) -> str: + (repo / "payload").write_text(contents, encoding="utf-8") + run(["git", "add", "payload"], cwd=repo) + run(["git", "commit", "-q", "-m", contents], cwd=repo) + return run(["git", "rev-parse", "HEAD"], cwd=repo) + + +def sign_tag(repo: Path, tag: str, home: Path, fingerprint: str) -> None: + env = os.environ.copy() + env["GNUPGHOME"] = str(home) + run( + [ + "git", + "-c", + "gpg.program=gpg", + "-c", + f"user.signingkey={fingerprint}", + "tag", + "-s", + "-m", + tag, + tag, + ], + cwd=repo, + env=env, + ) + + [email protected]( + "tag", + [ + "1.2.3", + "v01.2.3", + "v1.02.3", + "v1.2.03", + "v1.2.3-rc0", + "v1.2.3-rc01", + "v1.2.3-RC1", + "v1.2.3-extra", + ], +) +def test_parse_release_tag_rejects_noncanonical_names(tag): + with pytest.raises(validator.TagValidationError, match="not a canonical"): + validator.parse_release_tag(tag) + + +def test_signed_rc_and_final_on_same_commit_are_accepted(tmp_path, signing_keys): + (home, fingerprint, keys), _ = signing_keys + repo = repository(tmp_path) + sign_tag(repo, "v1.2.3-rc1", home, fingerprint) + sign_tag(repo, "v1.2.3", home, fingerprint) + + rc = validator.validate_release_tag(repo, "v1.2.3-rc1", keys) + final = validator.validate_release_tag(repo, "v1.2.3", keys) + + assert rc.matching_rc is None + assert final.commit == rc.commit + assert final.matching_rc == "v1.2.3-rc1" + + +def test_main_returns_success_and_failure_status( + tmp_path, signing_keys, monkeypatch, capsys +): + (home, fingerprint, keys), _ = signing_keys + repo = repository(tmp_path) + tag = "v1.2.4-rc1" + sign_tag(repo, tag, home, fingerprint) + arguments = [ + "validate_release_tag.py", + tag, + "--keys-file", + str(keys), + "--repository", + str(repo), + ] + + monkeypatch.setattr(sys, "argv", arguments) + assert validator.main() == 0 + capsys.readouterr() + + commit(repo, "after tag") + monkeypatch.setattr(sys, "argv", arguments) + assert validator.main() == 1 + captured = capsys.readouterr() + assert "release tag validation failed" in captured.err + + +def test_invalid_extra_rc_does_not_hide_a_valid_matching_rc(tmp_path, signing_keys): + (home, fingerprint, keys), _ = signing_keys + repo = repository(tmp_path) + sign_tag(repo, "v1.3.0-rc1", home, fingerprint) + run(["git", "tag", "v1.3.0-rc2"], cwd=repo) Review Comment: [P2] Make the lightweight-tag fixture ignore user signing config This bare git tag inherits global tag.gpgSign. For release managers with tag.gpgSign=true, Git attempts an annotated signed tag and the noninteractive test fails with "Terminal is dumb, but EDITOR unset"; the same applies to the fixture at line 245. Please explicitly disable signing for intentionally lightweight tags, for example with git -c tag.gpgSign=false tag ... ########## tools/tests/test_validate_release_tag.py: ########## @@ -0,0 +1,276 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. + +from __future__ import annotations + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + + +TOOLS = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(TOOLS)) + +import validate_release_tag as validator + + +def run( + command: list[str], + *, + cwd: Path, + env: dict[str, str] | None = None, +) -> str: + result = subprocess.run( + command, + cwd=cwd, + env=env, + text=True, + capture_output=True, + check=False, + ) + assert result.returncode == 0, result.stderr or result.stdout + return result.stdout.strip() + + +def generate_key(tmp_path: Path, identity: str) -> tuple[Path, str, Path]: + home = tmp_path / identity.replace(" ", "-") + home.mkdir(mode=0o700) + env = os.environ.copy() + env["GNUPGHOME"] = str(home) + run( + [ + "gpg", + "--batch", + "--pinentry-mode", + "loopback", + "--passphrase", + "", + "--quick-generate-key", + f"{identity} <{identity.replace(' ', '.')}@example.test>", + "ed25519", + "sign", + "0", + ], + cwd=tmp_path, + env=env, + ) + listing = run( + ["gpg", "--batch", "--with-colons", "--list-secret-keys"], + cwd=tmp_path, + env=env, + ) + fingerprint = next( + line.split(":")[9] for line in listing.splitlines() if line.startswith("fpr:") + ) + keys = tmp_path / f"{identity.replace(' ', '-')}.keys" + exported = run( + ["gpg", "--batch", "--armor", "--export", fingerprint], + cwd=tmp_path, + env=env, + ) + keys.write_text(exported + "\n", encoding="utf-8") + return home, fingerprint, keys + + [email protected](scope="module") +def signing_keys(tmp_path_factory): + root = tmp_path_factory.mktemp("release-signing-keys") Review Comment: [P2] Use a short temporary base for GNUPGHOME On macOS the default pytest temporary directory is already long. Nesting both the signing-key fixture and GNUPGHOME below it exceeds the gpg-agent Unix socket path limit, causing seven tests to fail during setup with "File name too long". Please allocate these GPG homes under a deliberately short canonical temporary directory and clean it afterward. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
