[BEAM-3612] Add a shim generator tool #7000
Conversation
|
R: @aaltay |
| @@ -57,6 +57,13 @@ func Generate(w io.Writer, filename, pkg string, ids []string, fset *token.FileS | |||
|
|
|||
| e.Print("*/\n") | |||
| data := e.Generate(filename) | |||
| if err := write(w, []byte(license)); err != nil { | |||
| return err | |||
There was a problem hiding this comment.
Do generated files need to have a Apache license?
There was a problem hiding this comment.
In principle: probably not.
In practice: the RAT check will likely complain if we don't, when trying to check in the files.
Note: We need to check in the files, since the design of go generate requires it be invoked manually, and the results are intended to be checked in.
Go says: go generate is for library authors, not library users.
Similarly, the protos are generated code, but we need to check in their results. (They don't have the license though, but they may pre-date the RAT check).
There was a problem hiding this comment.
Ack. But this is strange for the end users of the tool. They do not necessarily need to want to use a this license.
Perhaps a TODO for the future to see if we can resolve this with the RAT tool.
There was a problem hiding this comment.
Given it's an Apache tool generating the code, it's not a huge stretch that it should include the Apache license.
And a user that takes particular umbrage can always use the starcgenx package to write their own tooling to generate the code.
I would happily accept a patch (or spend the 2 minutes necessary to write the code) to flag the license inclusion lines, if a user requested it. I'd rather not speculate on possible usecases, and generalizing/flagging before there's a concrete need.
|
Thanks for the review! With this in, I'll be able to write a bunch of relatively rote CLs to add the shims to the provided transforms. They will largely be similar to #7002 (minus the diffbase of this PR) Edit: I needed to modify that PR anyway, since of course all the numeric types will lead to collisions if I do individual generations. Makes sense, the "simplest" example one per package, rather than dealing with individual package files. |
|
@aaltay If it LGTYou Please merge. Thanks! |
|
@aaltay Ping :) |
Adds a type assertion shim generation tool, akin to specialize.
In particular it takes in a list of identifiers and go files from a single package and generates specialized type assertion shims for the given identifiers, using static analysis on the package code to
A subsequent PR will do the work of adding the appropriate // go:generate tags to relevant beam provided DoFns to improve the performance of these.
A user could use the tool themselves with build tools like https://bazel.build or gradle with an appropriate invocation file, and avoid committing the generated files to their own repos, but have them when building with the tool. This doesn't help users who
go getthe beam packages though so we're best checking these into the beam repo.Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.It will help us expedite review of your Pull Request if you tag someone (e.g.
@username) to look at it.Post-Commit Tests Status (on master branch)