Skip to content

readme: mention Alpine package #427

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

Merged
merged 2 commits into from
Jul 9, 2024
Merged

readme: mention Alpine package #427

merged 2 commits into from
Jul 9, 2024

Conversation

WhyNotHugo
Copy link
Contributor

No description provided.

@WhyNotHugo
Copy link
Contributor Author

WhyNotHugo commented Jul 7, 2024

BTW: the Makefile claims that Alpine requires statically linked libraries:

zk/Makefile

Lines 82 to 86 in 00a4361

# Alpine (musl) requires statically linked libs. This should be compatible for
# Void linux and other musl based distros aswell.
define alpine
$(ENV_PREFIX) go $(1) -tags "fts5" -ldflags "-extldflags=-static -X=main.Version=$(VERSION) -X=main.Build=$(BUILD)" $(2)
endef

This is not correct, and most packages (including zk itself) are dynamically linked.

@tjex
Copy link
Member

tjex commented Jul 9, 2024

Our regular builds link to glibc, which is not compatible with Alpine and other musl based systems.
Therefore the builds for Alpine need to have the c lib statically linked.

@tjex tjex closed this Jul 9, 2024
@WhyNotHugo
Copy link
Contributor Author

Any comments on why you closed this? Did I do something wrong? Are my contributions unwelcome?

@WhyNotHugo
Copy link
Contributor Author

Our regular builds link to glibc, which is not compatible with Alpine and other musl based systems.
Therefore the builds for Alpine need to have the c lib statically linked.

The builds for Alpine do not need to have the libc statically linked, they merely need to link to musl instead of glibc.

As a clear example of this, zk installed from Alpine packages dynamically links libc:

> ldd =zk
	/lib/ld-musl-x86_64.so.1 (0x7f4e50bf8000)
	libc.musl-x86_64.so.1 => /lib/ld-musl-x86_64.so.1 (0x7f4e50bf8000)

These are built using make build: https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/testing/zk/APKBUILD#L21-23

I'm not sure where you heard this, but AFAIK, Alpine has no policy to statically link binaries. All the packages I've checked are dynamically linked.

@tjex
Copy link
Member

tjex commented Jul 9, 2024

Any comments on why you closed this? Did I do something wrong? Are my contributions unwelcome?

The comment was above the closed event:

Our regular builds link to glibc, which is not compatible with Alpine and other musl based systems.
Therefore the builds for Alpine need to have the c lib statically linked.

The solution to statically link was brought up by another Alpine user: #411

I'm personally not so experienced with the intricacies of cross compilation / system architectures. In that pr above, I did have a dynamically linked binary, but it was deemed better to statically link. From memory something to do with cross compiling with Docker and the go-sqlite3 go package, which requires CGO.

If there is an argument to not do this and there is an argument why this shouldn't be done, I'm happy to hear. Because as far as I'm aware, it works fine and offering the binaries from the release section of GitHub is simply another option for musl users to get zk. But I doubt it'll be many people's go to solution.

The aports image will of course not need static linking because it's built directly on a musl based system.

Your PR's / comments are welcome. It may be that you've just missinterpreted the comment in the Makefile and made a PR before checking the context. I'm not saying (because I wrote that comment in the Makefile), that Alpine requires statically linked binaries in general, but that for our specific case (because of above) we need to statically link the c libs.

@WhyNotHugo
Copy link
Contributor Author

My first comment was only vaguely related to the PR; the PR itself only adds the Alpine package to the list of downstream packages.

@WhyNotHugo
Copy link
Contributor Author

I understand the confusion now; I guess I shouldn't mention another topic if the PR itself doesn't have a description 😅

@tjex tjex reopened this Jul 9, 2024
@tjex
Copy link
Member

tjex commented Jul 9, 2024

Oh wow... Yeah, that was really confusing! I thought the code comment was the crux of the PR..
But honestly, I'm at fault. I've barely had any sleep last night and should know better to go through the regular process of a good code review (check what's actually being changed... And not base it off just the comment / discussion).

It was totally fine that you made a side mention about the Makefile. It was my fault for not seeing it as a separate context 👍

Thanks for the PRs 🫠

@WhyNotHugo
Copy link
Contributor Author

No problem. It takes two to mis-communicate anyway :)

@tjex tjex merged commit 739d2d4 into zk-org:main Jul 9, 2024
3 checks passed
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 this pull request may close these issues.

2 participants