config refactoring #51
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
kotlinx.fuzz.gradle/src/main/kotlin/kotlinx/fuzz/gradle/FuzzConfigDSL.kt
Fixed
Show fixed
Hide fixed
kotlinx.fuzz.gradle/src/main/kotlin/kotlinx/fuzz/gradle/FuzzConfigDSL.kt
Fixed
Show fixed
Hide fixed
DLochmelis33
added
ready for review
kotlinx.fuzz.api/src/main/kotlin/kotlinx/fuzz/config/CoverageConfig.kt
Outdated
Show resolved
Hide resolved
kotlinx.fuzz.api/src/main/kotlin/kotlinx/fuzz/config/EngineConfig.kt
Outdated
Show resolved
Hide resolved
| // TODO: we have to know which engine to use before building the config... | ||
| // Viable solution for future: build a stub config, check the engine, build a new config. | ||
| // Will need to be careful with instantiating delegates, as they will get validated. |
There was a problem hiding this comment.
Why can't we initialize all of them? I believe that users can have settings for several engines and use one of them by specifying engine field. It might be unclear how to keep API independent on supported engines, but since we directly specify jazzer here it is ok (probably)
There was a problem hiding this comment.
This is what Will need to be careful with instantiating delegates, as they will get validated. means here: if we try to initialize them, but properties are missing, validation will fail. I guess an alternative option is to allow some properties to remain invalid
There was a problem hiding this comment.
If nobody objects, I will leave this as-is for the time being
kotlinx.fuzz.api/src/main/kotlin/kotlinx/fuzz/config/KFuzzConfig.kt
Outdated
Show resolved
Hide resolved
kotlinx.fuzz.api/src/main/kotlin/kotlinx/fuzz/config/GlobalConfig.kt
Outdated
Show resolved
Hide resolved
| val keepGoing: Long = KFuzzConfigImpl.Companion.Defaults.KEEP_GOING, | ||
| val maxFuzzTime: String = KFuzzConfigImpl.Companion.Defaults.MAX_SINGLE_TARGET_FUZZ_TIME_STRING, | ||
| val keepGoing: Long = TargetConfig.Defaults.KEEP_GOING, | ||
| val maxFuzzTime: String = TargetConfig.Defaults.MAX_FUZZ_TIME_STRING, | ||
| val instrument: Array<String> = [], | ||
| val customHookExcludes: Array<String> = [], |
There was a problem hiding this comment.
Maybe, we should move instrument and customHookExcludes from TargetConfig to GlobalConfig. I can't see reasons to instrument different classes depending on target (we could instrument only the code we execute, but it's a different thing). Same for customHookExcludes: if we want to exclude something, we most likely want to exclude it everywhere
There was a problem hiding this comment.
I'm not sure that I fully agree with that. I think there might be reasons to want to instrument different parts of code for different targets. Because depending on what is instrumented, fuzzer will change its behaviour. So if I want a target to fuzz a specific part of the project, I would want to "communicate" that idea to the fuzzer
There was a problem hiding this comment.
IMO it makes more sense to have customHookINCLUDES for some targets and others not. But this is again a question about borrowing Jazzer naming. Will remove these two from @KFuzzTest arguments as well
kotlinx.fuzz.api/src/main/kotlin/kotlinx/fuzz/config/CoverageConfig.kt
Outdated
Show resolved
Hide resolved
|
|
||
| sealed interface EngineConfig | ||
|
|
||
| interface JazzerConfig : EngineConfig { |
There was a problem hiding this comment.
Right now, JunitEngine doesn't depend on any concerete FuzzEngine. However, here we have JazzerConfig in the API. From my point of view, concerete FuzzEngines should be configured somehow independently from the kotlinx.fuzz in general, since we might want to change them
There was a problem hiding this comment.
I too dislike the fact that all configs have to go into api subproject. That may be possible without ruining the syntax too much, but internal apis like KFuzzProperty will have to be made public and leak outside. In general what we are trying to do here is serialization into properties, and in order to not risk implementing our own kotlinx.serialization, I suggest keeping this as-is
|
|
||
| interface JazzerConfig : EngineConfig { | ||
| val libFuzzerRssLimitMb: Int | ||
| val enableLogging: Boolean |
There was a problem hiding this comment.
I think, we should rework this option
- It doesn't "enable logging". It repeats JazzerEngine's log into console
- It would be usefull for any
FuzzEnginenot just for jazzer.
So, consider moving it to global config under name like "detailedConsoleLog"
There was a problem hiding this comment.
On that note, I think we borrowed more names from Jazzer and we might want to reconsider them too, for example instrument seems way too unclear for me just from the name
kotlinx.fuzz.gradle/src/main/kotlin/kotlinx/fuzz/gradle/KFuzzPlugin.kt
Outdated
Show resolved
Hide resolved
|
Let's merge this PR before any others, because it conflicts with basically everything and I'm tired of fixing merge conflicts :D |
| val hooks: Boolean | ||
| val logLevel: LogLevel | ||
| val regressionEnabled: Boolean | ||
| val detailedLogging: Boolean |
There was a problem hiding this comment.
Based on its usage, it is more like "loggingToConsole". We log information in a file anyway, this flag just copies it to the standard output
|
Overall, it looks good to me |
list of changes:
KFuzzConfig, it now supports a) splitting properties into separate files b) granular editing via builder c) project properties