Skip to content

ci: add multi-platform build tests and system-test workflows#6574

Merged
kuny0707 merged 10 commits intotronprotocol:developfrom
0xbigapple:feature/migrate_ci_1
Mar 11, 2026
Merged

ci: add multi-platform build tests and system-test workflows#6574
kuny0707 merged 10 commits intotronprotocol:developfrom
0xbigapple:feature/migrate_ci_1

Conversation

@0xbigapple
Copy link
Contributor

@0xbigapple 0xbigapple commented Mar 10, 2026

Summary

This PR enhances the CI pipeline by introducing multi-platform build tests, Checkstyle and system tests workflows.

Changes

1. Multi-platform build tests

Add build tests across multiple platforms and architectures:

  • Ubuntu — x86_64 + JDK 8 and aarch64 + JDK 17
  • macOS — x86_64 + JDK 8 and aarch64 + JDK 17
  • Debian — Debian 11 build test using the eclipse-temurin:8-jdk base image.

2. Rocky Linux build test

Since CentOS 7 and CentOS 8 images are not supported in GitHub-hosted runners, as described in the
GitHub-hosted runners documentation, a Rocky Linux build test has been added as a compatible replacement for CentOS 8.

3. Checkstyle workflow

Add a workflow to perform Checkstyle code style checks.

4. System test workflow

Add a workflow to execute a subset of daily build test cases from https://github.com/tronprotocol/system-test.

The workflow performs the following steps:

  • Start a network with one node including two SRs
  • Test core APIs
  • Deploy smart contracts
  • Trigger smart contract execution

5. Upgrade CodeQL from v3 to v4

pull_request:
branches: [ 'develop', 'release_**' ]
types: [ opened, edited, synchronize, reopened ]
types: [ opened, synchronize, reopened ]
Copy link
Contributor

@Sunny6889 Sunny6889 Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing edited from the trigger types means pr-lint won't re-run when the PR title or description is modified after opening. A contributor could pass the lint check initially, then edit the title to a non-conventional format before merging.

If the goal is to avoid unnecessary build/test runs on description edits, consider keeping edited in the workflow trigger and adding if: github.event.action != 'edited' to the build/test jobs so they skip edit-triggered runs while pr-lint still re-validates on every edit.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I will optimize it.

- name: Prepare checkstyle config copy
run: |
set -euxo pipefail
cp -f config/checkstyle/checkStyle.xml config/checkstyle/checkStyleAll.xml || true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why here || true ?also the same question for other two places

Copy link

@liuyifei001 liuyifei001 Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this step begins with set -euxo pipelinefail, any command returning a non-zero status will immediately terminate the script. true ensures that the script will continue execution even if checkStyle.xml does not exist (cp fails). it can delete here.

./gradlew --stop || true It cannot be removed. If there is no running daemon, --stop will return a non-zero value, which needs to be considered to prevent step failure.


on:
pull_request:
branches: [ 'develop', 'release_**' ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason the master branch isn’t considered?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I will add it later.

run: ./gradlew :framework:checkstyleMain :framework:checkstyleTest :plugins:checkstyleMain

- name: Upload Checkstyle reports
if: failure()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkstyle {
    toolVersion = "${versions.checkstyle}"
    configFile = file("config/checkstyle/checkStyleAll.xml")
    // no failure condition configured
}

Since checkStyleAll.xml sets <property name="severity" value="warning"/>, all violations are reported as warnings. Gradle's Checkstyle plugin only fails the task on error-level violations, so the task always succeeds regardless of how many violations exist. The CI job is effectively a no-op — violations never block PR merges.

Two Fix Options

Option A: Change severity to error in checkStyleAll.xml

Violations are promoted to error level, causing the Gradle task to fail by default (ignoreFailures=false).

<property name="severity" value="error"/>

Option B: Add maxWarnings = 0 in Gradle

checkstyle {
    toolVersion = "${versions.checkstyle}"
    configFile = file("config/checkstyle/checkStyleAll.xml")
    maxWarnings = 0
}

Copy link

@liuyifei001 liuyifei001 Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we fix it as suggested above, the behavior may differ from the previous implementation. But we can try.

checkstyle {
toolVersion = "${versions.checkstyle}"
configFile = file("config/checkstyle/checkStyleAll.xml")
maxWarnings = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plugins need this, too.

os-name: macos
arch: x86_64
- java: '17'
runner: macos-latest

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor thought: since macos-latest is a floating alias that could shift to a newer version over time, would it make sense to pin it to macos-26 to stay consistent with macos-26-intel?

It looks like macos-26 is already generally available (actions/runner-images#13739).
If there's a specific reason for using macos-latest here, I'd love to learn more about it!

Copy link

@liuyifei001 liuyifei001 Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there available image for macos-26 aarch64 ? I will fix it as macos-26.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, according to actions/runner-images#13739, macos-26 points to Apple Silicon (aarch64) by default.

Thanks for the quick fix!

name: PR Check

on:
push:
Copy link
Contributor

@waynercheung waynercheung Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a bug: the pr-lint will crash on push events.

This workflow(line 15) triggers on both push (line 4-5) and pull_request (line 6-8).

The script accesses context.payload.pull_request.title, but on push events context.payload.pull_request is undefined, which throws a TypeError and marks this job as failure. Since downstream jobs (build, docker-build-*) depend on pr-lint via needs, they will all be skipped on push events.

Suggest splitting into two workflows:

  • pr-check.yml: triggered by pull_request only, includes pr-lint + checkstyle + builds.
  • post-merge.yml: triggered by push only, includes checkstyle + builds (no pr-lint).

This avoids the need for conditional if / always() workarounds on the needs chain.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you issue. I fix it as this: skip pr-lint when push; When the PR lint job succeeds or is skipped, the jobs depending on it can still run.

jobs:
pr-lint:
name: PR Lint
if: github.event_name == 'pull_request'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix works: adding if: github.event_name == 'pull_request' to pr-lint and !failure() to downstream jobs correctly handles the push/PR scenarios.

One remaining issue: !failure() also returns true when jobs are cancelled. If a user manually cancels the workflow, build jobs will still start. Suggest adding !cancelled():
if: github.event.action != 'edited' && !failure() && !cancelled()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users cannot manually cancel only the PR lint job. When the PR is updated, the running PR lint job will be canceled and all jobs will be triggered again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. You're right that the concurrency group handles the PR update case well — the entire old run is cancelled and a new one starts fresh.

The !cancelled() is more about the manual "Cancel workflow run" case from the Actions UI. In that scenario, !failure() alone would briefly allow pending jobs to start before GitHub cancels them again. The practical impact is minimal — at most a few seconds of wasted runner time.

Adding !cancelled() is a best practice for correctness, but I agree it's not a blocking issue. Feel free to address it or leave as-is.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback. There is no strong need to add !cancelled() now. If a similar scenario occurs during execution, we can add it later.


- name: Upload test reports
if: failure()
docker-build-rockylinux:
Copy link
Contributor

@waynercheung waynercheung Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can add timeout-minutes: 60 as the system-test job.
Besides, build and docker-build-debian11 can also add timeout-minutes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add it.

java-version: '8'
distribution: 'temurin'

- name: Clone system-test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using raw git clone + git checkout has a few drawbacks:

  1. The release_workflow branch is hardcoded — if it's deleted or renamed, CI breaks with no clear error.
  2. No GitHub token auth — will fail if the repo ever becomes private.
  3. No shallow clone optimization.

Suggest using actions/checkout instead:

  - name: Clone system-test
    uses: actions/checkout@v4
    with:
      repository: tronprotocol/system-test
      ref: release_workflow
      path: system-test

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize it.

- name: Run system tests
working-directory: system-test
run: |
cp solcDIR/solc-linux-0.8.6 solcDIR/solc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the system-test repo updates the solc version or changes the directory structure, this cp will fail and may produce confusing downstream test errors.

Suggest adding an existence check:

  if [ ! -f solcDIR/solc-linux-0.8.6 ]; then
    echo "ERROR: solc binary not found at solcDIR/solc-linux-0.8.6"
    exit 1
  fi
  cp solcDIR/solc-linux-0.8.6 solcDIR/solc

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add it.

path: |
~/.gradle/caches
~/.gradle/wrapper
key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle', '**/gradle-wrapper.properties') }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache key glob patterns are inconsistent — checkstyle and build(line 174) use **/*.gradle, while rockylinux(line 213) and debian11(line 274) use **/*.gradle*. Both work the same for this project, but suggest standardizing to one pattern for maintainability.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter much. If needed, I will change it next time.

checkstyle:
name: Checkstyle
runs-on: ubuntu-latest
runs-on: ubuntu-24.04-arm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkstyle is pure static analysis and architecture-independent.
Why do you use ARM instead of ubuntu-latest (x86)?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a pinned version instead of a mutable version.

- name: Build
run: ./gradlew clean build -x test

checkstyle:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (non-blocking): Currently checkstyle is only enabled for framework and plugins. It might be worth considering extending it to other modules (actuator, chainbase, common, etc.) in a follow-up PR to enforce consistent code style across the whole project.

Copy link

@liuyifei001 liuyifei001 Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. The current purpose is to migrate it from buildkite to workflow. Might extends it in another PR.

Copy link
Contributor

@waynercheung waynercheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kuny0707 kuny0707 merged commit 3fc3b53 into tronprotocol:develop Mar 11, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants