Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

temporary commandlinesplit alex's branch. For github diff and better reading for team #648

Draft
wants to merge 59 commits into
base: main
Choose a base branch
from

Conversation

Taepper
Copy link
Collaborator

@Taepper Taepper commented Nov 20, 2024

Summary

PR Checklist

  • All necessary documentation has been adapted or there is an issue to do so.
  • The implemented feature is covered by an appropriate test.

@Taepper Taepper changed the title temporary commandlinesplit alex's branch. For github diff and better reading temporary commandlinesplit alex's branch. For github diff and better reading for team Nov 20, 2024
For now it's better to continue to use the existing separate CI
commands for testing, as there is some mess in how dependencies are
currently tracked (this could be improved) and thus may be flakey.

TODO: docs
BREAKING CHANGE: config files need to be changed.

Refs: #524
* remove/replace AbstractConfigSource::Option::toCamelCase()
* factor out `optionalNonRepeatableOption`
* use asUnixOptionString

Closes #444 (Change casing of command line arguments to kebab-case)

Refs: #524
- Add struct `QueryOptions` and add it as field to `RuntimeConfig`.

- The bulk of `RuntimeConfig` becomes `ApiOptions`; `ApiOptions` was
  already introduced in commit c8cf88e ("refactor: enable
  YAMLFiles as ConfigSources to have substructs").

- add `overwrite` methods to ApiOptions and QueryOptions (and
  RuntimeConfig as a delegate)

Refs: #524
This replaces the previous way for choosing execution mode via `--api`
option with two different program names, but to save on linker time
and disk space build a multi-call binary that is made available under
the two new names via symlinks.

- Remove `--api` option.

- Add `enum class ExecutionMode { PREPROCESSING, API }`, and
  `executionModeFromPath` function that detects the case from the
  program path.

- Add a new class `SiloApp` that imlements `displayHelp` for shared
  help text.

- Implement separate `SiloPreprocessor` and `SiloServer` classes that
  inherit/implement `Poco::Util::ServerApplication` via `SiloApp`
  instead of just the previous `SiloServer`.

- Do not catch exceptions in the program `main` procedure if
  `DEBUG_EXCEPTION` is set (one step towards being able to use the
  debugger to look at exception contexts within http handlers, Poco
  itself still prevents it).

- Avoid duplication of code for adding of the database config option
  via new `addDatabaseConfigOption(..)` function.

- Generate a single binary now called `silo` instead of `siloApi`, but
  additionally create `siloServer` and `siloPreprocessor` symlinks to
  it via cmake in `CMakeLists.txt`.

- Update `Dockerfile`.

- Update the files in `.run/`.

Closes #524
That seems like a crude approach, but it gets the job done.

Refs: #524
config|config_reader => config_source, because the type is called
config source.

Refs: #524
…se strings

XXX sigh.  so  is it wrong now weil now zu VIEL  unix making drin isch gell ?
Because it covers preprocessing, too, and it's more natural to see
"main" in the file name to know that this is what defines the `main`
function.

Refs: #524
…_config_file`

* replaces the `Option` constants in silo::config
* replaces the manually written fields in the struct
* replaces the implementation of `overwrite`

* XXX and more to come

hat ZU VIEL:
 * removal of DATABASE_CONFIG_OPTION  mit ALT "databaseConfig" string gosh,
   muss doch bei " add field  for database config  to preprocessingconfig gl" sein.
   but eh alles zusamm ?  chm jenes hierher holen.
…k over all changed code should happen as well.

Now compiling, passing all tests and rebased on main
@Taepper Taepper changed the base branch from commandlinesplit to main November 21, 2024 09:20
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.

2 participants