Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ci] New optimized workflow for pull requests #5524

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adangel
Copy link
Member

@adangel adangel commented Feb 13, 2025

Describe the PR

The goal is

  • to have faster feedback for PRs
  • better visibility which part of the workflow failed
  • have build artifacts available to verify/test locally

The jobs are:

  • "compile": First a fast compile job. The result of this job are three artifacts:
    • the compilation results (everything in the target/ directories) is available in "compile-artifact"
    • a "staging-repository" with the currently built pmd artifacts. This is used by the dogfood job later.
    • a "dist-artifact" which contains the binary distribution files, ready to be downloaded from the workflow page, so that one can try out the built PMD from the PR branch without the need to built itself.
    • additional, the local maven repo is cached for the other jobs.
  • Then in parallel a bunch of other jobs:
    • "verify": runs a complete ./mvnw verify with all code checks, etc. This is only run on linux. It reuses the already compiled artifacts from the first "compile" job.
      • This runs very long (>10min). In future, this could be split up further e.g. to run checkstyle, japicmp, javadoc in parallel jobs.
    • "verify-unittests": just runs the unit tests on linux, windows and macos
      • unfortunately these jobs can't reuse the already compiled artifacts from the first "compile" job due to file timestamp issues and platform specific line endings. So PMD needs to be compiled again for windows/macos. For linux, we can reuse the "compile-artifact"
    • "dogfood": runs maven-pmd-plugin on PMD with the latest changes from the pull request
      • it uses the "staging-repository" artifact
    • "documentation": generates the rule documentations and builds PMD's documentation page using jekyll. Also executes the verification for wrong rule tags and dead links.
      • creates the artifact "docs-artifact"
    • "regressiontester": runs the pmdtester to produce the regression report
      • uses the artifact "dist-artifact" so that we don't need to build PMD again
      • doesn't use the same maven cache as the other jobs, but creates a new one. The cache contains the test projects (like spring framework) and their dependencies.
      • uses danger to run pmdtester
      • produces the artifact "pmd-regression-tester" which the regression report, if there were any changes.

Since most jobs can be run in parallel and not sequentially, the results of the PR build should be available faster.

The concurrency control makes sure, that we cancel any in-progress jobs for the current pull request to avoid unnecessary builds. Only the latest build matters.

This new workflow "pull-requests.yml" now doesn't depend on the shell script in build-tools, so it is hopefully easier to maintain. The workflow only uses read permissions and doesn't require any extra secrets.

Right now, I reused Danger for running pmdtester. Midterm however, I'd like to migrate away from that, since it can't be used in a secure way: To upload the report to some hosting facility, we need a secret, but in the PR run, we don't have access to the secrets. Or in order to set the status of the pull request, we would need write access to the repo. That's why Danger says in the logs "Danger does not have write access to the PR to set a PR status."... Midterm, "pull-requests.yml" should only run pmdtester and create the report and maybe a status file with the messages and upload these as artifacts. Then it should trigger another workflow run that runs in the context of our repository and this one has write access and can then properly update the PR status. See https://stackoverflow.com/questions/69499645/how-to-securely-allow-github-actions-to-check-pr-and-post-results-in-comment/71366152#71366152 for more info on that topic.
These changes are out of scope for this PR however and can follow some time later.

If this pull-requests.yml workflow works, I'd like to rewrite the main build workflow in a similar way, so that we have the same benefits: faster feedback and better visibility which part of the build broke. The release workflow should also be separate.

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

The goal is
- to have faster feedback for PRs
- better visibility which part of the workflow failed
- have build artifacts available to verify/test locally

The jobs are:
- "compile": First a fast compile job. The result of this job is
  shared via compile-artifact with some of the following jobs.
- Then in parallel a bunch of other jobs
  - verify: runs a complete `./mvnw verify` with all code checks,
    etc. This is only run on linux.
  - verify-unittests: just runs the unit tests on linux, windows
    and macos
  - dogfood: runs maven-pmd-plugin on PMD with the latest changes
    from the pull request
  - documentation: generates the rule documentation and builds PMD's
    documentation page with jekyll.
  - regressiontester: runs the pmdtester to produce the regression
    report

Since most jobs can be run in parallel and not sequentially, the
results of the PR build should be available faster.

The concurrency control makes sure, that we cancel any in-progress
jobs for the current pull request to avoid unnecessary builds.
Only the latest build matters.

Refs: pmd#4328
@adangel adangel added this to the 7.11.0 milestone Feb 13, 2025
@adangel adangel added the in:pmd-internals Affects PMD's internals label Feb 13, 2025
@pmd-test
Copy link

1 Message
📖 No regression tested rules have been changed.

Generated by 🚫 Danger

@oowekyala
Copy link
Member

This is a great idea :) It seems to work well, this PR finished its checks in 17mins, compared to at least 35 mins for current PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in:pmd-internals Affects PMD's internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants