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

Support 386 arch #146

Closed
wolfogre opened this issue Aug 2, 2024 · 7 comments · Fixed by #147
Closed

Support 386 arch #146

wolfogre opened this issue Aug 2, 2024 · 7 comments · Fixed by #147

Comments

@wolfogre
Copy link
Contributor

wolfogre commented Aug 2, 2024

Hi maintainers! Thanks for this awesome package.

I'm working on introducing zstd seekable format to Gitea, see go-gitea/gitea#31761.

Unfortunately, it doesn't support 386 arch.

image

I checked the code, it seems that it's just some Golang syntax issues and shouldn't be difficult to fix.

I will try to send a PR to fix it, and I wonder if it's possible to release a new version when it has been fixed? I will be very grateful.

@SaveTheRbtz
Copy link
Owner

Thank you for the contribution, the release will be coming shortly

@wolfogre
Copy link
Contributor Author

wolfogre commented Aug 3, 2024

@SaveTheRbtz Thanks! However, I noticed that the new tag v0.7.1 doesn't take effect on '/pkg' since there's a tag pkg/v0.7.0. Golang will use the last one when go get github.com/SaveTheRbtz/zstd-seekable-format-go/pkg.

@wolfogre
Copy link
Contributor Author

wolfogre commented Aug 3, 2024

Hmm, it seems that it is 8dcabb2 which make things difficult. It's unnecessary to split it into two modules even you want a cmd tool. Maybe you can follow a better project layout, wire can be a example.

@SaveTheRbtz
Copy link
Owner

SaveTheRbtz commented Aug 3, 2024

@wolfogre the goal was to avoid leaking cmd's dependencies into the lib, e.g. progressbar, xterm, and cdc.

Going through Go's module reference, seems like tagging all new releases with pkg/v0.x.x should be enough to make it work:

$ go list -m -versions github.com/SaveTheRbtz/zstd-seekable-format-go/pkg
github.com/SaveTheRbtz/zstd-seekable-format-go/pkg v0.7.0 v0.7.1 v0.7.2

Can you please re-verify if it works for your use-case:

$ go get github.com/SaveTheRbtz/zstd-seekable-format-go/pkg@v0.7.2

@SaveTheRbtz SaveTheRbtz reopened this Aug 3, 2024
@wolfogre
Copy link
Contributor Author

wolfogre commented Aug 3, 2024

@SaveTheRbtz

the goal was to avoid leaking cmd's dependencies into the lib, e.g. progressbar, xterm, and cdc.

I understand. But it's still unnecessary to split it into two modules, you can add a standalone go.mod for cmd. Like:

  • go.mod
  • cmd
    • zstdseek
      • go.mod

Let me show you another example: kratos

You can find there are both /go.mod and /cmd/kratos/go.mod. /cmd/kratos/go.mod constains github.com/fatih/color while /go.mod doesn't. And kratos has clean tags.

Going through Go's module reference, seems like tagging all new releases with pkg/v0.x.x should be enough to make it work

Yes, but it's unnecessary, since you are not putting multiple packages into one repo.

I have ample experience with golang projects. If you are willing, I can provide more help regarding golang. You can find my email address in my commit history. 😁

Can you please re-verify if it works for your use-case

Thanks! It works now.

@SaveTheRbtz
Copy link
Owner

Thanks for good examples! I come from a monorepo world where top-level module does not make much sense. Let's keep package structure as is for now and see how it goes when more packages are added. I wanted to eventually add an S3/GCP/Azure-based remote envs and maybe even content-addressable remote backend.

If it doesn't look good, we can always switch to a top-level package before the v1 release.

@wolfogre
Copy link
Contributor Author

wolfogre commented Aug 4, 2024

I completely understand. Anyway, thank you very much for this package!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants