Merge pull request ClickHouse#55092 from ClickHouse/backport/23.3/55028

Backport ClickHouse#55028 to 23.3: Get rid of the most of `os.path` stuff

.github/workflows/docs_check.yml

@@ -19,6 +19,7 @@ on:  # yamllint disable-line rule:truthy
       - 'docker/docs/**'
       - 'docs/**'
       - 'utils/check-style/aspell-ignore/**'
+      - 'tests/ci/docs_check.py'
 jobs:
   CheckLabels:
     runs-on: [self-hosted, style-checker]

.github/workflows/pull_request.yml

@@ -22,6 +22,7 @@ on:  # yamllint disable-line rule:truthy
       - 'docker/docs/**'
       - 'docs/**'
       - 'utils/check-style/aspell-ignore/**'
+      - 'tests/ci/docs_check.py'
 ##########################################################################################
 ##################################### SMALL CHECKS #######################################
 ##########################################################################################

tests/ci/ast_fuzzer_check.py

@@ -2,7 +2,6 @@
 
 import logging
 import subprocess
-import os
 import sys
 from pathlib import Path
 
@@ -16,9 +15,8 @@
     get_commit,
     post_commit_status,
 )
-from docker_pull_helper import get_image_with_version
+from docker_pull_helper import DockerImage, get_image_with_version
 from env_helper import (
-    GITHUB_RUN_URL,
     REPORTS_PATH,
     TEMP_PATH,
 )
@@ -27,19 +25,34 @@
 from report import TestResult
 from s3_helper import S3Helper
 from stopwatch import Stopwatch
+from tee_popen import TeePopen
+from upload_result_helper import upload_results
 
 IMAGE_NAME = "clickhouse/fuzzer"
 
 
-def get_run_command(pr_number, sha, download_url, workspace_path, image):
+def get_run_command(
+    pr_info: PRInfo,
+    build_url: str,
+    workspace_path: Path,
+    image: DockerImage,
+) -> str:
+    envs = [
+        f"-e PR_TO_TEST={pr_info.number}",
+        f"-e SHA_TO_TEST={pr_info.sha}",
+        f"-e BINARY_URL_TO_DOWNLOAD='{build_url}'",
+    ]
+
+    env_str = " ".join(envs)
+
     return (
         f"docker run "
         # For sysctl
         "--privileged "
         "--network=host "
         f"--volume={workspace_path}:/workspace "
+        f"{env_str} "
         "--cap-add syslog --cap-add sys_admin --cap-add=SYS_PTRACE "
-        f'-e PR_TO_TEST={pr_number} -e SHA_TO_TEST={sha} -e BINARY_URL_TO_DOWNLOAD="{download_url}" '
         f"{image}"
     )
 
@@ -49,14 +62,12 @@ def main():
 
     stopwatch = Stopwatch()
 
-    temp_path = TEMP_PATH
-    reports_path = REPORTS_PATH
+    temp_path = Path(TEMP_PATH)
+    temp_path.mkdir(parents=True, exist_ok=True)
+    reports_path = Path(REPORTS_PATH)
 
     check_name = sys.argv[1]
 
-    if not os.path.exists(temp_path):
-        os.makedirs(temp_path)
-
     pr_info = PRInfo()
 
     gh = Github(get_best_robot_token(), per_page=100)
@@ -70,7 +81,6 @@ def main():
     docker_image = get_image_with_version(reports_path, IMAGE_NAME)
 
     build_name = get_build_name_for_check(check_name)
-    print(build_name)
     urls = read_build_urls(build_name, reports_path)
     if not urls:
         raise Exception("No build URLs found")
@@ -84,25 +94,26 @@ def main():
 
     logging.info("Got build url %s", build_url)
 
-    workspace_path = os.path.join(temp_path, "workspace")
-    if not os.path.exists(workspace_path):
-        os.makedirs(workspace_path)
+    workspace_path = temp_path / "workspace"
+    workspace_path.mkdir(parents=True, exist_ok=True)
 
     run_command = get_run_command(
-        pr_info.number, pr_info.sha, build_url, workspace_path, docker_image
+        pr_info,
+        build_url,
+        workspace_path,
+        docker_image,
     )
     logging.info("Going to run %s", run_command)
 
-    run_log_path = os.path.join(temp_path, "run.log")
-    with open(run_log_path, "w", encoding="utf-8") as log:
-        with subprocess.Popen(
-            run_command, shell=True, stderr=log, stdout=log
-        ) as process:
-            retcode = process.wait()
-            if retcode == 0:
-                logging.info("Run successfully")
-            else:
-                logging.info("Run failed")
+    run_log_path = temp_path / "run.log"
+    main_log_path = workspace_path / "main.log"
+
+    with TeePopen(run_command, run_log_path) as process:
+        retcode = process.wait()
+        if retcode == 0:
+            logging.info("Run successfully")
+        else:
+            logging.info("Run failed")
 
     subprocess.check_call(f"sudo chown -R ubuntu:ubuntu {temp_path}", shell=True)
 
@@ -112,36 +123,40 @@ def main():
     s3_prefix = f"{pr_info.number}/{pr_info.sha}/fuzzer_{check_name_lower}/"
     paths = {
         "run.log": run_log_path,
-        "main.log": os.path.join(workspace_path, "main.log"),
-        "server.log.zst": os.path.join(workspace_path, "server.log.zst"),
-        "fuzzer.log": os.path.join(workspace_path, "fuzzer.log"),
-        "report.html": os.path.join(workspace_path, "report.html"),
-        "core.zst": os.path.join(workspace_path, "core.zst"),
-        "dmesg.log": os.path.join(workspace_path, "dmesg.log"),
+        "main.log": main_log_path,
+        "fuzzer.log": workspace_path / "fuzzer.log",
+        "report.html": workspace_path / "report.html",
+        "core.zst": workspace_path / "core.zst",
+        "dmesg.log": workspace_path / "dmesg.log",
     }
 
+    compressed_server_log_path = workspace_path / "server.log.zst"
+    if compressed_server_log_path.exists():
+        paths["server.log.zst"] = compressed_server_log_path
+
+    # The script can fail before the invocation of `zstd`, but we are still interested in its log:
+
+    not_compressed_server_log_path = workspace_path / "server.log"
+    if not_compressed_server_log_path.exists():
+        paths["server.log"] = not_compressed_server_log_path
+
     s3_helper = S3Helper()
-    for f in paths:
+    urls = []
+    report_url = ""
+    for file, path in paths.items():
         try:
-            paths[f] = s3_helper.upload_test_report_to_s3(Path(paths[f]), s3_prefix + f)
+            url = s3_helper.upload_test_report_to_s3(path, s3_prefix + file)
+            report_url = url if file == "report.html" else report_url
+            urls.append(url)
         except Exception as ex:
-            logging.info("Exception uploading file %s text %s", f, ex)
-            paths[f] = ""
-
-    report_url = GITHUB_RUN_URL
-    if paths["report.html"]:
-        report_url = paths["report.html"]
+            logging.info("Exception uploading file %s text %s", file, ex)
 
     # Try to get status message saved by the fuzzer
     try:
-        with open(
-            os.path.join(workspace_path, "status.txt"), "r", encoding="utf-8"
-        ) as status_f:
+        with open(workspace_path / "status.txt", "r", encoding="utf-8") as status_f:
             status = status_f.readline().rstrip("\n")
 
-        with open(
-            os.path.join(workspace_path, "description.txt"), "r", encoding="utf-8"
-        ) as desc_f:
+        with open(workspace_path / "description.txt", "r", encoding="utf-8") as desc_f:
             description = desc_f.readline().rstrip("\n")
     except:
         status = "failure"
@@ -153,6 +168,17 @@ def main():
     if "fail" in status:
         test_result.status = "FAIL"
 
+    if not report_url:
+        report_url = upload_results(
+            s3_helper,
+            pr_info.number,
+            pr_info.sha,
+            [test_result],
+            [],
+            check_name,
+            urls,
+        )
+
     ch_helper = ClickHouseHelper()
 
     prepared_events = prepare_tests_results_for_clickhouse(

tests/ci/bugfix_validate_check.py

@@ -1,10 +1,10 @@
 #!/usr/bin/env python3
 
+from pathlib import Path
 from typing import List, Tuple
 import argparse
 import csv
 import logging
-import os
 
 from github import Github
 
@@ -16,13 +16,13 @@
 from upload_result_helper import upload_results
 
 
-def parse_args():
+def parse_args() -> argparse.Namespace:
     parser = argparse.ArgumentParser()
-    parser.add_argument("status", nargs="+", help="Path to status file")
+    parser.add_argument("files", nargs="+", type=Path, help="Path to status files")
     return parser.parse_args()
 
 
-def post_commit_status_from_file(file_path: str) -> List[str]:
+def post_commit_status_from_file(file_path: Path) -> List[str]:
     with open(file_path, "r", encoding="utf-8") as f:
         res = list(csv.reader(f, delimiter="\t"))
     if len(res) < 1:
@@ -32,22 +32,24 @@ def post_commit_status_from_file(file_path: str) -> List[str]:
     return res[0]
 
 
-def process_result(file_path: str) -> Tuple[bool, TestResults]:
+def process_result(file_path: Path) -> Tuple[bool, TestResults]:
     test_results = []  # type: TestResults
     state, report_url, description = post_commit_status_from_file(file_path)
-    prefix = os.path.basename(os.path.dirname(file_path))
+    prefix = file_path.parent.name
     is_ok = state == "success"
     if is_ok and report_url == "null":
         return is_ok, test_results
 
-    status = f'OK: Bug reproduced (<a href="{report_url}">Report</a>)'
-    if not is_ok:
-        status = f'Bug is not reproduced (<a href="{report_url}">Report</a>)'
+    status = (
+        f'OK: Bug reproduced (<a href="{report_url}">Report</a>)'
+        if is_ok
+        else f'Bug is not reproduced (<a href="{report_url}">Report</a>)'
+    )
     test_results.append(TestResult(f"{prefix}: {description}", status))
     return is_ok, test_results
 
 
-def process_all_results(file_paths: str) -> Tuple[bool, TestResults]:
+def process_all_results(file_paths: List[Path]) -> Tuple[bool, TestResults]:
     any_ok = False
     all_results = []
     for status_path in file_paths:
@@ -59,12 +61,14 @@ def process_all_results(file_paths: str) -> Tuple[bool, TestResults]:
     return any_ok, all_results
 
 
-def main(args):
+def main():
     logging.basicConfig(level=logging.INFO)
+    args = parse_args()
+    status_files = args.files  # type: List[Path]
 
     check_name_with_group = "Bugfix validate check"
 
-    is_ok, test_results = process_all_results(args.status)
+    is_ok, test_results = process_all_results(status_files)
 
     if not test_results:
         logging.info("No results to upload")
@@ -76,7 +80,7 @@ def main(args):
         pr_info.number,
         pr_info.sha,
         test_results,
-        args.status,
+        status_files,
         check_name_with_group,
     )
 
@@ -93,4 +97,4 @@ def main(args):
 
 
 if __name__ == "__main__":
-    main(parse_args())
+    main()