sunchao commented on code in PR #7311: URL: https://github.com/apache/datafusion-comet/pull/7311#discussion_r4417102275
########## .github/workflows/ci_fork.yml: ########## @@ -0,0 +1,72 @@ +# 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. + +# Fork runs: a push to a branch of a contributor's fork runs the pull request +# tier on the contributor's own GitHub runners, so a fork pull request does not +# spend ASF's shared capacity on it. ci.yml upstream skips the PR tier for a +# fork pull request into main (see `is_fork_pull_request` in +# dev/ci/compute-changes.py), keeping only the preflight checks and the opt-in +# `run-*` suites, and fork_ci_status.yml mirrors this run onto the pull request +# as the informational `CI (fork)` check. The merge queue runs the queue tier on +# the merge result, so a fork run is feedback and never a gate. +# +# This is a separate workflow, and not a wider `push` trigger on ci.yml, for the +# same reason ci_label.yml is one. A skipped job still publishes a check run +# under its own name, and a push to a branch of this repository can land on the +# very commit a pull request is testing. A skipped `Required Checks` from that +# push would compete with the pull request's real verdict. Here the only job +# that can be skipped is named after this workflow's own run, and upstream it is +# always skipped. +# +# A fork's default branch is skipped by the job below, whatever it is called, +# because a push to it is a sync of upstream, which the merge queue already +# tested. A branch called `main` that is not the default is run like any other. +# ci.yml also triggers on a push to `main`, in a fork as upstream, but its +# Preflight job skips that run unless it came through this workflow, so a fork +# push is tested once and the bridge has one workflow to look up. A fork's caches +# are never warm; see "Pull requests from forks" in +# docs/source/contributor-guide/ci.md for what that costs. +name: Comet CI (fork run) + +# No `concurrency` here. ci.yml declares its own group, which for a push to a +# fork is keyed on the branch, so the next push to the branch supersedes this +# one. The same group on the caller as well would deadlock the two. +on: + push: + branches-ignore: + - asf-site + +permissions: + contents: read + +jobs: + ci: + name: Fork run + # Upstream, a push to any branch other than main is a pull request's own + # business: the pull request run tests it, and a second run here would + # test it twice. In a fork the default branch is skipped, whatever it is + # called; `branches-ignore` above can only name a fixed branch. + if: github.repository != 'apache/datafusion-comet' && github.ref_name != github.event.repository.default_branch Review Comment: A fork can have both `main` and `Main`. With `main` as its default, pushing `Main` skips the only job here because Actions compares strings without regard to case. The bridge uses case-sensitive JavaScript equality, so it treats `Main` as a tested branch. The new Python condition checker also returns `True` for this guard on `Main`/`main`, the opposite of Actions. Could the caller and bridge agree on which pushes really run? If case variants are deliberately excluded, the bridge should give them the same explicit untested status as a default-branch PR. [GitHub expression semantics](https://docs.github.com/en/actions/reference/workflows-and-actions/expressions#operators) ########## .github/workflows/ci.yml: ########## @@ -73,9 +80,13 @@ name: Comet CI # each other. Every other run is keyed on its event name: opened, synchronize # and reopened all map to `pull_request`, so a new push still supersedes its # predecessor, while a scheduled run at the tip of main never cancels that -# commit's push run. +# commit's push run. A pull request is keyed on its number, and not on its +# branch name: nearly every pull request comes from a fork, and two forks' +# branches can share a name, which would have one cancel the other's run. A push +# to a fork (ci_fork.yml) is keyed on its branch instead of its commit, so the +# next push to the branch cancels the run in flight. concurrency: - group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} + group: ${{ github.repository }}-${{ github.event.pull_request.number || (github.event_name == 'push' && github.repository != 'apache/datafusion-comet' && github.ref) || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} Review Comment: In one fork, `feature` and `Feature` are distinct Git branches. Their push concurrency keys differ only by case, which GitHub treats as the same group. With `cancel-in-progress: true`, pushing to the second branch can cancel the first PR’s build. Could the fork push key distinguish the two refs under GitHub’s group comparison, or handle that collision without cancelling the unrelated build? The existing group test compares JavaScript strings directly, so it misses this case. [GitHub concurrency semantics](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#concurrency) ########## dev/ci/check-ci-config.py: ########## @@ -1560,6 +1651,243 @@ def check_cache_save_scope(): return not failures +# Fork runs. A push to a contributor's fork runs the PR tier there (ci_fork.yml), +# upstream skips that tier for a fork pull request (compute-changes.py), and +# fork_ci_status.yml mirrors the fork's run onto the pull request. The pieces +# are in three files and break silently apart: a caller that also fires upstream +# runs the PR tier twice, a caller that fires on main runs it twice in a fork, a +# caller that cannot start leaves a fork pull request with no PR tier anywhere, +# and a bridge that reads the wrong workflow name reports "no fork CI" on every +# pull request. +FORK_WORKFLOW = WORKFLOWS / "ci_fork.yml" +BRIDGE_WORKFLOW = WORKFLOWS / "fork_ci_status.yml" +FORK_TEST = Path("dev/ci/test-fork-ci-status.cjs") +FORK_PUSH_SCRIPT = Path("dev/ci/fork-push-changed-files.sh") +FORK_PUSH_TEST = Path("dev/ci/test-fork-push-changed-files.py") + + +def workflow_name(path): + match = re.search(r"^name:\s*(.+?)\s*$", path.read_text(encoding="utf-8"), re.M) + return match.group(1) if match else None + + +def check_fork_runs(): + failures = [] + ci = (WORKFLOWS / "ci.yml").read_text(encoding="utf-8") + + # ci.yml itself must stay a push-to-main trigger: it is what runs on a + # fork's main, and a wider one would run the PR tier twice on every push to + # a fork's branch and publish skipped `Required Checks` upstream. + if not re.search(r"^ push:\n branches:\n - main\n", ci, re.M): + failures.append( + "ci.yml must trigger on `push` to `branches: [main]` only; " + "fork branches go through ci_fork.yml" + ) + # Preflight is where a fork run that is not a pull request's tier is dropped, + # and with it the whole run, since every other job needs Preflight through + # `changes`. ci.yml triggers on push to main, so it fires in a fork for a + # branch called `main` and for a sync of the default branch; only a push that + # came through ci_fork.yml, whose `github.workflow` is its own name, is tested. + ci_name = workflow_name(WORKFLOWS / "ci.yml") + fork_name = workflow_name(FORK_WORKFLOW) + failures.extend( + condition_failures( + WORKFLOWS / "ci.yml", + "preflight", + [ + (actions_context("schedule", UPSTREAM, ci_name), True, "the upstream nightly"), + (actions_context("schedule", FORK, ci_name), False, "a scheduled run in a fork"), + (actions_context("push", UPSTREAM, ci_name), True, "a push to upstream main"), + (actions_context("push", FORK, ci_name), False, "ci.yml's own push trigger in a fork"), + (actions_context("push", FORK, fork_name), True, "a push through ci_fork.yml"), + (actions_context("pull_request", UPSTREAM, ci_name), True, "a pull request"), + (actions_context("pull_request", FORK, ci_name), True, "a pull request inside a fork"), + (actions_context("merge_group", UPSTREAM, ci_name), True, "the merge queue"), + (actions_context("workflow_dispatch", FORK, ci_name), True, "a dispatch in a fork"), + ], + ) + ) + # `needs.preflight.result` is how the aggregator follows Preflight. + aggregator = job_condition(job_blocks(WORKFLOWS / "ci.yml").get(AGGREGATOR_JOB, "")) + if not aggregator or "needs.preflight.result" not in aggregator: + failures.append( + f"ci.yml's {AGGREGATOR_JOB} must skip itself when Preflight does " + f"(`needs.preflight.result != 'skipped'`), or a fork's skipped run still starts a runner" + ) + for needed in (FORK_TEST.name, FORK_PUSH_SCRIPT.name, FORK_PUSH_TEST.name): + if needed not in ci: + failures.append(f"ci.yml does not use or run {needed}, so a fork push goes untested") + + if not FORK_WORKFLOW.exists(): + failures.append(f"{FORK_WORKFLOW} is missing; a fork pull request would run no PR tier anywhere") + else: + body = FORK_WORKFLOW.read_text(encoding="utf-8") + if not CALLS_CI.search(body): + failures.append(f"{FORK_WORKFLOW} does not call ./.github/workflows/ci.yml") + ignored = re.search(r"^ branches-ignore:\n((?: .*\n)+)", body, re.M) + if not ignored or "- asf-site\n" not in ignored.group(1): Review Comment: I restored `- main` alongside `- asf-site` under the caller’s `push.branches-ignore`. The full config check and all 37 bridge/concurrency tests still passed. That one-line change prevents the `develop`-default / `main`-source workflow from ever starting, before its job `if` or the mocked bridge gets a chance to run. Could this check pin the intended exclusion list, or test that a push to `main` is admitted by the trigger as well as the job guard? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
