Add configuration to intercept #504
Conversation
This function converts the command line arguments into a ic::Configuration struct to be passed to the various components of intercept. For now this method starts with a default constructed Configuration, but in the future the function should use a user supplied config.
This pacth makes it so that all the components use the ic::Configuartion struct instead of parsing the flags themselfs.
| auto force_wrapper_arg = args.as_bool(cmd::intercept::FLAG_FORCE_WRAPPER); | ||
| auto command_arg = args.as_string_list(cmd::intercept::FLAG_COMMAND); | ||
|
|
||
| ic::Configuration config; |
There was a problem hiding this comment.
For now the configuration is default constructed (it's initialized with default values), but it would be better that the user can supply his own configuration (this will be done in a subsequent PR).
There was a problem hiding this comment.
What do you mean by "supply his own configuration"?
I think the user is already supplying the configuration, via environment variables and command line arguments. That should do.
Or do you mean to craft a constructor for this type? I think you can initialize a struct too.
|
|
||
| return rust::merge(execution, session, reporter) | ||
| return into_configuration(args) | ||
| .and_then<std::tuple<Execution, Session::Ptr, Reporter::Ptr>>([&envp](const auto& configuration) { |
There was a problem hiding this comment.
Passing this long template argument is a bit ugly, but that is the only way to make it work.
Maybe with deduction guides it's possible to make the compiler deduct the type, but deduction guides are an obscure part of C++ and I don't know them very well 😅
| result_a | ||
| fmt::fmt | ||
| nlohmann_json::nlohmann_json) | ||
| target_compile_options(intercept_json_a PRIVATE -fexceptions) |
There was a problem hiding this comment.
The configuration code for intercept requires a different compilation target because nlohmann json requires exceptions.
There was a problem hiding this comment.
Why the Configuration type needs JSON formatting? My understanding that this is an internal type, and we don't expose it to the user.
| auto force_wrapper_arg = args.as_bool(cmd::intercept::FLAG_FORCE_WRAPPER); | ||
| auto command_arg = args.as_string_list(cmd::intercept::FLAG_COMMAND); | ||
|
|
||
| ic::Configuration config; |
There was a problem hiding this comment.
What do you mean by "supply his own configuration"?
I think the user is already supplying the configuration, via environment variables and command line arguments. That should do.
Or do you mean to craft a constructor for this type? I think you can initialize a struct too.
| result_a | ||
| fmt::fmt | ||
| nlohmann_json::nlohmann_json) | ||
| target_compile_options(intercept_json_a PRIVATE -fexceptions) |
There was a problem hiding this comment.
Why the Configuration type needs JSON formatting? My understanding that this is an internal type, and we don't expose it to the user.
|
|
||
| namespace fs = std::filesystem; | ||
|
|
||
| namespace ic { |
There was a problem hiding this comment.
What I understand from the change... this is the type we create from the command line arguments (and environment variables), and passing an instance to the components which needs configuration. This was done before by passing the flags::Argument object. The benefit to doing it:
- we can do it only once in a central place,
- run a full validation on the flags,
- and the callers does not need to deal with
flags::ArgumentsfunctionsResultreturn types.
Did I get this right?
In this form, we have a type, that all parts of the system depends on. (Same as with flags::Arguments, but it's easier to use.) How about to slice this type up into specific configuration?
struct SessionConfig {};
struct LibraryPreloadSessionConfig : SessionConfig {
fs::path library;
fs::path executor;
};
struct WrapperSessionConfig : SessionConfig {
fs::path wrapper_dir;
std::map<std::string, fs::path> mapping;
std::map<std::string, fs::path> override;
}
struct ExecutionConfig {
bool verbose;
std::list<std::string> command;
};
struct OutputConfig {
fs::path output_file;
};
struct Configuration {
ExecutionConfig execution;
OutputConfig output;
std::shared_ptr<SessionConfig> session;
};This is what we do for complex applications. Each subsystem defines its own configuration and validation logic. The central configuration is just holds the values together, facilitate the validation process. The subsystems does not need to know any common type (less coupling).
I think this is not very different what the current master branch has. (Instead of having SessionConfig and Session types, it only has Session which does both the argument parsing and implement the logic.) For a small application like this, I think it might not worth the benefit.
My understanding was that we want to simplify Bear to not execute processes, but make an internal calls only. I don't clearly see how a centralized configuration will help us to get that?
This PR moves all the configuration values received from the flags to a Configuration struct that gets passed to all the components of intercept