Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update gogo protobuf dependency #3214

Closed
whyrusleeping opened this issue Sep 12, 2016 · 18 comments · Fixed by #5355
Closed

Update gogo protobuf dependency #3214

whyrusleeping opened this issue Sep 12, 2016 · 18 comments · Fixed by #5355
Assignees
Labels
help wanted Seeking public contribution on this issue topic/technical debt Topic technical debt

Comments

@whyrusleeping
Copy link
Member

We havent updated our gogo-protobuf dep in a while. The primary reason for this is that its not as simple to do as a lot of our other deps. The repo itself is huge, and our current gogo-protobuf package is roughly 40MB. This make the gx install ux slow and really annoying.

We should look at repackaging a newer version with exactly what we need, or finding a better way to shrink this package to just what we need to make the process of keeping things updated easier.

cc @Kubuxu @parkan @vyzo

@whyrusleeping whyrusleeping added the help wanted Seeking public contribution on this issue label Sep 14, 2016
@flyingzumwalt flyingzumwalt added the status/deferred Conscious decision to pause or backlog label Sep 26, 2016
@vyzo
Copy link
Contributor

vyzo commented Sep 27, 2016

This has escalated into a serious problem for us, because the vendored gogo-protobuf apparently generates code that can't marshall/unmarshall oneofs.

Is there any workaround with gx to make it not rewrite the gogo-protobuf dependency in our tree? We don't explicitly depend on the gx vendored package, but it gets rewritten because of the transitive dependency through libp2p.

@Kubuxu
Copy link
Member

Kubuxu commented Sep 27, 2016

The huge part of gogo-protobuf are tests (there is tests that has 400k lines). We could add them to .gxingnore.

@Kubuxu
Copy link
Member

Kubuxu commented Sep 27, 2016

Also it looks like we are using only /proto and /io.

@Kubuxu
Copy link
Member

Kubuxu commented Sep 27, 2016

Removing /test form the gx export will leave us with about 1MiB code package.

@Kubuxu
Copy link
Member

Kubuxu commented Sep 27, 2016

@vyzo you can try importing QmVsHZSaohNEZrzNuBPi9z63iiJxWc2rvN4PZKqJXQ9oU5 which is current master of gogo/protobuf but I have no idea how it will work (there might be backward incompatible changes).

@vyzo
Copy link
Contributor

vyzo commented Sep 27, 2016

@Kubuxu thanks, the explicit import of this new version works with protos compiled with unvendored gogo-protobuf!

@vyzo
Copy link
Contributor

vyzo commented Sep 27, 2016

And needless to say, it solves our marshaling woes :)

@Kubuxu Kubuxu added the topic/technical debt Topic technical debt label Oct 3, 2016
@Kubuxu
Copy link
Member

Kubuxu commented Oct 28, 2016

The process became just a bit more complicated, as gogo protobuf fixed bug with order of values.

Version of protobuf used by us includes the bug, current protobuf 3 version is fixed.

Meaning that if we update the protobuf library hashes of new files will change.

@Kubuxu
Copy link
Member

Kubuxu commented Oct 28, 2016

gogo/protobuf#42

@ribasushi
Copy link
Contributor

Was browsing around and ran across this issue.

Since you are about start producing new hashes anyway ( due to the different order of protobuf "frames" ) may I suggest re-declaring repeated varint fields as packed? According to the spec packed fields are forward and backward compatible...

This will save 1~2 bytes per unixfs link (and likely for other higher protocols too)

@Kubuxu
Copy link
Member

Kubuxu commented Nov 24, 2016

We won't be changing output hashes just to update protobuf Also as far as I know, we won't be touching protobuf-unixfs format. We plan deprecating it, and maybe creating ipld-unixfs in version 0.5 but it isn't still decided, AFAIK.

@whyrusleeping might know more about this.

@ribasushi
Copy link
Contributor

@Kubuxu I meant the above suggestion given that hashes will change as per your own writeup here: #3214 (comment)

@Kubuxu
Copy link
Member

Kubuxu commented Jan 29, 2017

As we don't want to change the hashes, we can't update for this new version of gogo-protobuf.

But we should update gogo-protobuf to a version with test vectors removed. It is 19MiB of 29MiB package, we probably could remove some other packages from it too.

@lgierth what is the status of the gx-worktree tool? Could it be used to painlessly create series of those PRs?

@Stebalien
Copy link
Member

Stebalien commented Aug 8, 2018

I'm currently working on this.

Plan:

@Stebalien Stebalien self-assigned this Aug 8, 2018
@mib-kd743naq
Copy link
Contributor

this involves making sure we don't re-generate the marshaler code

@Stebalien this seems like a suboptimal plan. What you want is to update gogo, apply the patch reverting the sorting change, and publish that in gx-go. Then you do regenerate the marshallers, taking advantage of improvements in the generated code. Perpahs even switching to the faster flavor ( no carriage of unrecognized field values )

The only reason I did not reopen/finish #5012 is because I ran out of time trying to figure out how gx-go works, and then never came back to it :(

@Stebalien
Copy link
Member

Then you do regenerate the marshallers, taking advantage of improvements in the generated code. Perpahs even switching to the faster flavor ( no carriage of unrecognized field values )

We should do that as well however, we want all of this to work without gx. That's why I'm taking this approach (use upstream as much as possible).

@Stebalien
Copy link
Member

The sharness tests pass. Running the interop tests now.

@Stebalien
Copy link
Member

The interop tests pass (those that usually do, at least).

Stebalien added a commit that referenced this issue Aug 9, 2018
fixes #3214

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@ghost ghost added status/in-progress In progress and removed status/deferred Conscious decision to pause or backlog labels Aug 9, 2018
@ghost ghost removed the status/in-progress In progress label Aug 9, 2018
hacdias pushed a commit that referenced this issue Nov 29, 2023
fixes #3214

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>


This commit was moved from ipfs/interface-go-ipfs-core@1ef1634

This commit was moved from ipfs/boxo@e415aa1
hacdias pushed a commit that referenced this issue Nov 29, 2023
fixes #3214

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>


This commit was moved from ipfs/interface-go-ipfs-core@1ef1634

This commit was moved from ipfs/boxo@e415aa1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Seeking public contribution on this issue topic/technical debt Topic technical debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants