binapigen cli improvements #67
Conversation
This patch also proposes an evolution to the binapigen CLI in order to make it more user-friendly & discoverable and allow build pipelines based on go:generate For now two usecases are in the picture: - Building from a directory containing .json files e.g. 'binapigen --api /usr/share/vpp/api --output ./myapp/bindings --filter interface,ip' - Building from a cloned local VPP e.g. 'binapigen --vpp ~/vpp --output ./myapp/binding' Signed-off-by: Nathan Skrzypczak <[email protected]> Change-Id: Idc2893e0ad4b1e1d2278d38e64d41becbd143ca2
sknat
force-pushed
the
feature/binapigen-cli
branch
from
00ea7fe to
d060f79
Compare
| NoVersionInfo bool // disables generating version info | ||
| NoSourcePathInfo bool // disables the 'source: /path' comment | ||
| ActivePluginNames []string | ||
| ApiDir string |
There was a problem hiding this comment.
Directory with API files should NOT really be an input parameter for the Generator. It should work only with the []vppapi.File and let caller load & parse from directory. This makes the Generator very simple, the input parameters are not coupled with filesystem and expect only API files defined with pre-defined structure. This allows the Generator to work in the runtime, which means it could for example: load API files from specific commit of some git repo, generate the JSON API files, load them and use as input to Generator.. or retrieve the API schemas directly from VPP when running (this would be very simple if we could retrieve the VPP API definition via the VPP API itself) without having any generated Go bindings before hand - the universal VPP API client.
| enumsByName: map[string]*Enum{}, | ||
| aliasesByName: map[string]*Alias{}, | ||
| structsByName: map[string]*Struct{}, | ||
| unionsByName: map[string]*Union{}, | ||
| messagesByName: map[string]*Message{}, | ||
| vppVersion: vppapi.ResolveVPPVersion(opts.ApiDir), |
There was a problem hiding this comment.
This related to the previous comment and the reasoning why the vpp version was injected into generator before.
EDIT: It seems like we need some definition of input parameters to generator. Something like:
type Input struct [
APIFiles []vppapi.File
VPPVersion string
}|
|
||
| type Plugin struct { | ||
| Name string | ||
| GenerateFile func(*Generator, *File) *GenFile | ||
| GenerateFile func(*Generator) |
There was a problem hiding this comment.
Should rather be named Generate, because we are dropping the *File parameter.
| return fmt.Errorf("plugin not found: %q", name) | ||
| func GetAvailablePluginNames() string { | ||
| s := make([]string, 0) | ||
| for k := range registeredPlugins { |
There was a problem hiding this comment.
This will create list with random order, won't it? That was reason for having the global plugins slice together with plugin map.
|
|
||
| // Filtering API files | ||
| filterList := flag.String("input-file", "", "[DEPRECATED: Use --filter] defines apis to generate.") | ||
| flag.StringVar(filterList, "filter", "", "Comma separated list of api to generate (e.g. ipip,ipsec, ...)") |
There was a problem hiding this comment.
Beside breaking users again, as this paramater has already changed the way it is set.. it will require users to write extra flag, making the generator line longer, which was exactly what the last change was supposed to eliminate.
The arguments to the generator supported using any separator including comma, right?
Is there any other reason to add this flag that I am missing?
| noSourcePathInfo = flag.Bool("no-source-path-info", false, "Disable source path info in generated files.") | ||
| ) | ||
| apiSrcDir := flag.String("input-dir", vppapi.DefaultDir, "[DEPRECATED, use --api] Input directory containing API files.") | ||
| flag.StringVar(apiSrcDir, "api", "", "Generate based on .api files in this directory.") |
There was a problem hiding this comment.
The API directory and VPP directory could be resolved automatically from the input-dir without needing user to set them in different parameters.
Let's say we rename the input-dir to input to make it shorter and resolve directory depending on the value of input:
input=""- is not set (default value)
- check if VPP is installed
- load API files from installed VPP or return error if not installed
- generate
input=<API_DIR>- is a path to directory withcoreandpluginsdirectories
- then assume it is the API directory
- load API files from
input - generate
input=<VPP_DIR>- is a path to directory with VPP source code (cloned git repository)
- then assume it is the VPP directory we do development on
- run
make json-api-files - load API files from
build-root/install-vpp-native/vpp/share/vpp/api/(relative toinput) - generate
Using a single parameter to define the input this opens up possibility to to accept even more types of inputs, making it very convenient:
input=<VPP_REPO_REF>- is a URL to a remote VPP repository / commit
- then clone the repository into temp dir
- continue as in step 3
input=<VPP_VERSION>- is a VPP version / branch name
- then clone the VPP repository into temp dir
- checkout tag or branch specified by
input - continue as in step 3
|
Closing as this will be implemented by #93. |
This patch also proposes an evolution to the binapigen CLI
in order to make it more user-friendly & discoverable
and allow build pipelines based on go:generate
For now two usecases are in the picture:
e.g. 'binapigen --api /usr/share/vpp/api --output ./myapp/bindings --filter interface,ip'
e.g. 'binapigen --vpp ~/vpp --output ./myapp/binding'