Skip to content

Resolve vicario from Maven Central instead of a bundled jar - #46

Open
jl-0 wants to merge 1 commit into
developfrom
feature/vicario-maven-central
Open

jl-0 wants to merge 1 commit into
developfrom
feature/vicario-maven-central

Conversation

@jl-0

@jl-0 jl-0 commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Purpose

TIG shipped a committed, prebuilt vicario.jar: an unreleased 2.7.0b0-SNAPSHOT fat jar, 17 MB, whose docs said VicarIO source was "not yet public". VicarIO is now open source at NASA-AMMOS/vicario and published to Maven Central. This PR builds the image from the published release instead.

Changes

  • Removed terrain-intelligence-generator/docker/vicario.jar.
  • Added docker/vicario/pom.xml, which pins gov.nasa.jpl.ammos.ids:vicario:2.7.2.
  • Dockerfile:
    • A new vicario stage (maven:3.9.9-eclipse-temurin-17) copies vicario and its runtime dependencies to /usr/local/lib/vicario.
    • The wrapper runs jpl.mipl.io.jConvertIIO on that classpath. Its CLI behavior is the same as before.
  • Java 11 → 17 in the runtime image, because vicario 2.7.2's pds4-jparser dependency is compiled for Java 17.
  • -Dcom.sun.media.jai.disableMediaLib=true is now passed by the wrapper. Comparing bytecode, this was the only functional change in the bundled "modified" jar: its main() set the property. The other two differing classes are a Swing viewer and a CSV error message.
  • Docs (VICARIO.md, docs/reference/vicario.md, docs/architecture/components.md, the image README) and build-opensource-image.sh no longer refer to vicario.jar.

Every dependency resolves from public repositories (Central, OSGeo, imageio-ext), so CI needs no new secrets. CI's build context is terrain-intelligence-generator/docker, which contains the new POM.

Testing

  • Old vs new output: I converted 3 Mars 2020 products (NCAM FDR, ZCAM FDR, NCAM XYZ) to PNG, JPEG and TIFF with the old jar on Java 11 and with vicario 2.7.2 on Java 17. All 9 outputs are byte-identical. Both ran on oraclelinux:8 with the same oform=byte rescale=true arguments the wrapper uses.
  • Image build: build-opensource-image.sh succeeds locally (linux/amd64).
  • test-docker-image.sh: wrappers, gen, list, copy, stretch, vicario PNG and JPEG, docker exec, MARS commands and MPI all pass.
    • The "files exist on host" check failed on my Mac. It also flags outputs from copy and stretch, which don't use vicario, so it looks like a Docker Desktop bind-mount issue on this machine.
    • A direct check is fine: running gen + vicario (PNG, TIFF) into a bind-mounted directory under $HOME produced valid files on the host.
    • I did not run the same script against the current image for comparison. CI runs this test on Linux.

Devin Review

VicarIO is now open source (NASA-AMMOS/vicario) and published to Maven
Central as gov.nasa.jpl.ammos.ids:vicario. Replace the committed 17 MB
vicario.jar (an unreleased 2.7.0b0-SNAPSHOT fat jar) with a Dockerfile
stage that resolves vicario 2.7.2 and its runtime dependencies from the
version pinned in docker/vicario/pom.xml.

- The wrapper runs jpl.mipl.io.jConvertIIO on /usr/local/lib/vicario/*.
- The bundled jar's only functional change was setting
  com.sun.media.jai.disableMediaLib=true in main(); the wrapper now passes
  it as a -D option.
- The runtime moves from Java 11 to Java 17: vicario 2.7.2's pds4-jparser
  dependency is compiled for Java 17.
- Docs and build-opensource-image.sh no longer refer to vicario.jar.
@sonarqubecloud

sonarqubecloud Bot commented Oct 8, 2026

Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

Project Requirements: PASS

PR #46 replaces the bundled vicario.jar with vicario resolved from Maven Central (pinned in docker/vicario/pom.xml) and moves the image to Java 17. Documentation parity: pass, because all relevant vicario, image and architecture docs were updated and the stale 'not yet public' text was removed. Broad deployability: pass, because a public open-source artifact replaces an opaque binary; the only friction is that builds now need Maven Central/OSGeo access. Test coverage: concern, because existing CI image tests cover 2-argument PNG/JPEG conversion, but the changed keyword pass-through branch, TIFF output, the Java version, the pinned vicario version and error handling are untested.

Requirement Verdict Notes
1. Documentation parity Pass This change is user-facing. It changes image contents: the bundled vicario.jar is replaced by vicario 2.7.2 resolved from Maven Central into /usr/local/lib/vicario/, Java 11 becomes Java 17, the Dockerfile gains a new build stage, and the build script no longer requires the jar. The PR updates every matching doc: docs/reference/vicario.md, docker/VICARIO.md, terrain-intelligence-generator/README.md (file layout) and docs/architecture/components.md (Java 17 and Maven Central). The stale 'source not yet public' / 'Build from Source' text is removed rather than only added to. A repo-wide grep finds no remaining user-facing references to vicario.jar, /usr/local/bin/vicario.jar or Java 11. User-facing invocation (positional vs keyword, exit codes) is unchanged, so README.md, QUICKSTART.md and getting-started.md need no edits. Two internal code comments still mention vicario.jar (tig-cli/src/tig_cli/container.py:572 and tig-cli/tests/test_container.py:1499). They are not user docs, but should be reworded at some point.
2. Broad deployability Pass This change makes TIG more portable. It removes an opaque 17 MB binary whose source was described as JPL-only and replaces it with a public, Apache-licensed artifact (gov.nasa.jpl.ammos.ids:vicario) from Maven Central, built in a public maven:3.9.9-eclipse-temurin-17 stage. Anyone can now rebuild or upgrade vicario by changing version.vicario in the POM. Nothing ties it to one site, one runtime or one platform: Java and the classpath wrapper are architecture-neutral, the VISOR and fullfeatured images inherit it from the base image, and nothing is mission-specific. One small source of friction: image builds now also need network access to Maven Central and to the OSGeo/imageio-ext repositories that vicario's POM declares. Air-gapped or proxied builders would need a Maven mirror. The build already downloads from GitHub releases, so this is consistent with existing practice and not a blocker; an optional build arg for a Maven settings.xml or mirror would ease it. I could not confirm the 2.7.2 artifact exists because Maven Central rate-limited my request (HTTP 429), so that check is left to CI.
3. Test coverage Concern This is changed packaging and invocation behavior, not new capability. The existing image tests already cover the main path: Tests 9, 10 and 12 in terrain-intelligence-generator/test-docker-image.sh and the matching steps in build-publish-terrain-intelligence-generator.yml run 2-argument vicario conversions to PNG and JPEG in the rebuilt image. That workflow's paths filter (terrain-intelligence-generator/docker/**) includes the new pom.xml and the Dockerfile, so CI will exercise the new Maven stage and the -cp jConvertIIO wrapper. No new tests were needed for the removed jar check in build-opensource-image.sh. However, some changed code paths are not exercised, so this is a concern rather than a pass.

3. Test coverage

  • terrain-intelligence-generator/docker/Dockerfile:224: The wrapper's pass-through branch (3 or more arguments, keyword form) was changed from 'java -jar' to 'java -cp ... jpl.mipl.io.jConvertIIO "$@"', but no test calls vicario with 3 or more arguments. A wrong main class or missing classpath entry would only break keyword-form users, and nothing would catch it. Fix: Add a test to test-docker-image.sh and the build-publish workflow that runs 'vicario inp=/workspace/test.vic out=/workspace/kw.png format=png' and checks that kw.png exists and is a valid PNG.
  • terrain-intelligence-generator/test-docker-image.sh:177: The docs and workflow summary list TIFF as a supported format, but only PNG and JPEG conversion are tested. The Java 17 move and the change from the shaded jar to a resolved classpath could drop a TIFF/imageio-ext plugin without any test failing. The tests also only check that the output file exists, not that it is a valid image. Fix: Add a VICAR-to-TIFF conversion test (vicario test.vic test.tif). Check the output's file type/magic bytes (e.g. with 'file') for PNG, JPEG and TIFF, not just that the file exists.
  • terrain-intelligence-generator/test-docker-image.sh: Nothing checks that the image contains Java 17 and the pinned vicario version, and there is no off-nominal check of the wrapper's error handling (bad input file → no output and a clear stderr message) under the new classpath and -Dcom.sun.media.jai.disableMediaLib=true setup. Fix: Add assertions that 'java -version' reports 17 and that /usr/local/lib/vicario/vicario-2.7.2.jar exists (or read the version from the POM). Add a test that runs vicario on a missing or malformed .vic file and asserts that no output file is created and stderr is not empty.
Automated pre-check hints
  • terrain-intelligence-generator/ changed but no test scripts changed (requirement 3).

Evaluated at 5052f33 · Devin session · workflow run

Criteria: .github/project-requirements/criteria.md. A fail blocks the merge; a maintainer can waive it by adding the requirements-waived label.

This branch has not been deployed

No deployments
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.

1 participant