[jnigen] Config API update - #3516
Conversation
Package publishingIf you have publishing permissions, you can use the links below to publish the changes after merging this PR.
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with |
| classes: ['com.github.dart_lang.jnigen.simple_package'], | ||
| sourcePath: [Uri.directory(sourcePath)], | ||
| input: Input( | ||
| classes: ['com.github.dart_lang.jnigen.simple_package'], |
There was a problem hiding this comment.
Helicopter view level question: We can't put this as include on visitClass right? Because this goes into the summarizer which is a different stage in the pipeline.
I think the readme of both FFIgen and JNIgen would benefit from mermaid diagrams showing the pipeline and what in the config is configuring which part of the pipeline. (Feel free to file an issue to do this later. Mermaid diagrams are rendered on GitHub, I don't know about pub, and probably not in dartdoc either.)
Another super high level question, should options only passed to the summarizer be nested in the summarizer options? That might make it more clear for users.
There was a problem hiding this comment.
I think the readme of both FFIgen and JNIgen would benefit from mermaid diagrams showing the pipeline and what in the config is configuring which part of the pipeline. (Feel free to file an issue to do this later. Mermaid diagrams are rendered on GitHub, I don't know about pub, and probably not in dartdoc either.)
Done #3527
Another super high level question, should options only passed to the summarizer be nested in the summarizer options? That might make it more clear for users.
Hmmm. Tricky question.
- Most of
Input's remaining fields are only passed to the summarizer. EvenMavenDownloadsandAndroidSdkpopulatesourcePathandclassPath, which are passed to the summarizer. The only one that is completely separate isimports. - I don't necessarily want to advertise to users that the summarizer is a separate tool we run. There's a backlog bug somewhere to port the summarizer to Dart. In that case,
extraArgs,workingDirectory, and maybe evenbackendcould be deleted. classes,sourcePath, andclassPathare way more common than any of the options inSummarizerOptions.- Just realized that in the FFIgen config,
importTypeis a top level member of the config object.
With all that in mind, maybe a better design is to move imports to be directly under Config, and then delete SummarizerOptions and flatten all its options directly into Input?
There was a problem hiding this comment.
- I don't necessarily want to advertise to users that the summarizer is a separate tool we run.
Hm, the fact that we have a summarizer puts a specific burden on the user right now, listing the class names.
There's a backlog bug somewhere to port the summarizer to Dart.
I would imagine we completely drop the list of classes then.
- Just realized that in the FFIgen config,
importTypeis a top level member of the config object.
The imports in FFIgen are for types that are generated by other FFIgen runs right? That "feels" more like a different source of information than the header files. I think conceptually the input relates to the parsing stage of the native code.
So I'd probably keep all the summarizer options in input then, since that's the parsing stage. Including the list fo classes.
And maybe importType should be nested in another config that indicates it is about importing already generated bindings generated by the same tool.
There was a problem hiding this comment.
So I'd probably keep all the summarizer options in input then, since that's the parsing stage. Including the list fo classes.
Do you mean keep it as it is in 5c24777 with the 3 old fields in SummarizerOptions, as it is in 11ccc34 with everything in Input. Or do you mean move classes, sourcePath, and classPath into SummarizerOptions?
There was a problem hiding this comment.
I think 11ccc34 looks slightly more natural to me. We don't explicitly call out the summarizer in the name but basically all of those fields relate to the parsing stage and are used with the summarizer.
But if you feel strongly about it, I don't have a strong opinion.
There was a problem hiding this comment.
I think that's the best approach too.
| ], | ||
| ), | ||
| output: Output( | ||
| dart: DartCodeOutputConfig( |
There was a problem hiding this comment.
We can't use dot shorthands to skip typing the constructor name can we?
There was a problem hiding this comment.
I think you might be able to do dart: .new(? Personally I don't find that more readable.
There was a problem hiding this comment.
Ah right, yeah I also find that not readable 😅
| ], | ||
| ), | ||
| output: Output( | ||
| dart: DartCodeOutputConfig( |
There was a problem hiding this comment.
We dropped Config suffix everywhere, this still has a config suffix.
There was a problem hiding this comment.
I noticed some others still have the config suffix as well, e.g. AndroidSdkConfig.
Update JNIgen's config API to align it with the proposed FFIgen API.
classes,sourcePath,classPath,summarizerOptions,mavenDownloads, andandroidSdkConfigintoInputimportandhideintoSymbolImports, which lives inInput.importsOutputConfigrenamed toOutput, and still containsdartConfig->dartandsymbolsConfig->symbols. Now also containspreamble,generateStubs, andformat.nonNullAnnotationsandnullableAnnotationsnow grouped underNullabilityAnnotations.isExcludedtoisIncluded.Sorry for the crazy number of changed files. It's mostly due to the version bump. There aren't any real bindings changes. Important files are:
#2062