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

De-duplicate Linux builds with priority of MUSL targets #4129

Closed
wants to merge 3 commits into from

Conversation

T-256
Copy link
Contributor

@T-256 T-256 commented Jun 7, 2024

Summary

Ref #4122 (comment) musllinux targets are superset of gnu targets.

Should we document it that why we use musl builds?

Test Plan

https://github.com/T-256/uv/actions/runs/9416773652/job/25940621883

@ofek
Copy link
Contributor

ofek commented Jun 7, 2024

I think it would be nice to document!

README.md Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor

henryiii commented Jun 8, 2024

I think the wheels also need to be dual tagged. You can tell maturin to do this. --compatibility 2_17 IIRC.

Co-authored-by: Henry Schreiner <[email protected]>
@T-256
Copy link
Contributor Author

T-256 commented Jun 8, 2024

Thanks @henryiii for your review.

I think the wheels also need to be dual tagged. You can tell maturin to do this. --compatibility 2_17 IIRC.

Can you open a new PR for that? I think it's out of scope of this PR.

@charliermarsh
Copy link
Member

I’m undecided on whether I want to do this. But we should either (1) do this and omit all the non-musl release targets (this would now be considered a breaking change), (2) do this but copy the musl to the non-musl targets (sort of strange IMO), or (3) re-add the GNU dynamically linked build (but keep the dual-tagged wheel).

@charliermarsh
Copy link
Member

Are there examples of other similar tools that only ship against musl?

@ofek
Copy link
Contributor

ofek commented Jun 8, 2024

ripgrep does that but only for x86_64 https://github.com/BurntSushi/ripgrep/releases/tag/14.1.0

@BurntSushi
Copy link
Member

For ripgrep, I don't really have a consistent philosophy here. I'm pretty reactive and add things that people request (or submit), and will generally remove artifacts if they become too burdensome to maintain. With that said, if I were to clean things up, I'd probably bias toward using musl for everything where I could, and not offer anything that is dynamically linked to glibc. I don't think I've ever offered a glibc release artifact for Linux x86-64, and I don't think I've ever heard anyone complain about it. Using musl is just more convenient and doesn't really confer any downsides for ripgrep specifically. (Its allocator tends to be slower than glibc, but I mitigate that by using a different allocator. I think we do the same for uv.)

@charliermarsh
Copy link
Member

Ok, I think for now, my preference is:

  • Keep these builds.
  • Re-add the dynamically-linked GNU variant to the release pipeline.
  • ...but dual-tag our wheels, and upload the dual-tagged wheels.

@T-256
Copy link
Contributor Author

T-256 commented Jun 8, 2024

Also see https://www.tweag.io/blog/2023-08-10-rust-static-link-with-mimalloc/

(Its allocator tends to be slower than glibc, but I mitigate that by using a different allocator. I think we do the same for uv.)

Correct, according to blog post with mimalloc it'd be even faster than linux-gnu target. We currently use jemalloc.

@charliermarsh
Copy link
Member

Yeah I'm ultimately a bit torn but it doesn't really cost us much to continue shipping the dynamically-linked versions, so I have a slight preference towards doing so.

@T-256
Copy link
Contributor Author

T-256 commented Jun 12, 2024

According to #4254, this is now breaking change,
@charliermarsh feel free to close it since you agreed continuation with both static and dynamic libc releases in the pipeline.

@zanieb zanieb closed this Jun 19, 2024
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.

6 participants