Optimize build and CI pipeline, remove redundant or dead code#7558
Closed
Vest wants to merge 311 commits into
Closed
Optimize build and CI pipeline, remove redundant or dead code#7558Vest wants to merge 311 commits into
Vest wants to merge 311 commits into
Conversation
Bumps org.apache.xmlgraphics:fop from 2.8 to 2.9. --- updated-dependencies: - dependency-name: org.apache.xmlgraphics:fop dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Added some logging.
Removed explicit System.exit(0) in Main. It allows to enable "slowtest" that generates and compares XMLs. Marked SystemExitInterceptor as deprecated and removal=true. Since Java does not offer any alternative, we have to find a better solution. P.S. NSIS is planned to be removed to use jpackage only.
…s StackTrace to determine the logger name. This is expensive and replaced with an explicit logger creation.
…methods that return null. In some cases, the null reference/result was not checked, but the code never failed, because it was not executed. There were no tests, that is why it was difficult to check, whether the refactoring was right :(
Update gh_build
- Remove fat jar: dependencies are now separate jars handled by jlink's forceMerge rather than bundled into pcgen.jar - Add xmlresolver to forceMerge to resolve package export conflict - Replace --patch-module with --module-path for compileJava: all dependencies are now visible as proper modules - Add requires directives for all runtime dependencies in module-info.java so the compiler can resolve them on the module-path
- Remove --patch-module hack from compileJava, place all dependencies on --module-path so IntelliJ and Gradle use the same module resolution - Add requires directives to module-info.java for all runtime deps (commons-lang3, commons-io, fop-core, fop-events, xmlgraphics-commons, spring, freemarker, jdom2, argparse4j, controlsfx, xmlunit, Saxon, jep) - Open additional packages to Spring for reflective bean instantiation - Add sourceSets.test.output to itest/slowtest compile classpaths so IntelliJ maps the test module dependency at Compile scope, not Runtime
The plugin was causing configuration cache warnings (SourceSetContainer serialization). JavaFX modules are already managed manually via downloadJavaFXLocal/extractJavaFXLocal tasks and explicit --module-path args. The jlink task now depends on downloadJavaFXMods directly to get JavaFX jmods into the target JDK.
Without the javafxplugin, IntelliJ can't find javafx.controls etc. Adding the local mods/lib jars as implementation dependencies makes them visible to the IDE while Gradle still uses --module-path.
…issues - Exclude module-info.java from Checkstyle (it uses syntax Checkstyle can't parse) - Remove redundant same-package import in RandomUtil.java - Fix incorrect @param tag in AbstractToken.isValidXMLChar()
Set maxParallelForks to half the available cores for utest and itest. Slowtest and integration test variants intentionally keep the global default of 1 since they share data files and call Main.main() per test.
…test forks calculation Signed-off-by: Vest <Vest@users.noreply.github.com>
Drop the dead Ivy repo for PCGen Base/Formula sourceforge jars (we now get those from project dependencies) and the freehep maven repo, which was already disabled and never re-enabled.
The localOnly property is not referenced from any Gradle script, source, or properties file. The commented-out 6.09.06-SNAPSHOT version is obsolete history.
The updateVersionRelease and updateVersionToNext tasks called a commitFile() helper that was never defined anywhere in the build, so they would have failed at runtime. Drop the broken calls and leave a comment that the version change should be committed manually.
testZip was registered but never depended on by any other task or workflow, and jreImage was defined but never applied via copySpec.with. Both are leftovers from the pre-jpackage distribution layout.
The NSIS-based Windows installer was retired in favor of jpackage, but the supporting copySpecs (baseLibs, lib32, lib64, pdfLibs, basePlugins, gmgenPlugins, nonPdfOutput, pdfOutput, baseData, optionalData), the nsisBaseFolder/nsisOptionFolder paths, the installerVerNum derivation, and the genDataList task that wrote installers/win-installer/data.nsh were left behind. None of them are referenced anywhere outside release.gradle. Drop them along with the now-unused FixCrLfFilter and DefaultCopySpec imports.
programDistsImage copied build/launch4j/pcgen.exe, but the launch4j plugin was removed in favor of jpackage and that directory is never produced. The copySpec contributed zero files to programZip and installDist, so drop both the copySpec and its applications.
The TestFX/Monocle-specific flags (testfx.* sysprops, prism.order=sw, the four --add-exports/--add-opens for monocle, prism.verbose) were applied to every Test task via configureEach, even though only the unit test source set (code/src/utest) contains TestFX-based tests. itest and slowtest don't use TestFX and don't need them. Move the TestFX-specific args onto the test task itself. Keep the JavaFX module loading args (--module-path, --add-modules, native access) on configureEach because the production code still requires the JavaFX modules to be on the path for class loading. Also drop -Dprism.verbose=true so test output stays quieter.
code/src/testcommon was listed as a srcDir in three source sets (test, itest, slowtest), so its 22 files were compiled three times. Extract it into its own testcommon source set; have test/itest/slowtest consume its compiled output via classpath instead. Wire testcommonImplementation/RuntimeOnly/CompileOnly to extend the matching test configurations so testcommon picks up the same JUnit and test-helper dependencies.
Previously each junit-platform/junit-jupiter coordinate listed its own version, with junit-platform-runner pinned to a stale 1.14.4 while the others moved to 6.0.3. junit-platform-runner is for legacy @RunWith(JUnitPlatform.class) interop and isn't used anywhere, so drop it. Adopt org.junit:junit-bom:6.0.3 to source the version once for all junit-platform / junit-jupiter modules. About 870 tests still use the JUnit 4 API. They were compiling before because junit-platform-runner pulled junit:junit transitively. Add an explicit junit:junit:4.13.2 declaration plus junit-vintage-engine so those tests keep compiling and running on the JUnit Platform.
The five per-game inttest variants (sfinttest, pfinttest, rsrdinttest, srdinttest, msrdinttest) were copy-pasted with only the include pattern differing. Replace them with a map-driven loop. Task names are stable (CI references pfinttest), and the resolved configuration is identical. Pre-existing: the include pattern uses a "slowtest/" path prefix that doesn't exist in compiled classes, so these tasks have always reported NO-SOURCE. Behavior is preserved here; fixing the pattern is a separate change.
github/codeql-action v2 has been deprecated; runs emit warnings and will fail once GitHub retires the v2 endpoints. v3 is a drop-in upgrade for init, autobuild, and analyze. actions/checkout is bumped to v4 to match the other workflows in this repo.
The primary cache key was prefixed with \${{ runner.os }} (e.g. Linux,
macOS, Windows) while the restore-keys used \${{ matrix.os }} (e.g.
ubuntu-latest, macos-latest). Restore-keys must be a prefix of the
primary key to ever match, so the fallback restore was dead code and
every run cold-started build/jre and build/libs.
Both gradle-release.yml and gradle-release-manual.yml declared the same workflow name "Create Release with Manual Tag", which made them indistinguishable in the Actions UI. The manual one creates a tag on demand; this one fires when a tag is pushed (or dispatched against an existing tag). Renaming this one to reflect that.
./gradlew build already runs the test task via the standard Java lifecycle (build → check → test). The follow-up step then re-invoked test alongside itest/datatest/slowtest. The re-invocation is a no-op under up-to-date checks, but it still spins up a fresh Gradle daemon step and clutters the run summary. Drop it and rename the step to reflect what it actually adds.
cache-disabled: false and cache-read-only: false are setup-gradle's defaults. cache-overwrite-existing: true is actively harmful for PR workflows: it forces every PR run to overwrite the shared Linux-runner Gradle cache, so concurrent PRs trash each other's warm caches. The intended behavior on PRs is to read the cache populated by master and write back only if the run is on the default branch — which is exactly what setup-gradle does by default.
The compound was:
./gradlew clean build copyToOutput test compileSlowtest \
datatest pfinttest allReports buildDist prepareRelease pcgenRelease
Several entries were already in the dependency graph of pcgenRelease:
- pcgenRelease dependsOn prepareRelease, assembleArtifacts, checksum
- prepareRelease dependsOn build
- assembleArtifacts dependsOn build, jlinkZip, sourcesJar
- sourcesJar dependsOn classes, copyToOutput, distTar, distZip, startScripts
- build (Java lifecycle) dependsOn check -> test
So build, test, copyToOutput, and prepareRelease were all redundant
when pcgenRelease was invoked at the end of the same command. The
fact that they ran first only meant Gradle short-circuited later
invocations under up-to-date checks, which still costs daemon time
and obscures the failure point if any single step fails.
The split makes each stage's purpose explicit and gives the GitHub
Actions UI a real timeline of where time is being spent.
Each workflow stacked three caches:
1. actions/setup-java cache: gradle -> caches ~/.gradle/{caches,wrapper}
2. gradle/actions/setup-gradle@v4 -> caches ~/.gradle/{caches,wrapper,configuration-cache}
3. actions/cache@v4 for build/jre + build/libs
Layers 1 and 2 cover the same paths; setup-gradle is the canonical
choice and adds Gradle-aware features (configuration cache, dep
reports, write-once-on-default-branch semantics). Layer 3 caches
*build outputs* across runs, which:
- is unsafe (stale jars from a different commit can leak in)
- is unused (build/jre is no longer produced; the 'jre' task is a
no-op aggregator that just dependsOn downloadJavaFXMods)
- duplicates Gradle's own up-to-date checks for build/libs
Drop layers 1 and 3, keep setup-gradle. Release workflows keep their
cache-overwrite-existing: true since they run from tagged commits on
master and should refresh the shared cache.
Both release workflows produced artifacts in build/release/ but only uploaded a subset of them as workflow artifacts (and only for macOS), leaving the actual GitHub Release empty. The original intent was sketched out in commented-out softprops/action-gh-release blocks at the bottom of gradle-release.yml. This wires the release.gradle output (sources jar, image zips, jpackage installers) directly to the GitHub Release using the same tag the create_release job used. softprops/action-gh-release is idempotent against an existing release with the same tag_name, so each matrix OS appends its own platform-specific artifacts. Workflow artifacts are kept too, so debugging a failed release run still works without having to publish.
Signed-off-by: Vest <Vest@users.noreply.github.com>
Contributor
Author
|
I am closing this PR. Use #7559. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pure cleanup of the Gradle build script and CI workflows on top of
kar_gh_build_gradle_deps. No application code changes — every commit either removes dead code, fixes a latent bug, or makes the build faster / more debuggable.20 small, focused commits. Net: +125 / −477 across 8 files.
What was done
Build performance
testanditestnow usemaxParallelForks = max(1, processors / 2)instead of runningserially. Big win on multi-core runners.
testcommonpromoted to a dedicated source set — was duplicated as asrcDirinsidetest,itest, andslowtest, causing the same classes to be compiled three times.junit-jupiter-*version pins withplatform('org.junit:junit-bom:6.0.3'). JUnit 4 and Vintage retained explicitly because ~870 legacy tests still use the JUnit 4 API.test— they were applied to everyTesttask viatasks.withType(Test).configureEach, which pulled headless-toolkit args into integration and slow tests that don't use TestFX.inttesttasks —sfinttest,pfinttest,rsrdinttest,srdinttest,msrdinttestcollapsedfrom copy-pasted definitions into a map-driven loop. Names preserved because CI references
pfinttest.converterJarrefactor for clarity.Dead code removal
commitFile()calls inupdateVersionRelease/updateVersionToNext(the function was never defined — thesetasks would have thrown if invoked).
testZiptask (never depended on).jreImageandprogramDistsImagecopySpecs (never applied; the latter pointed at abuild/launch4j/pcgen.exepath that hasn't existed since the jpackage migration).release.gradle(nsisBaseFolder,baseLibs,lib32/lib64,pdfLibs,basePlugins,gmgenPlugins,genDataListtask, etc.). NSIS was superseded by jpackage long ago; this scaffolding was dead.localOnlyproperty.CI workflows (
.github/workflows/)actions/checkoutv3 → v4,github/codeql-action/{init,autobuild,analyze}v2 → v3 (v2 isdeprecated and will stop working when GitHub retires the endpoints).
restore-keysmismatch fixed — release workflows had primary keys prefixed withrunner.os(e.g.Linux) butrestore-keyswithmatrix.os(e.g.ubuntu-latest). Restore-keys must be a prefix of the primary key to ever match,so the fallback restore was dead code and
build/jre/build/libscold-started every run.setup-java cache: gradle+setup-gradle@v4+ manualactions/cacheforbuild/jreandbuild/libs. Layers 1 and 2 cover the same paths; layer 3 cached build outputs across runs (unsafe — stale jars could leak between branches — andbuild/jreis no longer produced anyway). Now justgradle/actions/setup-gradle@v4, the canonical choice.cache-overwrite-existing: trueremoved from PR workflow — it forced every PR to overwrite the shared cache, so concurrent PRs were trashing each other's caches. Kept on release workflows since those run from tagged commits on master and should refresh.testtwice —./gradlew buildalready runstestvia the Java lifecycle; the follow-up step then re-invoked it alongsideitest datatest slowtest.clean build copyToOutput test compileSlowtest datatest pfinttest allReports buildDist prepareRelease pcgenReleasehad four redundant tasks (all inpcgenRelease's dependency graph) and gave the Actions UI no useful timeline. Split into 5 named stages so failures point at the actual broken stage.softprops/action-gh-releasestep hadfiles:commented out, so releases were created empty. Workflow artifacts were uploaded only for macOS, dropping Linux and Windows outputs on the floor. Now every matrix OS attachesbuild/release/*to the existingrelease (idempotent across the matrix because
tag_nameis shared).gradle-release.ymlandgradle-release-manual.ymldeclared the samename: Create Release with Manual Tag. Renamed the tag-triggered one toCreate Release on Tag Push.@karianna this PR should be merged after you merge #7556.
I know, I have polluted this PR with many commits, that's why I'd suggest you to squash the PR as well as do the same for #7556.
I hope, these changes are still bearable, because some of them I took from Claude.