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

v0.20.0-rc2: cannot build plugins #9839

Closed
3 tasks done
hsanjuan opened this issue Apr 26, 2023 · 21 comments
Closed
3 tasks done

v0.20.0-rc2: cannot build plugins #9839

hsanjuan opened this issue Apr 26, 2023 · 21 comments
Assignees
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization

Comments

@hsanjuan
Copy link
Contributor

hsanjuan commented Apr 26, 2023

Checklist

Installation method

ipfs-update or dist.ipfs.tech

Version

Kubo version: 0.20.0-rc2
Repo version: 13
System version: amd64/linux
Golang version: go1.19.8

Config

No response

Description

Building plugins that link against Kubo v0.20.0-rc2 is not possible:

asm: xxhash_amd64.s:120: when dynamic linking, R15 is clobbered by a global variable access and is used here: 00092 (/home/hector/go/pkg/mod/github.com/cespare/[email protected]/xxhash_amd64.s:120)	ADDQ	R15, AX
asm: assembly failed

The issue has been fixed in cespare/xxhash#54. However, boxo/gateway pulls xxhash v1, which seems unmaintained and carries this issue.

I don't know why this hasn't shown up before... boxo?

@hsanjuan hsanjuan added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Apr 26, 2023
@hsanjuan
Copy link
Contributor Author

This can be simply reproduced with go build -buildmode=plugin in the cmd/ipfs folder.

@hsanjuan
Copy link
Contributor Author

Unfortunately, anything that imports badger suffers this: dgraph-io/badger#1777

@hsanjuan
Copy link
Contributor Author

So all plugins somehow importing badger at the version we use (i.e. via Kubo or boxo) are going to be broken.

We would need to port the badger fix to the 1.6.2 branch. 🤦

@aschmahmann would it be ok to fork badger and have this fixed ourselves? Badger at 1.6.2 is fully unmaintained...

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Apr 26, 2023

Issue would be fixed by including something like:

replace github.com/dgraph-io/ristretto => github.com/hsanjuan/ristretto v0.0.0-20230426124528-4a4a94a6bb9c

in Kubo, plus switching boxo/gateway to v2.

hsanjuan added a commit to ipfs/boxo that referenced this issue Apr 26, 2023
This is related to ipfs/kubo#9839.

boxo still depends on badger, which still requires xxhash/v1, but at least doesn't require it xxhash/v1 itself directly anymore.
@aschmahmann
Copy link
Contributor

aschmahmann commented Apr 27, 2023

Out this week so can't take a look at this yet, but this sounds like you're reporting a regression of #8550.

There are sharness tests that test building plugins and make sure they work. IIRC of the possible solutions here, the easiest was to just not build Badger as a plugin but just include the code (https://github.com/ipfs/kubo/pull/8815/files#r832461708)... awkwardly I probably neglected to file an issue about bringing back some external test building with plugins for after the dependency issues linked were resolvable.

Is it possible that some of the tooling you're using to build the plugins (e.g. the bash scripts in the kubo repo) is buggy and has re-added building Badger as an external plugin rather than just included in the codebase?

Side note: @hsanjuan I wonder if #9782 might work for you as an alternative to Go plugins. Easier/harder/similar. If you've got some thoughts and time to comment on that PR it might help motivate changing our documentation to really dissuade users from using Go plugins (as opposed to the other plugin options or using boxo) unless necessary.

@hsanjuan
Copy link
Contributor Author

The issue is that I cannot build a plugin with buildmode=plugin when linking Kubo/Boxo. But I could perfectly do that a while ago while we merged #9750.

I'm not using any Kubo tooling to build the plugin nor doing anything weird. Perhaps the issue depend on how dependencies are resolved and the order is not always the same and refactors have affected this. Or somehow before I didn't transitively require badger and now with Boxo things have moved around and everything comes in.

#9782 is meh... I can always have kubo as submodule and patch things in and get custom binaries. What I would like to have is regular plugins working so they can be dropped in with any official builds.

Anyways fix is simple and safe enough, even if not overly pretty. Without fix then I'm forced to fork.

lidel pushed a commit to ipfs/boxo that referenced this issue Apr 27, 2023
This is related to ipfs/kubo#9839.

boxo still depends on badger, which still requires xxhash/v1, 
but at least doesn't require it xxhash/v1 itself directly anymore.
@lidel
Copy link
Member

lidel commented Apr 27, 2023

fwiw I've merged ipfs/boxo#285 and boxo/gateway no longer pulls-in the old xxhash.

As for badger in kubo, badger v1 is unmaintained, and the entire company that maintained newer versions imploded 🙈

  • Short term, the fix proposed in v0.20.0-rc2: cannot build plugins #9839 (comment) sounds like the only play we have if we want to avoid issues caused by unmaintained dependency?
  • Long term.. ideas welcome?
    • my intuition is to make a plan to deprecate and remove badgerv1 from kubo, and move it to a plugin similar to go-ds-s3

@aschmahmann
Copy link
Contributor

aschmahmann commented Apr 27, 2023

@lidel before adding in replace directives or forking I think we should figure out what changed and make sure the sharness tests are working again. We've been operating with this since we added Go1.17 support and IIUC nothing has changed except maybe some dependencies moved around/swapped during the boxo migration. Seems like fixing that will do the job.

Edit: We should confirm when things broke. It's possible the "fix" linked above didn't actually do much of anything for people building plugins which if true would mean nobody has been able to build any external plugins for quite a while which could inform future directions here.

Longer term yeah, we should remove badgerv1 being built-in to kubo by default and if we want badger still included use the latest version.

@BigLep
Copy link
Contributor

BigLep commented May 2, 2023

2023-05-02 maintainer conversation:

  1. Determine when this broke
  2. If this broke last release, we likely screwed up a dependency with Boxo things.
  3. If it's been broken for 1.5 years then it makes clear than no one is using the feature and the feature is effectively dead.

@hsanjuan
Copy link
Contributor Author

hsanjuan commented May 4, 2023

It may be that this is broken for a long time. At least go build -buildmode=plugin did not work before in cmd/kubo.

But I still could build the nopfs plugin with github.com/ipfs/kubo v0.18.2-0.20230324125910-9300769122d9 and I don't seem to be able to with v0.20-0-rc2. I will look some more but with all the refactor it's not super easy to tell when things stopped from the plugin (I have to additionally manually align every other dependency in the go.mod).

@hsanjuan
Copy link
Contributor Author

hsanjuan commented May 4, 2023

Closed by mistake. So before

github.com/cespare/xxhash v1.1.0 // indirect

Did not need to be in the go.mod. Now requiring v0.20.0-rc2 makes it appear.

There is a good chance that bubbling ipfs/boxo#285 to Kubo fixes my issue since it will make xxhash only reachable via test code (again).

@hsanjuan hsanjuan reopened this May 4, 2023
@hsanjuan
Copy link
Contributor Author

hsanjuan commented May 4, 2023

go mod why:

Before:

go mod why -m github.com/cespare/xxhash
# github.com/cespare/xxhash
github.com/ipfs-shipyard/nopfs/ipfs/plugin
github.com/ipfs/kubo/core/node
github.com/ipfs/go-ipfs-pinner/dspinner
github.com/ipfs/go-ipfs-pinner/dspinner.test
github.com/ipfs/go-ds-badger
github.com/dgraph-io/badger
github.com/dgraph-io/ristretto/z
github.com/cespare/xxhash

After:

go mod why -m github.com/cespare/xxhash
# github.com/cespare/xxhash
github.com/ipfs-shipyard/nopfs/ipfs/plugin
github.com/ipfs/kubo/core/node
github.com/ipfs/boxo/gateway
github.com/cespare/xxhash

I think it's clearly a refactor that made core/node import gateway and that imports xxhash. So releasing and bubbling boxo should fix it.

@aschmahmann
Copy link
Contributor

@hsanjuan thanks for the clarification that the build used to succeed and then started failing in nopfs rather than if kubo was built with buildmode=plugin.

I think with things bubbled up now it should work. I've updated the dependencies in nopfs to work with kubo master in ipfs-shipyard/nopfs#1.

Going to close this issue for now and we're planning to cut v0.20 on Monday so if you get a chance to let us know if something isn't working by then reopen and we'll try and figure it out.

@hsanjuan
Copy link
Contributor Author

It seems v0.20 was cut with the wrong version of boxo (still 0.8.1). So I can't attempt to integrate with the official binaries or release. Needless to say, it's disappointing. Do I have to wait another month now?

@hsanjuan
Copy link
Contributor Author

I understood that v0.20.0 was going to include the fix: #9694 (comment)

@aschmahmann
Copy link
Contributor

aschmahmann commented May 12, 2023

Needless to say, it's disappointing ... I understood that v0.20.0 was going to include the fix

@hsanjuan sorry I messed this up. Should've been obvious that some commits needed to be cherry picked and a boxo release cut, but I missed it.

Will check with the other maintainers on if getting a patch release out soon is feasible.

@BigLep
Copy link
Contributor

BigLep commented May 17, 2023

Reopening as a signal that we need to talk about the release of this.

@BigLep
Copy link
Contributor

BigLep commented May 18, 2023

@hsanjuan : so as @aschmahmann shared, we messed up here. By default this will go out with the 0.21 release (currently scheduled for 2023-06-08). A Kubo release isn't free on our end, but we will do a 0.20.1 release on end if it impacts you to wait until 0.21. I'd obviously like to avoid doing extra work here, but we will certainly do it if this is impacting you. Please don't hesitate to say if it is. Thanks - our apologies on the delay here.

In terms of how to prevent this from happening again in the future, I have created ipfs/kuboreleaser#6, which I'm hoping we can land for 0.21.

@BigLep BigLep mentioned this issue May 23, 2023
@hsanjuan
Copy link
Contributor Author

Waiting for 0.21 is ok, thanks @BigLep !

@BigLep
Copy link
Contributor

BigLep commented Jun 8, 2023

Closing since there is a Boxo 0.9 release that has the required items and is in Kubo master. Thanks for your patience!

@BigLep BigLep closed this as completed Jun 8, 2023
@github-project-automation github-project-automation bot moved this from 🥞 Todo to 🎉 Done in IPFS Shipyard Team Jun 8, 2023
@hsanjuan
Copy link
Contributor Author

Everything works now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization
Projects
No open projects
Archived in project
Development

No branches or pull requests

4 participants