Java sub-projects have been added, including PCGen-base and PCGen-formula repositories#7563
Merged
Conversation
Restructure the build to make PCGen-base and PCGen-Formula proper Java
modules (JPMS) as Gradle subprojects, replacing the old external JAR
dependencies (net.sourceforge.pcgen:PCGen-base:1.0.170 and
net.sourceforge.pcgen:PCGen-Formula:1.0.200).
Build infrastructure:
- Add 'PCGen-base' and 'PCGen-Formula' to settings.gradle as included projects
- Write new build.gradle for each subproject (java-library, Java 25, JUnit 5)
- Replace external JAR deps with project(':PCGen-base') and project(':PCGen-Formula')
- Remove ivy fileRepo repository (no longer needed)
- Remove PCGen-base/PCGen-Formula from jlink forceMerge (proper named modules now)
- Move JavaCompile/Test task config out of allprojects{} to root-only scope
Module descriptors:
- Create module-info.java for pcgen.base (exports utility packages)
- Create module-info.java for pcgen.formula (requires transitive pcgen.base,
exports formula parser and solver packages)
- Update main module-info.java: requires pcgen.base; requires pcgen.formula
Split-package resolution (JPMS forbids two modules owning same package):
- Move generic formula classes to PCGen-Formula: AddingFormula, DividingFormula,
MultiplyingFormula, SubtractingFormula, ReferenceFormula
- Move generic format/test classes to PCGen-base: Dice, Die, DiceFormat,
InequalityTester
- Delete TypeSafeConstant from main (identical copy exists in PCGen-base)
- Relocate pcgen.base.formula.Formula to pcgen.cdom.formula.Formula (legacy
interface that depends on pcgen.core — cannot live in generic module)
- Relocate pcgen.base.calculation.* to pcgen.cdom.calculation.* (PCGen-specific
adapter layer, not part of the generic solver)
Pre-commit JavaCC parser:
- Generate parser source from formula.jjt using jjtree+javacc 7.0.13
- Place all generated files in PCGen-Formula/code/src/java/pcgen/base/formula/parse/
- No build-time generation needed
Update ~150 files for import changes across main source and tests.
Both subprojects compile successfully. Main source has remaining compilation
errors from API differences between the old published JARs and the current
in-repo source (Phase 6: API migration still needed).
Complete API migration enabling the main module to compile against the in-repo PCGen-Formula and PCGen-base subprojects (current source) rather than the old published JARs. Key API changes applied: - LegalScope -> ImplementedScope (getParentScope -> drawsFrom/isGlobal) - FormulaManager removed; replaced by ManagerFactory pattern - ScopeManagerInst -> ImplementedScopeLibrary (registerScope -> addScope) - VariableStrategy -> DependencyStrategy - Modifier.getDependencies -> captureDependencies; added isValid - NEPFormula.getDependencies -> captureDependencies - ScopeInstance.getLegalScope -> getImplementedScope - VarScoped: removed getLocalScopeName/getVariableParent from interface, added getProviderFor(ImplementedScope) for scope hierarchy navigation - SolverManager: addModifierAndSolve -> addModifier, solveChildren -> processSolver, getDefaultValue -> getDefault - FormatManagerFactory.build and FormatManagerLibrary.getFormatManager signature changes (Optional parameters) - VariableLibrary.getVariableFormat now returns Optional<FormatManager<?>> - ScopeInstanceFactory.get(String, VarScoped) replaces old Optional-based API - FormulaFactory.getValidFormula no longer takes FormulaManager parameter Structural additions: - GlobalPCVarScoped: sentinel VarScoped for PC global scope instances - PCGenScoped.getLocalScopeName(): moved from VarScoped interface to PCGen-specific PCGenScoped interface - ImplementedScopeLibrary.getScopes(): collection accessor for GUI code - VariableContext.validateDefaults(): delegates to SupplierValueStore
Mechanical updates to test/testcommon sources to align with renamed and reshaped APIs in the JPMS subprojects: - Modifier.getDependencies → captureDependencies, added isValid stub - NEPFormula.getDependencies → captureDependencies - LegalScope → ImplementedScope / PCGenScope type references - SimpleScopeInstance constructor: removed Optional parent parameter - AggressiveSolverManager/DynamicSolverManager → SimpleSolverManager - ColumnFormatFactory/TableFormatFactory.build() now takes Optional params Remaining test errors require rewriting AbstractFormulaTestCase (removes FormulaManager) and dependent test classes — left for a separate change.
- Add equals/hashCode to GlobalPCVarScoped to fix ScopeInstance identity mismatches when multiple instances are created across test and production code - Fix scope getName() methods to return fully-qualified names (e.g. "PC.SKILL") matching the registration keys used by ImplementedScopeLibrary - Add getFunctionLibrary() and getOperatorLibrary() accessors to VariableContext for test infrastructure access - Rewrite AbstractFormulaTestCase to use ManagerFactory/ScopeInstanceFactory instead of removed FormulaManager - Update SimpleSolverManager.processSolver to preserve existing store values when no solver is built, preventing VariableChannel.set() from being overwritten by format defaults - Adapt tests to explicitly call processSolver after addModifier since SimpleSolverManager does not auto-evaluate like the old DynamicSolverManager
SimpleSolverManager.processSolver() writes the format default when no solver exists, which overwrites values set via VariableChannel.set() and ChannelUtilities.setGlobalChannel(). This was correct for the old DynamicSolverManager (which auto-cascaded), but incorrect for SimpleSolverManager where channel writes are the final value. - Revert SimpleSolverManager.processSolver() to original upstream logic - Remove processSolver() call from VariableChannel.set() - Remove processSolver() call from ChannelUtilities.setGlobalChannel() - Remove unused SolverManagerFacet field from ChannelUtilities
PCGen-base's StagingProxy.applyTo() reflectively invokes methods on VarHolder (in pcgen.cdom.base). With proper module boundaries enforced, this requires an explicit opens directive.
JPMS modular projects with inferModulePath require the launcher artifact explicitly on the test runtime classpath.
Reverts the change from a7e1952 which added an `instanceof String` branch to equals(). This violates the equals symmetry contract: cis.equals("Foo") returned true but "Foo".equals(cis) always returns false since String is unaware of CaseInsensitiveString. The upstream pcgen-base repo never accepted this change, and the existing CaseInsensitiveStringTest.testString() explicitly asserts that this comparison must be false. Callers that need case-insensitive comparison with a raw String should wrap it in a CaseInsensitiveString first.
…erFacet The Phase 6 API migration (094525d) replaced addModifierAndSolve() with addModifier() but omitted the follow-up processSolver() call that recomputes the variable value. Without it, modifiers were registered but never evaluated, causing GlobalModifyTest to hang waiting for a value that was never computed. Also short-circuit global-scope resolution: when the target scope is global, call scopeFacet.getGlobalScope() directly instead of walking the VarScoped hierarchy — global variables have no meaningful scoped object to traverse. In GlobalModifyTest.targetFacetCount(), guard against an empty diagnose list (which is valid when no solver has been built yet) and remove a stale comment about a missing API method.
When MODIFY is parsed inside a non-global scope (e.g., MODIFY:Face on a SIZE object), VarModifier.getLegalScope() reports the parsing scope, not the variable's actual scope. Following that path produced a ScopeInstance that did not match the one used when reading the variable, so modifiers were applied to a different VariableID than channels read from. Routine the scope through resolveScope(): if the parsed scope is global, or if the variable is legally defined at the global scope, use the global ScopeInstance. Otherwise fall back to the local scope as before. Fixes the Pathfinder FACE tests (which use MODIFY:Face on SIZE objects to set the global Face variable to "5,5") and the GlobalModifyTest identity-mismatch failures.
PlayerCharacter.clone() invokes bean.copyContents(id, aClone.id) on every facet, but AbstractItemFacet.copyContents() copies the underlying map directly without going through set(), so ScopeFacet.set()'s side effect of registering the GlobalPCVarScoped sentinel was skipped on the clone. The cloned character then had a null entry in globalVarScopedMap, and getGlobalScope() handed back a ScopeInstance whose owner was null, causing NullPointerExceptions in VarScoped.getProviderFor() during the 26 pcGenGUI*Test save/restore flows. Override copyContents() to also copy the globalVarScopedMap entry.
- jlink: add `addExtraDependencies('javafx')` so the merged module (which
inherits `requires javafx.graphics` from controlsfx) can resolve JavaFX
modules from `mods/lib` during merged-module compilation.
- jlink: declare `prepareMergedJarsDir.dependsOn copyToLibs` to satisfy
Gradle 9.5's implicit-dependency validation; both tasks read/write
`build/libs/`.
- Test config: move the JavaFX `--module-path mods/lib --add-modules
javafx.*` JVM args out of `allprojects { tasks.withType(Test) }`. The
pure-formula subprojects (PCGen-base, PCGen-Formula) have no `mods/lib`
and don't depend on JavaFX, so applying those args to their tests
crashed JVM startup with `FindException: Module javafx.web not found`.
- Remove unused imports flagged by checkstyle after the JPMS API
migration.
The PCGen-Formula parse package gitignores seven JavaCC-generated files (FormulaParser, FormulaParserConstants, FormulaParserTokenManager, ParseException, Token, TokenMgrError, SimpleCharStream). They were previously produced by an Ant build that no longer runs after the JPMS subproject migration, so clean CI checkouts (including CodeQL's autobuild) failed compileJava with "cannot find symbol: FormulaParser". Add a javacc configuration and two JavaExec tasks (jjtree, javacc) to PCGen-Formula/build.gradle. JJTree converts formula.jjt into an annotated .jj under build/generated/jjtree, then JavaCC emits the seven parser/token classes into build/generated/sources/javacc, which is added as a secondary srcDir of the main source set. The hand-maintained AST/Visitor/Node/SimpleNode/Operator classes in code/src/java remain canonical because JJTree's auxiliary outputs land in a directory not on the source path.
Vest
commented
May 19, 2026
The 16 AST classes plus Node, JJTFormulaParserState, FormulaParserTreeConstants, FormulaParserVisitor, and FormulaParserDefaultVisitor in PCGen-Formula/code/src/java/pcgen/base/formula/parse were byte-identical to JJTree's output from formula.jjt — pure boilerplate that didn't need to live in source control. Wire JJTree's output dir as a secondary srcDir of the main source set, drop JJTree's SimpleNode.java stub in a doLast (the hand-edited SimpleNode in code/src/java is canonical), and remove the 21 redundant files. The .gitignore in the parse package now covers all generated artifacts so they can't accidentally be re-added. Operator.java and SimpleNode.java are the only remaining hand-maintained files in that package.
Note the PCGen-specific Operator/text fields, the ~50 call sites that prevent a rename, and the NODE_CLASS=PCGenBaseNode alternative for any future cleanup.
ImplementedScopeLibrary (new API) keys scopes by scope.getName() directly, whereas the old ScopeManagerInst built the full dotted name from the scope hierarchy. All hardcoded PCGenScope implementations (SkillScope, StatScope, etc.) were updated to return their full name (e.g. "PC.SKILL"), but DynamicScope was missed — it still returned only the category's local name (e.g. "MOVEMENT" instead of "PC.MOVEMENT"). This caused MODIFYOTHER:PC.MOVEMENT|... and LOCAL:PC.MOVEMENT|... tokens to fail with "illegal variable scope" warnings at load time, breaking DataLoadTest for any source that uses dynamic scopes (e.g. Starfinder armor entries using MODIFYOTHER to apply movement penalties).
In the Phase 6 API migration, the call:
instFactory.get(localScopeName.get(), Optional.of(owner))
was replaced with:
SCOPE_FACET.get(id, owner)
ScopeFacet.get(CharID, VarScoped) always uses GlobalPCScope.GLOBAL_SCOPE_NAME
("PC"), so variables declared in child scopes (e.g. CHANNEL*STATSCORE in
PC.STAT) could not be found — VariableManager.getActiveScope only walks
drawsFrom() upward, not into child scopes.
Restore the original intent: get the owner's proper local scope name via
PCGenScoped.getLocalScopeName() and use the ScopeInstanceFactory directly.
Replace wildcard VariableID<?> returns on getLocalVariableID with a generic <T> parameter (S1452), matching the existing pattern on getGlobalVariableID. Replace the unguarded Optional.get() on the owner's local scope name with orElseThrow (S3655), carrying the variable name, owner class, and owner identity in the message so a misrouted call can be diagnosed from the log alone.
Replace defunct AdoptOpenJDK links with Eclipse Temurin (Adoptium). Correct git rebase command: fetch before checkout, and rebase from upstream/master.
After the JPMS subproject migration, subprojects produce their own test results under PCGen-base/ and PCGen-Formula/. Add those paths to the Publish Test Results step so they are not silently omitted.
com.yuvimasory:orange-extensions had no references in any source file. The library exposes Apple-specific eawt APIs that the project does not use; the dependency was carried by the build configuration alone.
Only PcgenFtlTestCase referenced org.xmlunit.*; no production code did. Keeping it on the production module path forced a `requires org.xmlunit;` in module-info and pulled the jar into the runtime image for nothing.
The BASIC_LETTER token definition covers Unicode ranges beyond Latin. Without UNICODE_INPUT=true, JavaCC warns about non-ASCII characters in the generated regex. The option also ensures the parser handles Unicode input correctly when a non-default Reader is used.
Contributor
Author
|
Comment to ca9df42: Build warnings audit — all safe to ignore Three categories of warnings appear in the build output that are not actionable: createMergedModule: Spring ServiceLoader uses clause
The jlink plugin statically analyses bytecode to derive uses clauses for the merged module. AbstractServiceLoaderBasedFactoryBean uses a reflection-based ServiceLoader call that cannot be resolved statically. This Spring class is never instantiated by PCGen, so there is no runtime risk. Silencing it would require adding a speculative uses directive for an unknown service interface. createDelegatingModules: Terminal digits in module names
javac style warning about auto-generated module-info.java for non-modular JARs (commons-lang3, jdom2). The module names are derived from the libraries' own Automatic-Module-Name manifest entries — they are upstream jlink: java.base not found in jmods
JDK 24+ supports JEP 493 (linkable runtime images), which allows jlink to operate without a separate jmods directory. If Temurin JDK is used, it was built with this feature enabled. This is expected behaviour on JDK 25. |
jpackageImage alone skips assembleJpackageImage, leaving data/plugins/ preview/outputsheets out of the bundle. fullJpackage runs the full chain. Also document the macOS .DS_Store workaround.
Pre-late-2024, PCGen used NSIS for the Windows installer. NSIS is
script-driven and platform-agnostic, so a single host could produce
installers for all five target platforms (linux x64/aarch64, mac
x64/aarch64, windows x64). The all-platforms JDK download chain
(downloadJDKs / extractJDKs / downloadJavaFXMods, plus the five-entry
targetPlatform() block in jlink {}) was load-bearing for that workflow,
and PCGEN_ALL_PLATFORMS=true was the switch that turned it on.
NSIS was retired in commits 306cee4..be48763 in favor of
jpackage. jpackage is not platform-agnostic: it shells out to native OS
tooling (hdiutil/pkgbuild on macOS, WiX on Windows, dpkg-deb on Linux),
and the Beryx jlink plugin's mergedModule step invokes the target JDK's
native javac to synthesize a module-info. The moment NSIS left,
cross-platform builds from a single host stopped working - verified
2026-05-19 on macOS aarch64, where PCGEN_ALL_PLATFORMS=true ./gradlew
jlink fails with "jdk_linux_aarch64/bin/javac: cannot execute binary
file".
CI sidestepped this by moving to a per-OS matrix
(.github/workflows/gradle-release.yml) where each runner builds only
its own platform. PCGEN_ALL_PLATFORMS is never set in any workflow and
would fail on every GitHub-hosted runner if it were.
This commit removes the now-dead surface:
- The PCGEN_ALL_PLATFORMS env-var branches in jlink {} and
tasks.named("jlink"); registers only the host targetPlatform().
- Aggregator tasks downloadJDKs, extractJDKs, downloadJavaFXMods.
- The unused 'jre' task (no callers).
Per-platform downloadJdk_*, extractJdk_*, downloadJfxMods_*, and
extractJfxMods_* tasks are kept - they back the host-only path and CI
caches them by glob.
AGENTS.md updated to point at the per-platform task names.
Follow-up to "Remove dead cross-platform build infrastructure". The
prior commit removed the all-platforms aggregator tasks and the
five-entry targetPlatform() block, but the four platforms.each { }
loops were left intact, registering 20 download/extract tasks of
which only the host platform's 4 ever fire.
Collapse the loops to single tasks: downloadJdk, extractJdk,
downloadJfxMods, extractJfxMods. Filenames remain platform-stamped
(jdks/jdk_${hostOs}_${hostArch}.${ext}) so the GitHub Actions cache
glob in gradle-release.yml keeps working - per-runner cache keys
already isolate the linux/mac/windows builds. Remove the platforms
ext list, which existed only to drive the loops.
Lift host-OS/arch detection to a single computation at script scope
(above jlink {}) and reuse it from jlink {}, jpackage {}, and
tasks.named("jlink") instead of recomputing in three places.
Verified by running ./gradlew jlink end-to-end on macOS aarch64:
build/image/pcgen-mac-aarch64/bin/java reports openjdk 25.0.3.
Updates CI workflow comments and AGENTS.md to reference the new flat
task names.
Contributor
Author
|
@karianna ok, let's wait. I hope, it is the last change that I wanted to have in this PR. |
The application plugin's distZip/distTar produce build/distributions/ archives containing the pcgen jar plus ~150 runtime dependency jars and the generated start scripts. Nothing in this project consumes them: - CI publishes the custom 5-zip layout from buildDist (data, docs, program, libs, image) plus jpackage native installers. The release pipeline never reads build/distributions/. - End users install via jpackage-produced .dmg/.pkg/.exe/.deb/.rpm with a bundled JRE, not a raw jar pile that needs a system JDK. - Local dev uses ./gradlew run / qbuild, which don't touch dist*. Disable both with enabled = false. They stay in the task graph (assemble depends on them) but skip their actions, so every build stops materializing ~150 jars into build/distributions/ for nobody to read. Drop distTar/distZip from sourcesJar's dependsOn — that was a stale workaround for a Gradle implicit-dependency warning between copyToLibs and the dist tasks, now moot. sourcesJar's content comes from sourceSets.main.allSource, which is independent of dist*.
- buildonly: registered in 2014, never referenced anywhere in the repo
(no CI, docs, scripts). Functionally a worse qbuild — same dependency
on copyToOutput but missing the actual copy logic that makes qbuild
useful. Pure dead weight.
- quickbuild: same story, never referenced. "build runnable output and
run tests" is just `./gradlew qbuild test` — no dedicated task needed.
- println("IN copyToOutput") / println("IN buildonly"): debug prints
left over from the 2014 Gradle conversion (commit f00f99c). They
spammed stdout on every build with no useful info.
- Stale TODO + commented-out `dependsOn copyToOutput` in the build
task: documenting a non-change from 18 months ago. The decision to
not have build populate output/ is permanent — qbuild fills that
role for devs who want it. Kept mustRunAfter clean (load-bearing)
with a clearer comment.
- Dropped the now-orphan "// Alias tasks" comment.
karianna
approved these changes
May 19, 2026
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
2 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.
Convert PCGen-base and PCGen-Formula from external Maven dependencies to in-tree JPMS subprojects, migrate the main code and tests to their new APIs, and fix the CI release pipeline that the migration uncovered.
The two libraries are now built locally as Gradle subprojects with their own module-info.java, included via implementation project(':PCGen-base') / implementation project(':PCGen-Formula'). This unblocks coordinated
changes across the formula stack without round-tripping through external releases.
Changes
JPMS subprojects
renamed in the main module.
Main-source API migration
Behavioral fixes uncovered by the migration
CI release pipeline
merged-module compilation. Without it :createMergedModule failed with error: module not found: javafx.graphics.
libraries, have no mods/lib, and don't depend on JavaFX — applying those args to their tests crashed JVM startup with FindException: Module javafx.web not found.
Tooling