Refactor build to not use patches on ethereumapi

alonmuroch commented 4 years ago

ethereumapi is used widely through out the project. prysm uses Bazel patches to modify ethereumapi with a bunch of addons, some are tags for serialisation.

For any other project to use ethereumapi it will need to apply that patch as well but some of the modifications in it are hard coded, for example the prysm workspace name.

For ethereumapi to truly be a standalone project that can be used anywhere it needs to encompass those patches as well.

@prestonvanloon

prestonvanloon commented 4 years ago

cc: @rauljordan

I understand it is difficult or cumbersome to extend Prysm's flavor of Ethereum APIs.

A few clarifying questions:

How do you handle your dependency management in your project? (Bazel, go mod, vendored?)
What are your most basic needs? (gRPC + ssz, just gRPC, just ssz?)

alonmuroch commented 4 years ago

@prestonvanloon @rauljordan

I use Bazel
My particular case was building a remote wallet that connects to the validator, serves pub keys and signs. The remote wallet has the responsibility to prevent itself signing a slashable request. He gets BeaconBlockHeader/ AttestationData and needs to locally serialise them. Thats where the patch issue comes into play as we need the ssz tags to be equal, they will be equal if I apply the patch as well on a remote wallet (which is a different project) It might cause other issues but that's where I encountered it.

prestonvanloon commented 4 years ago

OK great. Thanks for the response.

I am thinking that we can provide a starlark workspace macro, such as prysm_dependencies() where you'd automatically receive the same ethereumapis version (and patch) that we are targeting. Would that be a helpful solution?

alonmuroch commented 4 years ago

I'll do that and get back with some insights.

prestonvanloon commented 4 years ago

I can take a look at this idea tomorrow. Gazelle (the BUILD file generator for go) has functionality where we can specify to write dependencies in a macro rather than in the workspace.

Alternatively, you can simply reference our patch as @prysm//third_party:com_github_prysmaticlabs_ethereumapis-tags.patch

Example:


http_archive(
   name = "prysm",
   urls = ["https://github.com/prysmaticlabs/prysm/archive/78a865eb0bf332117d46859ffc8affb8c3d46b38.tar.gz"],
   strip_prefix = "prysm-78a865eb0bf332117d46859ffc8affb8c3d46b38",
)

go_repository(
    name = "com_github_prysmaticlabs_ethereumapis",
    commit = "62fd1d2ec119bc93b0473fde17426c63a85197ed",
    importpath = "github.com/prysmaticlabs/ethereumapis",
    patch_args = ["-p1"],
    patches = [
        "@prysm//third_party:com_github_prysmaticlabs_ethereumapis-tags.patch",
    ],
)

With this, you won't need to do any manual maintenance of the patch file.

Edit: Thinking about it some more, we have some custom rule stuff in that patch flie that might need to be namespaced. Let me know how that goes or please share an example and I can take a look!

alonmuroch commented 4 years ago

@prestonvanloon for now in order to make the patch work the workspace namespace needs to be prysm as well. Not sure if we can make it dynamic. The above could work in terms of keeping the latest patch up to date though I suspect changes will require more changes (like adding more dependencies into the workspace file if you add any more dependencies to ethereumapi).

prestonvanloon commented 4 years ago

Prysm's workspace namespace is prysm. On the point of dependencies, yes that is still a concern. I think we'll need to add a rule and test to ensure the dependencies.

I'll spend a few minutes on this today.

alonmuroch commented 4 years ago

@prestonvanloon what is the concern of having all the ssz tags within the actual ethereumapi project? (leaving the work needed to do that) For me it seems they should be there and not as a patch as they are very important part to make ethereumapi interop

rauljordan commented 4 years ago

Hi @alonmuroch thanks again for brining this up - we're very happy to introduce this change and it is now live in the latest master branch of ethereumapis and soon in Prysm with #5570 :)

prestonvanloon commented 4 years ago

This has been done. @alonmuroch can you let us know if there is more we need to do here to resolve your use case? Thanks

prestonvanloon commented 4 years ago

Sorry, I commented too soon. Now it is done :)

alonmuroch commented 4 years ago

@prestonvanloon @rauljordan that's great! We will be testing it out in the next few weeks.

prestonvanloon commented 4 years ago

I think we mentioned this at one point, but we’re refactoring dependencies to be a star lark rule that can be called in a downstream project. Follow #6005 if you were interested

alonmuroch commented 4 years ago

@prestonvanloon I've added something along those lines already https://github.com/prysmaticlabs/ethereumapis/blob/master/tools/ethereumapi_dependencies.bzl

prysmaticlabs / prysm

Refactor build to not use patches on ethereumapi #5130