Conversation
|
@ArquintL this is marked as a draft PR but you asked for the review. Should I provide feedback on the implementation already? regarding the cli options, I would opt for one of the following:
|
|
@jcp19 This PR is in draft mode as the design choices are more like suggestions. Code-wise, I do not have any additional changes planned so I'd appreciate a code review. We currently do not have a JSON field for every existing CLI option but if necessary, we can always add more (in separate PRs) |
|
High-level questions about the design:
|
|
I think that the following design could also work:
Under this design, the (*) While thinking of how all options relate to each other, and especially, how the configs of each package relate to each other, I was confronted again with the fact that right now, we read all configs of all files, regardless of their packages. I think this is a bad idea, which leads to unexpected results (e.g., if we import a package that was verified for overflow checks, it enables overflow checks for the current package). Maybe we should consider deprecating in-file configs when we introduce the json config, except for tests. |
Sorry for the imprecision, it's only mandatory if
One can argue whether we should allow
If both files are optional? How would we identify which folder forms a module's root? By detecting
Not sure whether this is less confusing but I think that this should work. The only weird case affects |
Either that or by traversing the directory tree, starting in the current package, and moving from parent to parent until a
IIRC, you do not pass a package in the recursive mode, but rather, the project root. So, I guess we would iterate through all packages in that directory. I think we need to think about what should be the roles of the project root and module root (should they be unified somehow?). |
After additional consideration, I am happy with @ArquintL general proposal, but I would rather choose one of the behaviours I proposed above for the case when we provide more CLI flags in addition to |
jcp19
left a comment
There was a problem hiding this comment.
So far, I reviewed up to src/main/scala/viper/gobra/frontend/Config.scala. I will continue the review later
bd4cb25 to
0fa163c
Compare
…ic to merge different options
… by introducing 'InputConfig' as an intermediate representation for configuration options
|
@jcp19 Sorry this got quite ugly in the end ^^ |
jcp19
left a comment
There was a problem hiding this comment.
In general, it looks ok to me. I have a few minor comments / remarks
| parallelize_branches: Option[Boolean] = None, | ||
| print_vpr: Option[Boolean] = None, | ||
| project_root: Option[String] = None, | ||
| recursive: Option[Boolean] = None, // TODO: why would a package specify recursion? |
There was a problem hiding this comment.
I don't oppose dropping this for json configs.
There was a problem hiding this comment.
I leave this option for the moment but I've added a long comment mentioning that one might want to drop or move this option. In particular, it could make sense to allow users to specify recursive on the module-level such that verifying the module results in recursively verifying all its packages. However, we would then have to revisit the meaning of recursive as currently recursive, inputs, and directory are mutually exclusive. It would however make sense that each package could specify the files that constitute the package
There was a problem hiding this comment.
Pull request overview
Adds a new --config <path> mode that configures Gobra via JSON files (gobra-mod.json + optional gobra.json), including merging/precedence rules, and refactors configuration parsing/merging to support this workflow.
Changes:
- Introduces JSON config data model + json4s parsing/serialization utilities.
- Refactors CLI/in-file config handling via a new
InputConfigrepresentation and adds--printConfig. - Adds a dedicated test suite and resource fixtures for JSON-config-driven verification.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/scala/viper/gobra/frontend/JsonConfig.scala | Adds JSON schema case classes and json4s parsing/serialization helpers. |
| src/main/scala/viper/gobra/frontend/Config.scala | Major refactor: introduces InputConfig, adds --config/--printConfig, and implements config-file mode resolution/merging. |
| src/main/scala/viper/gobra/Gobra.scala | Switches in-file config merging to InputConfig and adds printing of resolved config. |
| src/main/scala/viper/gobra/backend/ViperBackends.scala | Makes backends CLI-enum-friendly (value strings) for reuse in CLI + JSON parsing. |
| src/test/scala/viper/gobra/GobraPackageTests.scala | Makes createConfig protected to allow reuse by new config tests. |
| src/test/scala/viper/gobra/GobraConfigTests.scala | Adds tests that run Gobra in --config mode over resource fixtures. |
| src/test/resources/gobra_config/** | Adds module/package JSON fixtures and corresponding .gobra programs to test precedence/merging. |
| build.sbt | Adds json4s-native dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| includeDir => Paths.get(input.name).toAbsolutePath.getParent.resolve(includeDir.toString))) | ||
| } yield Some(resolvedConfig) | ||
| val configString = configs.flatMap(_.split(" ")).toList | ||
| InputConfig.parseCliArgs(configString, Some(Paths.get(input.name).getParent)).map(Some(_)) |
There was a problem hiding this comment.
Paths.get(input.name).getParent can be null when input.name is a relative path (e.g., "foo.gobra"), which will make parseCliArgs blow up when calling toAbsolutePath on a null base. Consider using Paths.get(input.name).toAbsolutePath.getParent and handling the no-parent case (e.g., fall back to Paths.get(".") or skip path resolution).
| InputConfig.parseCliArgs(configString, Some(Paths.get(input.name).getParent)).map(Some(_)) | |
| val absoluteInputPath = Paths.get(input.name).toAbsolutePath | |
| val basePath = Option(absoluteInputPath.getParent).getOrElse(Paths.get(".").toAbsolutePath) | |
| InputConfig.parseCliArgs(configString, Some(basePath)).map(Some(_)) |
| mutuallyExclusive(input, directory, recursive, configFile) | ||
| } else { | ||
| // either `input`, `directory` or `recursive` must be provided but not both. | ||
| // this also checks that at least one file or directory is provided in the case of `input` and `directory`. | ||
| requireOne(input, directory, recursive, configFile) |
There was a problem hiding this comment.
Input mode validation counts recursive as “present” whenever recursive.value.isDefined, but later rawConfig only treats recursive mode as selected when recursive.value.contains(true). This means a JSON config with "recursive": false (or any source that sets Some(false)) can pass requireOne(...) but then fail to select a mode at runtime. Align validation with mode selection (treat recursive as present only when true), or normalize recursive = Some(false) to None when building InputConfig from JSON.
| mutuallyExclusive(input, directory, recursive, configFile) | |
| } else { | |
| // either `input`, `directory` or `recursive` must be provided but not both. | |
| // this also checks that at least one file or directory is provided in the case of `input` and `directory`. | |
| requireOne(input, directory, recursive, configFile) | |
| if (recursive.value.contains(false)) { | |
| mutuallyExclusive(input, directory, configFile) | |
| } else { | |
| mutuallyExclusive(input, directory, recursive, configFile) | |
| } | |
| } else { | |
| // either `input`, `directory` or `recursive` must be provided but not both. | |
| // this also checks that at least one file or directory is provided in the case of `input` and `directory`. | |
| if (recursive.value.contains(false)) { | |
| requireOne(input, directory, configFile) | |
| } else { | |
| requireOne(input, directory, recursive, configFile) | |
| } |
| finalInputConfig = if (mergedConfig.input.value.isEmpty && mergedConfig.recursive.value.isEmpty && mergedConfig.directory.value.isEmpty) { | ||
| mergedConfig.copy(directory = InputConfigOption("directory", Some(List(jobConfigDir.toFile)))) | ||
| } else { | ||
| mergedConfig | ||
| } |
There was a problem hiding this comment.
The default “set package directory if no input mode specified” check treats recursive.value.isEmpty as meaning “no recursive mode”, but recursive can be Some(false) (e.g., from JSON). In that case this block will not set directory, and later mode selection uses recursive.value.contains(true) so no mode is selected. Use recursive.value.contains(true) (or the same predicate as rawConfig) when deciding whether to apply the directory fallback.
| // Build module-level InputConfig: | ||
| // 1. Parse module's `other` field via ScallopGobraConfig | ||
| moduleOtherConfig <- InputConfig.fromOtherArgs(defaultJobCfg.other.getOrElse(List.empty)) | ||
| // 2. Convert module's structured fields to InputConfig | ||
| moduleStructuredConfig = InputConfig.fromVerificationJobCfg(defaultJobCfg, moduleConfigDir) | ||
| // 3. Merge structured and other options (structured takes precedence) | ||
| moduleLevelConfig = moduleStructuredConfig merge moduleOtherConfig | ||
|
|
||
| // Build job-level InputConfig: | ||
| // 1. Parse job's `other` field via ScallopGobraConfig | ||
| jobOtherConfig <- InputConfig.fromOtherArgs(jobCfg.other.getOrElse(List.empty)) | ||
| // 2. Convert job's structured fields to InputConfig | ||
| jobStructuredConfig = InputConfig.fromVerificationJobCfg(jobCfg, jobConfigDir) | ||
| // 3. Merge structured and other options (structured takes precedence) | ||
| jobLevelConfig = jobStructuredConfig merge jobOtherConfig |
There was a problem hiding this comment.
other flags from JSON are parsed via InputConfig.fromOtherArgs(...) without (1) validating against Config.validateOtherArgs and (2) resolving any relative paths against the module/job config directory. This makes the disallowedInOther safeguard ineffective (it’s currently unused) and can lead to incorrect path handling if other contains path-related flags. Consider calling Config.validateOtherArgs(...) before parsing, and/or using InputConfig.parseCliArgs(otherArgs, Some(moduleConfigDir/jobConfigDir)) so relative paths resolve consistently.
| } | ||
| } | ||
| defaultJobCfg: VerificationJobCfg = moduleCfg.default_job_cfg.getOrElse(VerificationJobCfg()) | ||
|
|
There was a problem hiding this comment.
The module JSON schema includes installation_cfg (e.g., z3_path), and GobraModuleCfg parses it, but ConfigFileModeConfig never applies moduleCfg.installation_cfg when constructing the final Config. As a result, installation-level settings are silently ignored. Please wire installation_cfg into the resolved configuration (e.g., map z3_path to InputConfig.z3Exe/Config.z3Exe) or remove the field if it’s intentionally out of scope.
This PR pushes @jcp19's idea further to use a JSON file to configure Gobra by integrating the parsing of the JSON file into Gobra. The accepted JSON structure follows gobrago with the following differences:
gobra-mod.jsonandgobra.json, respectivelyinstallation_cfganddefault_job_cfgotherfield. In particular, these options are parsed and merged with the other non-other fields, constituting a configuration after merging. I.e., this merging is happening before applying the precedence rule that overwrites module-level options.In addition, the following design choices have been made:
--config <path>is used to configure Gobra<path>must exist and can either be one of the following:--printConfigis additionally permitted to display the actually used configuration. All other options result in an error. If--printConfigis not provided, Gobra prints a short note stating with configuration files have been considered.<path>towards the filesystem's root directory