Skip to content

Changes pertaining to bootstrap builds and enforciong szBuildVersion.json is correct at build time#312

Open
barrycaceres wants to merge 1 commit intomainfrom
caceres-version-check-3
Open

Changes pertaining to bootstrap builds and enforciong szBuildVersion.json is correct at build time#312
barrycaceres wants to merge 1 commit intomainfrom
caceres-version-check-3

Conversation

@barrycaceres
Copy link
Copy Markdown
Contributor

  • Fixes handling of SENZING_PATH=.../build/dist
  • Enforces szBuildVersion.json correctness during the build

@barrycaceres barrycaceres requested a review from a team as a code owner April 12, 2026 06:58
@barrycaceres barrycaceres self-assigned this Apr 12, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 12, 2026

Code Coverage

Overall Project 92.07% -0.49% 🟢
Files changed 63.19% 🔴

File Coverage
InstallUtilities.java 81.95% -1.42% 🟢
GenerateTestJVMScript.java 0% -6.59% 🔴

@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

Code Review

Files reviewed: GenerateTestJVMScript.java, InstallUtilities.java
Purpose: Add dist-inside-build directory layout support for dev builds; move RUNTIME_SENZING_VERSION earlier to fix initialization order; add build-vs-runtime version mismatch validation.


Code Quality

Style guide compliance

Several new code blocks place the opening brace on its own line, inconsistent with the K&R style used throughout the rest of both files:

  • GenerateTestJVMScript.java:59-60&& senzingPath.trim().length() > 0) \n {
  • GenerateTestJVMScript.java:136-138 — multi-line if with brace on new line
  • InstallUtilities.java:283-284 — same pattern in parseVersionFromJson
  • InstallUtilities.java:527-529 — same in findSenzingPathFromLib

No commented-out code

Meaningful variable names ✅ — erDir, devBuild, runtimeVersion, etc. are clear.

DRY principle

The dist-inside-build detection predicate is duplicated three times across two files with no shared helper:

  • GenerateTestJVMScript.java:63-64
  • GenerateTestJVMScript.java:136-137
  • InstallUtilities.java:527-529

Consider extracting isDevBuildDir(File dir) into a shared utility method, or at minimum a private helper within each class.

Defects / Logic errors

  1. Potential NullPointerExceptionGenerateTestJVMScript.java:64

    devBuild = ((baseDir.getName().equalsIgnoreCase("dist"))
        && baseDir.getParentFile().getName().equalsIgnoreCase("build"));

    File.getParentFile() returns null when baseDir is a filesystem root. The call to .getName() on the null result will throw NPE. Needs a null guard:

    File parentFile = baseDir.getParentFile();
    devBuild = parentFile != null
        && baseDir.getName().equalsIgnoreCase("dist")
        && parentFile.getName().equalsIgnoreCase("build");
  2. Potential NullPointerExceptionGenerateTestJVMScript.java:137
    Same issue. parentDir (line 134) is senzingDir.getParentFile(), which can be null. The guard senzingDir.exists() && senzingDir.isDirectory() does not protect against a null parentDir. Line 137 calls parentDir.getName() unconditionally.

  3. Misleading comment — InstallUtilities.java:543-544

    // unexpected location for "lib" directory -- it does not appear to
    // be located within the Senzing installation or build
    try {
        return senzingDir.getCanonicalFile();   // <-- SUCCESS path

    At this point in the code we have confirmed erDirName is "er" (the expected name). This comment describes a failure condition but immediately precedes the success return. It is a leftover from the pre-refactor code and is now actively misleading. It should be updated to something like:

    // standard Senzing installation: return parent of "er" directory
  4. Trailing whitespaceInstallUtilities.java has trailing spaces on blank lines within the moved parseVersionFromJson body (lines between eatWhiteSpace calls) and after the closing } of the early-return null check block. These will be flagged by most editors/linters and are inconsistent with the project's checkstyle configuration.

  5. Alignment spacesGenerateTestJVMScript.java:61 uses extra spaces to vertically align =:

    File baseDir    = new File(senzingPath);

    The surrounding code in the same method does not use this style.


Testing

Unit tests for new logic

  • The devBuild detection (the dist-inside-build predicate) is new branching logic with no visible test coverage added for this PR.
  • The version mismatch IllegalStateException thrown by getInstallBuildVersion() has no visible unit test covering the mismatch case.
  • parseVersionFromJson was already in the codebase; if existing tests cover it, that is fine — but the new mismatch validation path is untested.

Edge cases ❌ — The null-parent edge cases noted above have no corresponding tests.


Documentation

Javadoc ✅ — The parseVersionFromJson Javadoc moved with the method and is intact.

Inline comments for complex logic ✅ — The new findSenzingPathFromLib logic has adequate comments (aside from the misleading one noted above).

CHANGELOG.md ❌ — No CHANGELOG.md update is visible in the diff for what is a functional behavior change (dev-build path support, build-vs-runtime version validation).


Security

No hardcoded credentials

Input validation ✅ — parseVersionFromJson defensively checks bounds before every character access.

Error handling ✅ — The catch (Exception | UnsatisfiedLinkError e) in the RUNTIME_SENZING_VERSION static initializer correctly handles the case where native libraries are unavailable at class-load time.

No sensitive data in logs

No license files


Summary

Severity Issue
Bug NPE on getParentFile().getName()GenerateTestJVMScript.java:64 and :137
Bug Misleading success-path comment in findSenzingPathFromLibInstallUtilities.java:543
Quality DRY violation — dist/build detection repeated 3× across 2 files
Quality Brace style inconsistency throughout the diff
Quality Trailing whitespace and alignment spaces
Testing No tests for new devBuild detection or version-mismatch exception
Docs No CHANGELOG.md update

The refactoring of RUNTIME_SENZING_VERSION to appear before INSTALL_BUILD_VERSION (which depends on it via getInstallBuildVersion()) is correct and necessary — that ordering fix is sound.

Automated code review analyzing defects and coding standards

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