Fix bundling process #275

josiasbruderer · 2023-11-07T16:14:05Z

This fixes bundling process that was broken with a2884d2: see the issue regarding to bundling issue.

welcome · 2023-11-07T16:14:07Z

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

The context for this PR is clear, with relevant discussion, decisions
and stakeholders linked/mentioned.
Your contribution itself is clear (code comments, self-review for the
rest) and in its best form. Follow the code contribution
guidelines
if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

A maintainer will triage and assign priority to this PR, commenting on
any missing things and potentially assigning a reviewer for high
priority items.
The PR gets reviews, discussed and approvals as needed.
The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

Jorropo

Thx tackling this, I have a few questions.

README.md

Jorropo · 2023-11-07T16:33:58Z

src/s3ds.go

@@ -1,4 +1,4 @@
-package main
+package plugin



src means nothing can you move it to ./plugin ?

Sure! Thanks for your advice! It's done in: b8cf0e9

Jorropo · 2023-11-07T16:34:17Z

go-ds-s3-plugin/main.go

+
+var Plugins = plugin.Plugins //nolint
+
+func main() {


This is suspicious, s3ds.go doesn't needed to do this, why does this needs it ?

tl;dr: Line 7 is needed, lines 9-11 can be removed. See 06217e6

Thanks for questioning this! I actually copied main.go from v0.9.0 and tried to understand why it was written like this:

Importing the plugin without using it will give GO error. So line 7 was to suppress import error ("nolint"). I tried importing with blank identifier (underscore) as explicit package name. This worked but caused another error:

Using plugin unbundled (like .ipfs/plugins/go-ds-s3-plugin) will fail when importing with blank identifier as described above. Kubo (f.E. ipfs version) thows error: plugin: symbol Plugins not found in plugin.

Jorropo · 2023-11-07T16:35:35Z

docker/Dockerfile

-RUN go get github.com/ipfs/go-ds-s3/plugin@latest
-RUN echo "\ns3ds github.com/ipfs/go-ds-s3/plugin 0" >> plugin/loader/preload_list
+RUN go get github.com/ipfs/go-ds-s3/go-ds-s3-plugin@latest
+RUN echo "\ns3ds github.com/ipfs/go-ds-s3/go-ds-s3-plugin 0" >> plugin/loader/preload_list


Is that even working ? I thought it tried it and it refused to import it because it was main.
It is still main, what I expect to happen is github.com/ipfs/go-ds-s3/plugin would be imported.

If this works then no .go file need to be changed.

Tbh I just modified the path the same as I did in README.md
Creating the Docker Image is broken anyway because docker image ipfs/kubo v0.19.0 was not compatible with latest ipfs version (kubo/bin/container_daemon dependencies changed).

Following commit will fix docker image: b37ea16

For long term: I guess docker image is to be thought through because ipfs version is hard coded and parameter are unclear and incomplete (f.E. regionEndpoint is missing). But imo this is to be done in a separate PR. Let me know if you share this opinion.

Jorropo · 2023-11-07T16:35:48Z

README.md

 # Clone kubo.
 > git clone https://github.com/ipfs/kubo
 > cd kubo

 # Pull in the datastore plugin (you can specify a version other than latest if you'd like).
-> go get github.com/ipfs/go-ds-s3/plugin@latest
+> go get github.com/ipfs/go-ds-s3/go-ds-s3-plugin@latest


Suggested change

> go get github.com/ipfs/go-ds-s3/go-ds-s3-plugin@latest

> go get github.com/ipfs/go-ds-s3@latest

Applied in 80ac067

Jorropo · 2023-11-07T16:35:58Z

README.md


 # Add the plugin to the preload list.
-> echo -en "\ns3ds github.com/ipfs/go-ds-s3/plugin 0" >> plugin/loader/preload_list
+> echo -en "\ns3ds github.com/ipfs/go-ds-s3/go-ds-s3-plugin 0" >> plugin/loader/preload_list


Same comment about import path here.

Removing "/go-ds-s3-plugin" doesn't work here. Build error: plugin/loader/preload.go:28:21: undefined: plugins3ds.Plugins
I assume GO needs the specific path.

Setting it to "github.com/ipfs/go-ds-s3/plugin" doesn't work as well.
-> I only manage to get it work with "github.com/ipfs/go-ds-s3/go-ds-s3-plugin".

Your PR hasn't been merged nor released, so it can't be picked up automatically by go mod, did you tried adding a temporary replace github.com/ipfs/go-ds-s3 => ../go-ds-s3 to your Kubo go.mod file ? 🙂

Yes I'm aware of that. I'm testing on a fork so I can mess around with master branch.
Temporary adding replace github.com/ipfs/go-ds-s3 => ../go-ds-s3 doesn't change the behavior described above.

I merge and this can be fixed in upcoming PR if still doesn't work.

Co-authored-by: Jorropo <[email protected]>

hsanjuan · 2023-11-08T12:45:35Z

Moving things around is probably breaking https://github.com/ipfs/go-ds-s3/blob/master/.github/workflows/release.yml

josiasbruderer · 2023-11-08T14:22:40Z

Moving things around is probably breaking https://github.com/ipfs/go-ds-s3/blob/master/.github/workflows/release.yml

@hsanjuan workflows won't be broken since path of plugin stays ./go-ds-s3-plugin. I also tested this in my fork just now: https://github.com/chixodo-xyz/go-ds-s3/actions/runs/6799364118 which produced the expected release.

hsanjuan · 2023-11-22T11:23:08Z

Thank you very much @josiasbruderer

fix bundling process

2c56540

josiasbruderer mentioned this pull request Nov 7, 2023

Bundling not working since v0.10.0 #273

Closed

Jorropo reviewed Nov 7, 2023

View reviewed changes

josiasbruderer and others added 5 commits November 8, 2023 05:05

change plugin path

b8cf0e9

simplify main.go

06217e6

simplify bundling instructions

80ac067

fix docker image

b37ea16

modify instruction for bundling about go cache cleaning

6b699b0

Co-authored-by: Jorropo <[email protected]>

hsanjuan merged commit 01d7975 into ipfs:master Nov 22, 2023
9 checks passed

josiasbruderer deleted the v0.11.0_contrib branch November 23, 2023 12:18

josiasbruderer mentioned this pull request Nov 23, 2023

Release v0.11.0 #277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bundling process #275

Fix bundling process #275

josiasbruderer commented Nov 7, 2023

welcome bot commented Nov 7, 2023

Jorropo left a comment

Jorropo Nov 7, 2023

josiasbruderer Nov 8, 2023

Jorropo Nov 7, 2023

josiasbruderer Nov 8, 2023

Jorropo Nov 7, 2023 •

edited

Loading

josiasbruderer Nov 8, 2023

Jorropo Nov 7, 2023

josiasbruderer Nov 8, 2023

Jorropo Nov 7, 2023

josiasbruderer Nov 8, 2023

josiasbruderer Nov 8, 2023

Jorropo Nov 8, 2023

josiasbruderer Nov 8, 2023

hsanjuan Nov 22, 2023

hsanjuan commented Nov 8, 2023

josiasbruderer commented Nov 8, 2023

hsanjuan commented Nov 22, 2023

	> go get github.com/ipfs/go-ds-s3/go-ds-s3-plugin@latest
	> go get github.com/ipfs/go-ds-s3@latest

Fix bundling process #275

Fix bundling process #275

Conversation

josiasbruderer commented Nov 7, 2023

welcome bot commented Nov 7, 2023

Jorropo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorropo Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsanjuan commented Nov 8, 2023

josiasbruderer commented Nov 8, 2023

hsanjuan commented Nov 22, 2023

Jorropo Nov 7, 2023 •

edited

Loading