Separate cli and gui #415
Separate cli and gui #415
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## integration #415 +/- ##
==================================================
+ Coverage 0.00% 18.33% +18.33%
- Complexity 0 48 +48
==================================================
Files 2 8 +6
Lines 8 709 +701
Branches 0 107 +107
==================================================
+ Hits 0 130 +130
- Misses 8 562 +554
- Partials 0 17 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
how can i install veraPDF with the help of winget? |
|
@hueldoeu the veraPDF installer is based on izPack, which is a generic Java-based installer. It does into include WinGet manifest. There is however a way to install veraPDF from command line as described here: https://docs.verapdf.org/install/#automated-installation BTW, your question doesn't seem to be related to this particular PR. If this is indeed the case, it would be better to post an issue in veraPDF-library repository. |
carlwilson
left a comment
carlwilson
left a comment
There was a problem hiding this comment.
So I like this all in all. I do have some comments as to the final build products and installer behaviour, but they're optimisations.
I'm not in love with the two fat jars approach, I know that we mentioned it in the call. This means a full install's bin directory looks like:
total 18M
drwxr-xr-x 2 cfw cfw 4.0K Jun 13 10:37 .
drwxr-xr-x 7 cfw cfw 4.0K Jun 13 10:37 ..
-rwxr-xr-x 1 cfw cfw 8.7M Jun 13 10:11 cli-1.29.0-SNAPSHOT.jar
-rwxr-xr-x 1 cfw cfw 8.9M Jun 13 10:11 gui-1.29.0-SNAPSHOT.jarNearly twice as big as before. I think we need to break the jars/modules apart for deployment. I'd also be interested to see what gets deployed to Maven, but we can review then.
My only other observation is regarding installation and clean-up. If you install on top of an old installation and change between CLI/GUI the jars in the bin directory get "cleaned" by the installer, i.e. installing the CLI will remove any existing GUI jar and vice-versa. The installer doesn't tidy up the corresponding script files so you are left with a "broken" verapdf-gui script:
$ ./verapdf-gui
Error: Could not find or load main class org.verapdf.apps.GreenfieldGuiWrapper
Caused by: java.lang.ClassNotFoundException: org.verapdf.apps.GreenfieldGuiWrapperWe should remove the unused script as well if possible, this could perhaps be done before merging.
📝 WalkthroughWalkthroughReplaces the greenfield-apps module with two new modules (apps, cli), updates build and installer configurations to reference gui and cli packages, moves constants from GUI to Apps, and adjusts CI/reporting paths and resource lookup strings accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (1)gui/src/main/java/org/verapdf/gui/CheckerPanel.java (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/src/main/java/org/verapdf/apps/AppsConstants.java (1)
1-8: Migration from GUIConstants is incomplete and results in code duplication.The
AppsConstantsclass provides newZIPconstants, butGUIConstantsstill defines these same constants and they remain actively used inCheckerPanel.java(lines 118, 269, 287, 785, 892). This creates duplication without a clear migration strategy. Consider either completing the migration by updating all GUI module usages toAppsConstantsand deprecating theGUIConstantsversions, or clearly document the intended separation of concerns between modules.apps/pom.xml (1)
1-23: License header inconsistency.The license header references "VeraPDF Library GUI" but this is the new
appsmodule. Consider updating the header to reflect the correct module name.cli/pom.xml (2)
1-23: License header inconsistency.The license header references "VeraPDF Library GUI" but this is the
climodule. Consider updating the header to reflect the correct module name.
125-133: Add test scope to test dependencies.
junitandequalsverifierare testing libraries and should have<scope>test</scope>to prevent them from being included in the runtime classpath and the jar-with-dependencies.🔎 Proposed fix
<dependency> <groupId>junit</groupId> <artifactId>junit</artifactId> + <scope>test</scope> </dependency> <dependency> <groupId>nl.jqno.equalsverifier</groupId> <artifactId>equalsverifier</artifactId> + <scope>test</scope> </dependency>installer/src/main/izpack/install.xml (1)
108-136: CLI pack structure is correct; consider deduplicating the plugins README.The CLI pack mirrors the GUI pack structure appropriately. However, both packs include
plugins/README.txt(line 88 and line 117). While IzPack handles duplicate file installations gracefully, consider extracting the plugins directory setup to a separate required pack or keeping it only in one location to avoid redundancy.Additionally, since both GUI and CLI packs are now
required="no", users could theoretically complete installation without either component. If this is unintended, consider adding pack dependencies or making at least one pack required.🔎 Optional: Extract plugins setup to avoid duplication
You could create a minimal required "Core" pack that includes shared resources:
<pack name="veraPDF Core" required="yes"> <description>Core veraPDF files.</description> <file targetdir="$INSTALL_PATH/plugins" src="plugins/README.txt"/> </pack>Then remove the
plugins/README.txtentries from both GUI and CLI packs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cli/src/test/resources/tmpFilesTest.pdfis excluded by!**/*.pdfcli/src/test/resources/veraPDFtest-pass-a.pdfis excluded by!**/*.pdf
📒 Files selected for processing (38)
.github/workflows/test-pr.ymlREADME.mdapps/pom.xmlapps/src/main/java/org/verapdf/apps/Applications.javaapps/src/main/java/org/verapdf/apps/AppsConstants.javaapps/src/main/java/org/verapdf/apps/SoftwareUpdater.javaapps/src/main/java/org/verapdf/apps/SoftwareUpdaterImpl.javaapps/src/main/java/org/verapdf/apps/utils/ApplicationUtils.javaapps/src/main/resources/org/verapdf/release/apps.propertiesauto-install-tmp.xmlcli/pom.xmlcli/src/main/java/org/verapdf/apps/GreenfieldCliWrapper.javacli/src/main/java/org/verapdf/cli/CliConstants.javacli/src/main/java/org/verapdf/cli/FormatterHelper.javacli/src/main/java/org/verapdf/cli/VeraPdfCli.javacli/src/main/java/org/verapdf/cli/VeraPdfCliProcessor.javacli/src/main/java/org/verapdf/cli/commands/VeraCliArgParser.javacli/src/main/java/org/verapdf/cli/multithread/BaseCliRunner.javacli/src/main/java/org/verapdf/cli/multithread/MultiThreadProcessor.javacli/src/test/java/org/verapdf/apps/test/MultithreadingTest.javacli/src/test/java/org/verapdf/apps/test/TempFileClosingTest.javacli/src/test/java/org/verapdf/cli/VeraPdfCliProcessorTest.javacli/src/test/java/org/verapdf/cli/commands/VerCliFlavourTest.javacli/src/test/java/org/verapdf/cli/commands/VeraCliArgParserTest.javacli/src/test/java/org/verapdf/cli/commands/VeraCliFormatTest.javacli/src/test/java/org/verapdf/cli/commands/VeraCliGetPathsTest.javacli/src/test/java/org/verapdf/cli/commands/VeraCliProfileOptionTest.javacli/src/test/java/org/verapdf/cli/commands/VeraCliTasksTest.javacli/src/test/java/org/verapdf/pdfa/validation/validators/test/CallableValidatorForTest.javadocker-install.xmlgreenfield-apps/.gitignoregreenfield-apps/pom.xmlgui/pom.xmlgui/src/main/java/org/verapdf/apps/GreenfieldGuiWrapper.javagui/src/main/java/org/verapdf/gui/PDFValidationApplication.javainstaller/pom.xmlinstaller/src/main/izpack/install.xmlpom.xml
💤 Files with no reviewable changes (3)
- greenfield-apps/pom.xml
- cli/src/test/java/org/verapdf/apps/test/MultithreadingTest.java
- greenfield-apps/.gitignore
🧰 Additional context used
🧬 Code graph analysis (2)
apps/src/main/java/org/verapdf/apps/utils/ApplicationUtils.java (1)
apps/src/main/java/org/verapdf/apps/AppsConstants.java (1)
AppsConstants(3-8)
cli/src/main/java/org/verapdf/cli/commands/VeraCliArgParser.java (1)
apps/src/main/java/org/verapdf/apps/AppsConstants.java (1)
AppsConstants(3-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Checkout and Build (25)
- GitHub Check: Checkout and Build (11)
- GitHub Check: Checkout and Build (21)
- GitHub Check: Checkout and Build (17)
- GitHub Check: Checkout and Build (8)
🔇 Additional comments (21)
README.md (1)
114-117: LGTM!The documentation correctly reflects the new module structure, updating references from
greenfield-appstoguifor testing the build.cli/src/main/java/org/verapdf/cli/FormatterHelper.java (1)
59-135: LGTM!The explicit qualification of
DefaultUsageFormatter.s(indentCount)improves code clarity by making the method source explicit. All usages have been consistently updated throughout the class..github/workflows/test-pr.yml (1)
49-90: LGTM!The workflow correctly updates all references from
greenfield-appstoclifor coverage reporting, while preserving GUI-related paths. The changes are consistent across artifact upload, download, and reporting steps.gui/src/main/java/org/verapdf/gui/PDFValidationApplication.java (2)
351-351: Verify consistency with Applications.getAppDetails().The update check now uses
ReleaseDetails.byId("apps"), which should match the ID returned byApplications.getAppDetails(). This is correct based on the changes inapps/src/main/java/org/verapdf/apps/Applications.java.
432-432: Theapps.propertiesresource file exists at the expected location.The code change updating the path from
"app."to"apps."is properly supported. Theapps.propertiesfile is present atapps/src/main/resources/org/verapdf/release/apps.properties.cli/src/main/java/org/verapdf/cli/VeraPdfCli.java (1)
80-80: LGTM!The resource path update from
"app."to"apps."aligns with the broader refactoring to separate CLI and GUI modules. This change is consistent with the corresponding update inPDFValidationApplication.java.apps/src/main/java/org/verapdf/apps/Applications.java (1)
91-91: The apps.properties resource file exists with valid release metadata.The
apps.propertiesfile is present atapps/src/main/resources/org/verapdf/release/apps.propertiesand contains the required release metadata properties that support the ReleaseDetails.byId("apps") call.cli/src/main/java/org/verapdf/cli/commands/VeraCliArgParser.java (2)
25-25: LGTM!The import correctly references the new
AppsConstantsclass from theappsmodule, aligning with the module restructuring in this PR.
728-731: LGTM!The constant references are correctly updated from
GUIConstants.ZIPtoAppsConstants.ZIP. The logic for checking ZIP file extensions in password handling and metadata fixing remains unchanged.Also applies to: 735-746
apps/src/main/java/org/verapdf/apps/utils/ApplicationUtils.java (3)
33-33: LGTM!Import correctly added for the new
AppsConstantsclass.
87-88: LGTM!Extension checks correctly updated to use
AppsConstants.PDFandAppsConstants.ZIP. The logic appropriately allows both PDF and ZIP files at the top-level filtering.
105-105: LGTM!Extension check correctly updated to use
AppsConstants.PDF. The intentional difference fromfilterPdfFiles(no ZIP check in recursive directory filtering) is preserved.apps/src/main/resources/org/verapdf/release/apps.properties (1)
1-4: LGTM!Properties file correctly configured with Maven placeholders for resource filtering. The file path (
org/verapdf/release/apps.properties) aligns with the updated resource loading paths inVeraPdfCliandPDFValidationApplication.auto-install-tmp.xml (1)
29-34: LGTM!Pack configuration correctly updated to reflect the new module structure. The replacement of the old "Mac and *nix Scripts" and "Validation model" packs with the new "veraPDF CLI" pack is appropriate, and having both GUI and CLI selected by default for the automated install template provides a complete installation.
docker-install.xml (1)
29-34: LGTM!Docker installation configuration correctly defaults to CLI-only (GUI deselected), which is appropriate for headless container environments. The pack structure aligns with the new module organization.
installer/pom.xml (2)
47-48: LGTM!New properties correctly define separate package identifiers for GUI and CLI modules, replacing the previous single
verapdf.apps.packageproperty.
54-65: LGTM!Dependencies correctly updated to reference the new
cliandguimodules with version alignment via${project.version}. This ensures the installer builds with the correct module artifacts.apps/pom.xml (1)
65-68: Remove or add test scope to align with root pom.The
equalsverifierdependency is redeclared in apps/pom.xml without a scope, overriding the root pom's<scope>test</scope>declaration and making it compile-scoped. Sinceequalsverifieris not used in the apps module (or cli/gui), either add<scope>test</scope>to align with the parent declaration or remove this dependency entirely.installer/src/main/izpack/install.xml (1)
79-107: GUI pack configuration looks well-structured.The changes properly separate GUI into an optional pack with dedicated launcher scripts (
verapdf-gui/verapdf-gui.bat). The OS-specific handling withparsableandexecutabledirectives is correct.gui/pom.xml (2)
39-81: Build plugins configuration is correct. The assembly plugin is properly configured to create a fat JAR with the specified main classorg.verapdf.apps.GreenfieldGuiWrapper(which exists ingui/src/main/java/org/verapdf/apps/), andappendAssemblyId=falseproduces a clean artifact name. Themaven-compiler-plugindeclaration correctly inherits its configuration from the parent POM, so no additional configuration is needed here.
84-117: JAXB dependency versions are properly managed in parent POM. Thejavax.xml.bind:jaxb-api,com.sun.xml.bind:jaxb-core, andcom.sun.xml.bind:jaxb-impldependencies without version tags correctly inherit versions from the root pom.xml's<dependencyManagement>section (2.3.1, 2.3.0.1, and 2.3.2 respectively).The
coredependency is necessary and not redundant. The gui module directly imports fromorg.verapdf.corein multiple source files (PDFValidationApplication.java, ValidateWorker.java, PolicyPanel.java, and CheckerPanel.java) for utilities like LogsFileHandler, VeraPDFException, and FileUtils. Therefore, the explicitcoredependency declaration is appropriate alongsidevalidation-model.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.