feat: gethclone binary
#1215
feat: gethclone binary
#1215
Conversation
| # 2. Sort the unique results | ||
| # #. Print out the difference between the search results and the list of specified allowed package imports from geth. | ||
| extra_imports=$(grep -r --include='*.go' --exclude-dir='simulator' '"github.com/ethereum/go-ethereum/.*"' -o -h | sort -u | comm -23 - ./scripts/geth-allowed-packages.txt) | ||
| extra_imports=$(grep -Pr --include='*.go' --exclude-dir='simulator' '^\s+"github.com/ethereum/go-ethereum/.*"' -o -h | tr -d '[:blank:]' | sort -u | comm -23 - ./scripts/geth-allowed-packages.txt) |
There was a problem hiding this comment.
There was a false positive on const geth = "github.com/ethereum/go-ethereum/" so I updated the check to only match with (possible) whitespace but no other prefix.
|
Interesting. For mechanical changes like imports or struct modifications this seems like a very promising approach. Might this approach also be appropriate for more complex fork maintenance (i.e. maintaining our changes at the AST level rather than as git commits)? |
I think it would form part of a broader strategy. Changes like you describe, as well as deletions and other "simple" modifications can be easily implemented as an AST change. Additions of entire methods/functions should just be done as new files already in the destination module (i.e. regular Go code). More complex code changes will be addressed as we encounter them. Cloned+transformed files should probably still be checked in so others can import the package, but with pre-merge checks to confirm that they're up to date with Advantages of maintaining the changes as AST and in-situ files:
The primary disadvantage is the complexity of writing an AST patch although I plan on writing some generators; that would be used something like: patches.AddStructField(geth("core/types"), "StateAccount", &ast.Field{..."IsMultiCoin"...})
patches.RemoveMethod(geth("common"), "Address", "Cmp") |
|
No argument that the current approach of merging changes without shared history is not ideal, but how would you compare/contrast the AST approach proposed in this PR with maintenance of a proper fork with shared git history for which sets of persistent and transient carry patches are maintained? |
x/gethclone/main.go
Outdated
| processed map[string]bool | ||
| } | ||
|
|
||
| const geth = "github.com/ethereum/go-ethereum" |
There was a problem hiding this comment.
maybe consider moving this into a flag while retaining this value as the default value ?
There was a problem hiding this comment.
What's the end goal in having it configurable? This will likely have flow-on effects (e.g. the _ import would be insufficient to guarantee that packages.Load() can resolve the package).
Context for others as I didn't know about transient/carry patches: https://github.com/openshift/kubernetes/blob/master/REBASE.openshift.md As for an answer: I'll get back to you @marun because I don't know yet. |
Moved into |
Co-authored-by: Ceyhun Onur <[email protected]>
3 participants
Why this should be merged
From
x/gethclone/README.md:How this works
A subset of geth packages is specified via the
--packagesflag, retrieved with Go's native package resolution. The import graph is traversed for all otherethereum/go-ethereumpackages, and (as yet undefined) AST patches are applied. The--output_go_modflag specifies thego.modfile of the module to which the transformed files are to be written.See examples of intended future use in comment below.
How this was tested
The
astpatchpackage has tests to demonstrate proper AST traversal. As no real patches exist yet, this only records and confirms visited nodes.The
gethclonebinary itself is not yet tested. The intention is that the cloned files themselves will have tests to demonstrate proper transformation. All*_test.gofiles will be cloned and run. Modifications (e.g. addition of struct fields) will have tests defined in the destination module. Ideas for more specific tests are welcome.How is this documented
READMEs and Go comments.