JSBML fork upgrade plan: catch up to upstream, and what to send back

Start with Step 0: add jitpack.yml with jdk: openjdk8 in the fork (virtualcell/vcell-jsbml), since the build log quoted here shows publish silently relies on JitPack's JDK 8 default and -DskipTests. Then read the root pom's jsbml.version and the two cited issues #1461 and #1971 to see which fork patch survives an upgrade. Done means the fork delta is down to the single ParserManager.getManager() synchronization patch, with the other candidates individually verified against vcell-core usage before porting.

We carry a fork of JSBML at virtualcell/vcell-jsbml, consumed via JitPack as com.github.virtualcell.vcell-jsbml:{jsbml-core,jsbml-spatial} (jsbml.version in the root pom, currently 1.6.1-VCELL-4). This issue is the plan for catching that fork up to upstream, and the list of changes worth sending upstream.

Filed after #1461 and #1971 — two bugs whose fixes sat in the fork for months without reaching a published artifact.

Where we are

Fork master vs sbmlteam/jsbml master: 23 ahead, 122 behind. Of the 122, 68 touch core/src, which is what we actually consume:

area	commits
util	28
math	12
xml	10
AbstractSBase.java	7
ext, validator, SBase, SBMLDocument, ASTNode, …	rest

The "23 ahead" overstates our divergence — it counts the cherry-picks from the recent master/aot-pipeline reconciliation. The real patch set is small: two functional changes, plus release plumbing (version stamps, JitPack config, two commented-out ext/comp tests).

Upstream's last release is jsbml-1.6.1, dated 2022-01-10 — four and a half years ago. Their master is active (commits within the last two weeks) but sits unreleased at 1.7-SNAPSHOT. So "catching up" means tracking an unreleased branch, not taking a newer release. That is the central tension in everything below.

Our two patches, and what upstream has done about them

1. The annotation ClassCastException guard (#1461) — upstream has fixed this, better than we did.

0454dce6 (upstream PR #264, #282) guards the same cast. Theirs is a strict superset of ours:

if (lastElement instanceof Annotation) {          // ← our guard
    Object parent = annotation.getParent();
    if (parent instanceof SBase) {                // ← additional guard we do NOT have
        for (AnnotationReader annoReader : annotationParsers) { ... }
    } else {
        logger.error("...expected parent of Annotation to be an SBase but found " + ...);
    }
} else {
    logger.error("...expected top stack element to be an Annotation but found " + ...);
}

They also log the offending type instead of a bare message, and ship a test (SBMLReaderAnnotationCastTest). On upgrade we drop our patch entirely and gain a guard. Ours was written first; theirs is the one to keep.

2. The ParserManager concurrency fix (#1971) — upstream still does not have this.

getManager() on upstream master today:

public static ParserManager getManager() {     // still unsynchronized
  if (manager == null) { manager = new ParserManager(); }
  return manager;
}

with the same static, single-use ServiceLoader iterators at lines 48/53. This is the one patch a catch-up would not obtain, and therefore the one worth sending upstream.

Concrete changes to send upstream

Ordered by how ready each is.

1. Synchronize ParserManager.getManager() — ready now. One word, already written and tested in the fork (virtualcell/vcell-jsbml#3, #4). Draft issue text is on #1971 and has not been filed. Precedent is good: their #195 ("XMLNode.convertStringToXMLNode is not thread safe", the same static-but-stateful shape) was fixed within days.

2. Finish or revert the slf4j introduction. Upstream's core/pom.xml now declares slf4j-api 1.7.32, but exactly one file (AbstractSBase.java) imports slf4j while 82 still import org.apache.log4j. That looks like an unfinished migration rather than a decision, and it puts two logging frameworks in a library's dependency set. Worth asking what the intent is before we inherit it. (Not urgent for us: VCell already has slf4j-api 2.0.16 and the log4j-1.2-api bridge on the classpath, so both routes already work — though note the version skew, upstream declares 1.7.32.)

3. Cut a release. The most valuable thing upstream could do for every downstream consumer, and the thing that would let us drop the fork. Four and a half years of fixes are stranded in 1.7-SNAPSHOT. Worth asking about plans rather than assuming.

4. Java 7 source/target. jsbml-build.properties still sets 1.7, which no longer compiles under a modern JDK — building the fork locally needs -Djsbml.build.javac.source=8 -Djsbml.build.javac.target=8. Only reason this has not bitten us is that JitPack's default image is JDK 8 (see the risk below).

Plan for the fork

Do not do the 122-commit catch-up as a single step, and do not treat it as a goal in itself. SBML import is the path every BioModel import goes through; the payoff has to be identified, not assumed.

Proposed sequence:

• Step 0 — pin the build. Add a jitpack.yml with jdk: openjdk8. Today there is none, and the publish pipeline silently depends on JitPack's default image happening to be JDK 8. From the 1.6.1-VCELL-4 build log: Java version: 1.8.0_292 … Running: ./mvnw clean install -DskipTests. If that default moves forward, our tags stop building with Source option 7 is no longer supported, and we find out the next time we need to cut a release. Cheap, no behaviour change, do it first. Also worth noting from the same log: -DskipTests, so nothing in the publish path runs the fork's tests — which is why the VCell-side test added in #1974 is the one that actually guards us.

• Step 1 — file the ParserManager fix upstream (needs @jcschaff's go-ahead; draft is on #1971).

• Step 2 — evaluate the 68 core/src commits for anything we actually want. Candidates spotted, none yet assessed:

  • 2aa8efd4 prevent infinite recursion in ExpandFunctionDefinitionConverter
  • 076bf2b3 operator precedence brackets in FormulaCompiler (their #259)
  • b4e8a414 unary minus / NOT precedence in the ASTNode formula parser (their #249)
  • 4788407b ASTNode NPE; fe7bc4f1/92676005 derived-units NPEs; 4fc4380e IndexOutOfBoundsException in UnitsCompiler.lambda
  • 44412ad8 strict namespace URI checking for plugins (their #216) — behaviour change, could reject documents we currently accept
  • XStream 1.4.20 → 1.4.21 (XStream has a CVE history; worth checking whether the bump matters to us)

  Relevance check, so nobody over-reads that list: ASTNode is referenced directly in 15 vcell-core main-source files, but FormulaCompiler, UnitsCompiler and ExpandFunctionDefinitionConverter are referenced in none — they would only be reached indirectly. The precedence fixes sound alarming (wrong math is worse than a crash), but whether any of them changes a formula VCell actually produces is unverified. Verify before porting; port individually rather than merging the branch.

• Step 3 — only if step 2 finds something we need, do the full merge, on a branch, gated on the BMDB nightly suite rather than on the Fast group. That suite is the only thing that exercises SBML import at scale.

Risks worth stating plainly

• Tracking an unreleased branch. 1.7-SNAPSHOT has not been release-qualified by upstream. Taking it wholesale moves VCell's SBML import onto a dev branch.
• 44412ad8 strict namespace checking could turn models that import today into models that do not. The BMDB nightly is the instrument that would catch it.
• Pom conflicts are now expected and fine. Since the fork's master carries 1.6.1-VCELL version stamps where upstream has 1.7-SNAPSHOT, any upstream merge conflicts on the poms. Trivial and localized — and far better than the previous arrangement, where a branch nothing shipped from hid two fixes for seventeen months.

Definition of done

Not "fork deleted" — that needs an upstream release we do not control. The reachable goal is fork delta reduced to one patch (ParserManager), so that dropping the fork becomes a version bump on the day upstream cuts a release.