diff --git a/.github/workflows/javaCodestyle.yml b/.github/workflows/javaCodestyle.yml index 7dedc5f4865..a63e3298760 100644 --- a/.github/workflows/javaCodestyle.yml +++ b/.github/workflows/javaCodestyle.yml @@ -21,6 +21,16 @@ name: Java Codestyle +# Two Java style gates share this workflow: +# * Checkstyle -- whole-tree rule check (dev/checkstyle), on push and PR. +# * Java Format -- Eclipse formatter (dev/CodeStyle_eclipse.xml) applied to +# ONLY the lines a PR edits; fails if any edited line would +# change. The tree is not yet fully formatter-clean, so +# scoping to edited lines keeps it actionable and lets the +# codebase converge line-by-line. This gate needs the PR +# base commit, so it runs on pull_request only. +# They are separate jobs so each reports its own pass/fail status. + on: push: paths-ignore: @@ -28,7 +38,6 @@ on: - '*.md' - '*.html' - 'src/main/python/**' - - 'dev/**' branches: - main pull_request: @@ -37,16 +46,18 @@ on: - '*.md' - '*.html' - 'src/main/python/**' - - 'dev/**' branches: - main +permissions: + contents: read + concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} jobs: - java_codestyle: + java_checkstyle: name: Java Checkstyle runs-on: ubuntu-latest steps: @@ -62,3 +73,47 @@ jobs: - name: Run Checkstyle run: mvn -ntp -B -Dcheckstyle.skip=false checkstyle:check + + java_format: + name: Java Format Check + # line-scoped to the PR diff -> needs the pull_request base commit + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - name: Checkout Repository + uses: actions/checkout@v7 + with: + fetch-depth: 0 + + - name: Setup Java 17 adopt + uses: actions/setup-java@v5 + with: + distribution: adopt + java-version: '17' + cache: 'maven' + + - name: Setup Python + uses: actions/setup-python@v5 + with: + python-version: '3.11' + + - name: Unit-test the format checker + run: | + python -m pip install --quiet pytest + python -m pytest dev/tests -q + + - name: Check formatting of PR-edited lines + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + # Fails only if the Eclipse formatter would change a line this PR edited. + # See dev/format_changed.py for the line-scoping logic. + if ! python3 dev/format_changed.py --check "$BASE_SHA"; then + echo "::error::Some lines edited by this PR are not formatted per dev/CodeStyle_eclipse.xml." + echo "Fix only your edited lines locally and commit the result:" + echo "" + echo " dev/format-changed.sh" + echo "" + echo "(Do NOT run a bare 'mvn formatter:format' -- it reformats the whole tree.)" + exit 1 + fi diff --git a/dev/format-changed.sh b/dev/format-changed.sh new file mode 100755 index 00000000000..a49192e3d29 --- /dev/null +++ b/dev/format-changed.sh @@ -0,0 +1,48 @@ +#!/usr/bin/env bash +#------------------------------------------------------------- +# +# 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. +# +#------------------------------------------------------------- +# +# Apply the Eclipse formatter (dev/CodeStyle_eclipse.xml) to ONLY the lines you +# edited -- the exact scope the "Java Format" CI check enforces. +# +# A bare `mvn formatter:format` would reformat EVERY .java file under the source +# roots, and even scoping to changed *files* would reformat their pre-existing +# (not-yet-clean) lines. The existing tree is not fully formatter-clean, so both +# would produce a large unrelated diff. This delegates to dev/format_changed.py, +# which formats each changed file but keeps only the changes that land on the +# lines you actually edited. +# +# "Changed" = lines that differ from the base branch (the merge target), +# including your committed-on-branch, staged, unstaged and untracked edits. Run +# `git fetch upstream main` first so the diff is accurate, or pass an explicit, +# current base ref if the default is stale/behind your branch point. The base-ref +# fallback order lives in one place: see resolve_base() in dev/format_changed.py. +# +# Usage: +# dev/format-changed.sh [base-ref] +# base-ref branch/commit to diff against (default: resolved by +# dev/format_changed.py) +# +set -euo pipefail + +cd "$(git rev-parse --show-toplevel)" + +exec python3 dev/format_changed.py --fix "$@" diff --git a/dev/format-exclude.txt b/dev/format-exclude.txt new file mode 100644 index 00000000000..dd3d9749eed --- /dev/null +++ b/dev/format-exclude.txt @@ -0,0 +1,35 @@ +#------------------------------------------------------------- +# +# 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. +# +#------------------------------------------------------------- + +# Files exempt from the Java Format check (dev/format_changed.py). +# +# One pattern per line; blank lines and lines starting with '#' are ignored. +# Each pattern is matched (glob / fnmatch) against the repo-relative path AND +# against the bare file name, so both of these work: +# +# src/main/java/org/apache/sysds/conf/DMLConfig.java +# *DMLConfig.java +# +# Use this only for files whose non-standard layout is intentional and must not +# be reformatted (e.g. hand-aligned tables of constants). + +# Hand-aligned configuration constants; intentional non-standard formatting. +src/main/java/org/apache/sysds/conf/DMLConfig.java diff --git a/dev/format_changed.py b/dev/format_changed.py new file mode 100755 index 00000000000..ce8b7acccff --- /dev/null +++ b/dev/format_changed.py @@ -0,0 +1,324 @@ +#!/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. +# +# ------------------------------------------------------------- +# +# Line-scoped Java formatting against dev/CodeStyle_eclipse.xml. +# +# The Eclipse formatter only works on whole files, and the existing tree is not +# fully formatter-clean, so formatting a whole edited file would flag lines the +# PR never touched. This script therefore formats each changed file but keeps +# only the formatting changes that fall on the lines the PR edited (like +# clang-format-diff): it diffs the original against the fully-formatted version +# and restricts the result to the changed line ranges. +# +# --check (default): print the changed-line formatting fixes and exit 1 if any. +# --fix : apply only the changed-line formatting fixes in place. +# +# Usage: dev/format_changed.py [--check|--fix] [base-ref] +# +import difflib +import fnmatch +import os +import re +import subprocess +import sys + +FMT_VERSION = "2.24.1" +CONFIG = "dev/CodeStyle_eclipse.xml" +EXCLUDE_FILE = "dev/format-exclude.txt" +SRC_PREFIX = r"src/(main|test)/java/" +SRC_RE = re.compile(r"^" + SRC_PREFIX + r".+\.java$") + + +# --- small process / IO helpers ------------------------------------------------ + +def git(*args, check=True): + # core.quotePath=false so non-ASCII paths are emitted verbatim (not \NNN + # escaped), otherwise SRC_RE would silently skip them and bypass the check. + proc = subprocess.run(["git", "-c", "core.quotePath=false", *args], + capture_output=True, text=True) + if check and proc.returncode != 0: + sys.exit(f"ERROR: `git {' '.join(args)}` failed:\n{proc.stderr.strip()}") + return proc.stdout + + +def read_text(path): + # newline="" keeps line endings byte-exact so a check-mode restore (or a + # fix-mode non-flagged file) round-trips without CRLF->LF rewrites. + with open(path, encoding="utf-8", newline="") as fh: + return fh.read() + + +def write_text(path, text): + with open(path, "w", encoding="utf-8", newline="") as fh: + fh.write(text) + + +def strip_src_prefix(path): + return re.sub(r"^" + SRC_PREFIX, "", path) + + +# --- exemption list ------------------------------------------------------------ + +def load_excludes(): + # glob patterns of files exempt from the style check, one per line + patterns = [] + if os.path.exists(EXCLUDE_FILE): + with open(EXCLUDE_FILE, encoding="utf-8") as fh: + for line in fh: + s = line.strip() + if s and not s.startswith("#"): + patterns.append(s) + return patterns + + +def is_excluded(path, patterns): + base = os.path.basename(path) + return any(fnmatch.fnmatch(path, p) or fnmatch.fnmatch(base, p) for p in patterns) + + +# --- git ref / file discovery -------------------------------------------------- + +def ref_exists(ref): + return subprocess.run(["git", "rev-parse", "--verify", "--quiet", ref + "^{commit}"], + capture_output=True).returncode == 0 + + +def resolve_base(explicit): + if explicit: + if not ref_exists(explicit): + sys.exit(f"Base ref not found: {explicit}") + return explicit + for ref in ("upstream/main", "origin/main", "main"): + if ref_exists(ref): + return ref + sys.exit("Could not determine a base ref; pass one explicitly.") + + +def merge_base(base): + mb = git("merge-base", base, "HEAD", check=False).strip() + return mb or base + + +def base_has(mergebase, path): + return subprocess.run(["git", "cat-file", "-e", f"{mergebase}:{path}"], + capture_output=True).returncode == 0 + + +def discover_files(mergebase): + tracked = [f for f in git("diff", "--name-only", "--diff-filter=ACMR", + mergebase, "--").splitlines() if SRC_RE.match(f)] + untracked = [f for f in git("ls-files", "--others", "--exclude-standard").splitlines() + if SRC_RE.match(f)] + seen = set(tracked) + files = tracked + [f for f in untracked if f not in seen] + + patterns = load_excludes() + skipped = [f for f in files if is_excluded(f, patterns)] + files = [f for f in files if not is_excluded(f, patterns)] + return files, skipped, set(untracked) + + +# --- changed-line ranges (pure parsing, unit-tested) --------------------------- + +def _hunk_range(header): + # parse the new-side (+) range of a `@@ -a,b +c,d @@` unified-diff header; + # returns a 1-based inclusive (start, end), or None for pure deletions / non-headers + m = re.match(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@", header) + if not m: + return None + start = int(m.group(1)) + count = int(m.group(2)) if m.group(2) is not None else 1 + return (start, start + count - 1) if count > 0 else None + + +def parse_hunks(diff_text): + # all new-side ranges in a single-file unified diff (used by tests and below) + ranges = [] + for line in diff_text.splitlines(): + r = _hunk_range(line) + if r is not None: + ranges.append(r) + return ranges + + +def changed_ranges(mergebase, files): + # one batched `git diff` for all files, split per file on the `+++ b/` header + ranges = {f: [] for f in files} + if not files: + return ranges + out = git("diff", "-U0", mergebase, "--", *files) + current = None + for line in out.splitlines(): + if line.startswith("+++ b/"): + current = line[len("+++ b/"):] + elif current is not None: + r = _hunk_range(line) + if r is not None and current in ranges: + ranges[current].append(r) + return ranges + + +def overlaps(i1, i2, ranges): + # original-side region [i1, i2) (0-based half-open) vs 1-based inclusive ranges + lo, hi = i1 + 1, i2 # convert to 1-based inclusive; insert (i1==i2) -> lo>hi + for (s, e) in ranges: + if i1 == i2: # pure insertion between original lines i1 and i1+1 + if s - 1 <= i1 <= e: + return True + elif not (hi < s or lo > e): + return True + return False + + +def line_scoped_result(original_text, formatted_text, ranges): + # reconstruct a file that keeps original content everywhere except on the + # formatting hunks that intersect the PR-edited ranges; returns the new text + # plus the kept hunks (for reporting). Pure function -- no IO. + a = original_text.splitlines(keepends=True) + b = formatted_text.splitlines(keepends=True) + sm = difflib.SequenceMatcher(None, a, b, autojunk=False) + result = [] + kept = [] + for tag, i1, i2, j1, j2 in sm.get_opcodes(): + if tag == "equal": + result.extend(a[i1:i2]) + elif overlaps(i1, i2, ranges): + result.extend(b[j1:j2]) + kept.append((i1, len(a[i1:i2]), len(b[j1:j2]), a[i1:i2], b[j1:j2])) + else: + result.extend(a[i1:i2]) + return "".join(result), kept + + +# --- formatter ----------------------------------------------------------------- + +def run_formatter(files): + includes = ",".join(strip_src_prefix(f) for f in files) + subprocess.run(["mvn", "-q", "-ntp", "-B", + f"net.revelc.code.formatter:formatter-maven-plugin:{FMT_VERSION}:format", + f"-Dconfigfile={os.getcwd()}/{CONFIG}", + "-Dmaven.compiler.source=17", "-Dmaven.compiler.target=17", + f"-Dformatter.includes={includes}"], check=True) + + +def print_report(path, kept): + print(f"\n--- a/{path}") + print(f"+++ b/{path}") + for i1, n_old, n_new, olds, news in kept: + print(f"@@ -{i1 + 1},{n_old} +{i1 + 1},{n_new} @@ (changed lines)") + for ln in olds: + print("-" + ln.rstrip("\n")) + for ln in news: + print("+" + ln.rstrip("\n")) + + +# --- CLI ----------------------------------------------------------------------- + +def parse_args(argv): + mode = "check" + positionals = [] + for a in argv: + if a == "--fix": + mode = "fix" + elif a == "--check": + mode = "check" + elif a.startswith("-"): + sys.exit(f"Unknown option: {a} (usage: format_changed.py [--check|--fix] [base-ref])") + else: + positionals.append(a) + if len(positionals) > 1: + sys.exit(f"Expected at most one base ref, got: {positionals}") + return mode, (positionals[0] if positionals else None) + + +def compute_ranges(mergebase, files, untracked, originals): + batched = changed_ranges(mergebase, files) + ranges_by_file = {} + for f in files: + r = batched[f] + if not r and (f in untracked or not base_has(mergebase, f)): + # brand-new/untracked file has no base version: treat every line as edited + r = [(1, max(1, len(originals[f].splitlines())))] + ranges_by_file[f] = r + return ranges_by_file + + +def main(): + mode, base_arg = parse_args(sys.argv[1:]) + os.chdir(git("rev-parse", "--show-toplevel").strip()) + base = resolve_base(base_arg) + mergebase = merge_base(base) + + files, skipped, untracked = discover_files(mergebase) + if skipped: + print(f"Skipping style-exempt files ({EXCLUDE_FILE}):") + for f in skipped: + print(f" {f}") + if not files: + print(f"No changed Java source files to check (base: {base}).") + return 0 + + originals = {f: read_text(f) for f in files} + ranges_by_file = compute_ranges(mergebase, files, untracked, originals) + + reports = [] + wrote_fix = False + try: + try: + run_formatter(files) + except subprocess.CalledProcessError as e: + sys.exit(f"ERROR: could not run the Eclipse formatter (is `mvn` on PATH?): {e}") + + results = {} + for f in files: + result, kept = line_scoped_result(originals[f], read_text(f), ranges_by_file[f]) + results[f] = result + if kept: + reports.append((f, kept)) + + if mode == "fix": + for f in files: + write_text(f, results[f]) + wrote_fix = True + finally: + # never leave mvn's whole-file reformat on disk: check mode is read-only, + # and fix mode must restore originals unless it fully wrote the scoped results + if mode == "check" or (mode == "fix" and not wrote_fix): + for f in files: + write_text(f, originals[f]) + + if reports and mode == "check": + for path, kept in reports: + print_report(path, kept) + print("\nERROR: the changes above are required on lines this PR edited " + "(per dev/CodeStyle_eclipse.xml).") + print("Fix locally with: dev/format-changed.sh") + return 1 + if reports and mode == "fix": + print("Applied changed-line formatting. Review and commit the result.") + else: + print("All PR-edited Java lines are correctly formatted.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/dev/tests/test_format_changed.py b/dev/tests/test_format_changed.py new file mode 100644 index 00000000000..dda2c26db66 --- /dev/null +++ b/dev/tests/test_format_changed.py @@ -0,0 +1,150 @@ +# ------------------------------------------------------------- +# +# 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. +# +# ------------------------------------------------------------- +# +# Unit tests for the pure logic in dev/format_changed.py (the line-scoping math, +# hunk parsing, and exclude matching). Run with: python -m pytest dev/tests +# +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +import format_changed as fc # noqa: E402 + + +# --- overlaps: replace/delete regions vs edited ranges ------------------------- + +def test_overlaps_replace_region_hits_and_misses(): + # replace of original lines 5..7 -> 0-based half-open [4, 7) + assert fc.overlaps(4, 7, [(6, 6)]) is True # inside + assert fc.overlaps(4, 7, [(5, 5)]) is True # first line of region + assert fc.overlaps(4, 7, [(7, 7)]) is True # last line of region + assert fc.overlaps(4, 7, [(4, 4)]) is False # one before + assert fc.overlaps(4, 7, [(8, 8)]) is False # one after + assert fc.overlaps(4, 7, [(1, 2)]) is False # entirely before + assert fc.overlaps(4, 7, [(8, 9)]) is False # entirely after + + +def test_overlaps_multiple_ranges(): + assert fc.overlaps(4, 7, [(1, 2), (7, 9)]) is True + assert fc.overlaps(4, 7, [(1, 2), (10, 11)]) is False + + +# --- overlaps: pure insertions (i1 == i2) ------------------------------------- + +def test_overlaps_pure_insertion_boundaries(): + # insertion at 0-based index 5 = between 1-based lines 5 and 6 + assert fc.overlaps(5, 5, [(5, 5)]) is True # i1 == e + assert fc.overlaps(5, 5, [(6, 6)]) is True # i1 == s - 1 + assert fc.overlaps(5, 5, [(7, 7)]) is False # below the range + assert fc.overlaps(5, 5, [(3, 4)]) is False # above the range + assert fc.overlaps(0, 0, [(1, 1)]) is True # insert before the first line + + +# --- _hunk_range / parse_hunks ------------------------------------------------ + +def test_hunk_range_multiline(): + assert fc._hunk_range("@@ -1,3 +10,4 @@") == (10, 13) + + +def test_hunk_range_missing_count_defaults_to_one(): + assert fc._hunk_range("@@ -0,0 +5 @@") == (5, 5) + + +def test_hunk_range_pure_deletion_is_none(): + assert fc._hunk_range("@@ -4,2 +3,0 @@") is None + + +def test_hunk_range_non_header_is_none(): + assert fc._hunk_range("+ some added line") is None + assert fc._hunk_range("public int mul(int a) {") is None + + +def test_parse_hunks_collects_all_ranges(): + diff = ( + "diff --git a/X.java b/X.java\n" + "--- a/X.java\n" + "+++ b/X.java\n" + "@@ -1,1 +1,1 @@\n" + "-a\n+a \n" + "@@ -10,0 +11,2 @@\n" + "+x\n+y\n" + "@@ -20,2 +22,0 @@\n" # pure deletion -> skipped + ) + assert fc.parse_hunks(diff) == [(1, 1), (11, 12)] + + +# --- exclude matching --------------------------------------------------------- + +def test_is_excluded_by_full_path(): + p = "src/main/java/org/apache/sysds/conf/DMLConfig.java" + assert fc.is_excluded(p, [p]) is True + + +def test_is_excluded_by_basename_glob(): + p = "src/main/java/org/apache/sysds/conf/DMLConfig.java" + assert fc.is_excluded(p, ["*DMLConfig.java"]) is True + + +def test_is_excluded_no_match(): + assert fc.is_excluded("src/main/java/Foo.java", ["*DMLConfig.java"]) is False + assert fc.is_excluded("src/main/java/Foo.java", []) is False + + +def test_load_excludes_skips_comments_and_blanks(tmp_path, monkeypatch): + f = tmp_path / "exclude.txt" + f.write_text("# a comment\n\n \nsrc/main/java/Foo.java\n *Bar.java \n", + encoding="utf-8") + monkeypatch.setattr(fc, "EXCLUDE_FILE", str(f)) + assert fc.load_excludes() == ["src/main/java/Foo.java", "*Bar.java"] + + +def test_load_excludes_missing_file_returns_empty(tmp_path, monkeypatch): + monkeypatch.setattr(fc, "EXCLUDE_FILE", str(tmp_path / "does-not-exist.txt")) + assert fc.load_excludes() == [] + + +# --- strip_src_prefix --------------------------------------------------------- + +def test_strip_src_prefix(): + assert fc.strip_src_prefix("src/main/java/org/A.java") == "org/A.java" + assert fc.strip_src_prefix("src/test/java/org/A.java") == "org/A.java" + + +# --- line_scoped_result: only edited-line formatting is kept ------------------ + +def test_line_scoped_result_keeps_only_edited_lines(): + # lines 1 and 3 are mis-formatted; an unchanged line 2 separates them so + # difflib yields distinct hunks. Only line 3 is "edited", so line 1 (a + # pre-existing violation the PR did not touch) must stay original. + original = "int a=1;\nint ok = 0;\nint b=2;\n" + formatted = "int a = 1;\nint ok = 0;\nint b = 2;\n" + result, kept = fc.line_scoped_result(original, formatted, [(3, 3)]) + assert result == "int a=1;\nint ok = 0;\nint b = 2;\n" + assert len(kept) == 1 + + +def test_line_scoped_result_no_edited_lines_is_noop(): + original = "int a=1;\nint b=2;\n" + formatted = "int a = 1;\nint b = 2;\n" + result, kept = fc.line_scoped_result(original, formatted, []) + assert result == original + assert kept == []