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

feat: turn dynamic lib into static #8

Merged
merged 8 commits into from
Jun 5, 2024
Merged

feat: turn dynamic lib into static #8

merged 8 commits into from
Jun 5, 2024

Conversation

noel2004
Copy link
Member

@noel2004 noel2004 commented Jun 3, 2024

This PR turn the dynamic lib (.so) for scroll's zstd into static lib.

It also include the precompiled of linux_amd64 under libzstd

zimpha
zimpha previously approved these changes Jun 4, 2024
georgehao
georgehao previously approved these changes Jun 4, 2024
@colinlyguo colinlyguo changed the title Turn dynamic lib into static feat: turn dynamic lib into static Jun 4, 2024
Copy link
Member

@0xmountaintop 0xmountaintop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. we'd better don't commit the binary to github (discussed with @colin and seems acceptable)
2. this PR is platform dependent (it only supports darwin_arm64 & linux_amd64) (what about darwin_amd64 & linux_arm64?)

@colinlyguo colinlyguo dismissed stale reviews from georgehao and zimpha via cfb92a1 June 4, 2024 06:55
@colinlyguo
Copy link
Member

colinlyguo commented Jun 4, 2024

  1. we'd better don't commit the binary to github
  2. this PR is platform dependent (it only supports darwin_arm64 & linux_amd64) (what about darwin_amd64 & linux_arm64?)

For 1, this PR uses the same approach as https://github.com/valyala/gozstd, otherwise, other repos all need to maintain the workflow to download specific .a based on system env.

Another approach is https://github.com/DataDog/zstd, it copies all c code into the repo, which is basically the same. but need to sync our customized params with libscroll_zstd, and add some random tests.

For 2, yeah we can add them.

@0xmountaintop
Copy link
Member

  1. we'd better don't commit the binary to github
  2. this PR is platform dependent (it only supports darwin_arm64 & linux_amd64) (what about darwin_amd64 & linux_arm64?)

For 1, this PR uses the same approach as https://github.com/valyala/gozstd, otherwise, other repos all need to maintain the workflow to download specific .a based on system env. For 2, yeah we can add them.

OK

@colinlyguo
Copy link
Member

colinlyguo commented Jun 4, 2024

  1. we'd better don't commit the binary to github
  2. this PR is platform dependent (it only supports darwin_arm64 & linux_amd64) (what about darwin_amd64 & linux_arm64?)

For 1, this PR uses the same approach as https://github.com/valyala/gozstd, otherwise, other repos all need to maintain the workflow to download specific .a based on system env. For 2, yeah we can add them.

OK

btw, linux_musl_arm64.a (used by Alpine) failed to build due to compile error, we can either fix the code or omit this .a file for now. relevant logs:

error[E0463]: can't find crate for `core`
  |
  = note: the `aarch64-unknown-linux-musl` target may not be installed
  = help: consider downloading the target with `rustup target add aarch64-unknown-linux-musl`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

error[E0463]: can't find crate for `compiler_builtins`

error: cannot find macro `include` in this scope
  --> /usr/local/cargo/git/checkouts/zstd-rs-b533f7596c159504/5c0892b/zstd-safe/zstd-sys/src/lib.rs:38:1
   |
38 | include!("bindings_zdict_experimental.rs");
   | ^^^^^^^

error: cannot find macro `include` in this scope
  --> /usr/local/cargo/git/checkouts/zstd-rs-b533f7596c159504/5c0892b/zstd-safe/zstd-sys/src/lib.rs:31:1
   |
31 | include!("bindings_zstd_experimental.rs");
   | ^^^^^^^

For more information about this error, try `rustc --explain E0463`.
error: could not compile `zstd-sys` (lib) due to 4 previous errors

@zimpha
Copy link
Member

zimpha commented Jun 4, 2024

  1. we'd better don't commit the binary to github
  2. this PR is platform dependent (it only supports darwin_arm64 & linux_amd64) (what about darwin_amd64 & linux_arm64?)

For 1, this PR uses the same approach as https://github.com/valyala/gozstd, otherwise, other repos all need to maintain the workflow to download specific .a based on system env. For 2, yeah we can add them.

OK

btw, linux_musl_arm64.a (used by Alpine) failed to build due to compile error, relevant logs: we can either fix the code or omit this .a file for now.

error[E0463]: can't find crate for `core`
  |
  = note: the `aarch64-unknown-linux-musl` target may not be installed
  = help: consider downloading the target with `rustup target add aarch64-unknown-linux-musl`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

error[E0463]: can't find crate for `compiler_builtins`

error: cannot find macro `include` in this scope
  --> /usr/local/cargo/git/checkouts/zstd-rs-b533f7596c159504/5c0892b/zstd-safe/zstd-sys/src/lib.rs:38:1
   |
38 | include!("bindings_zdict_experimental.rs");
   | ^^^^^^^

error: cannot find macro `include` in this scope
  --> /usr/local/cargo/git/checkouts/zstd-rs-b533f7596c159504/5c0892b/zstd-safe/zstd-sys/src/lib.rs:31:1
   |
31 | include!("bindings_zstd_experimental.rs");
   | ^^^^^^^

For more information about this error, try `rustc --explain E0463`.
error: could not compile `zstd-sys` (lib) due to 4 previous errors

did you run rustup target add aarch64-unknown-linux-musl before compile?

@colinlyguo
Copy link
Member

  1. we'd better don't commit the binary to github
  2. this PR is platform dependent (it only supports darwin_arm64 & linux_amd64) (what about darwin_amd64 & linux_arm64?)

For 1, this PR uses the same approach as https://github.com/valyala/gozstd, otherwise, other repos all need to maintain the workflow to download specific .a based on system env. For 2, yeah we can add them.

OK

btw, linux_musl_arm64.a (used by Alpine) failed to build due to compile error, relevant logs: we can either fix the code or omit this .a file for now.

error[E0463]: can't find crate for `core`
  |
  = note: the `aarch64-unknown-linux-musl` target may not be installed
  = help: consider downloading the target with `rustup target add aarch64-unknown-linux-musl`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

error[E0463]: can't find crate for `compiler_builtins`

error: cannot find macro `include` in this scope
  --> /usr/local/cargo/git/checkouts/zstd-rs-b533f7596c159504/5c0892b/zstd-safe/zstd-sys/src/lib.rs:38:1
   |
38 | include!("bindings_zdict_experimental.rs");
   | ^^^^^^^

error: cannot find macro `include` in this scope
  --> /usr/local/cargo/git/checkouts/zstd-rs-b533f7596c159504/5c0892b/zstd-safe/zstd-sys/src/lib.rs:31:1
   |
31 | include!("bindings_zstd_experimental.rs");
   | ^^^^^^^

For more information about this error, try `rustc --explain E0463`.
error: could not compile `zstd-sys` (lib) due to 4 previous errors

did you run rustup target add aarch64-unknown-linux-musl before compile?

got it. met another issue. will fix it later.

@colinlyguo
Copy link
Member

  1. we'd better don't commit the binary to github
  2. this PR is platform dependent (it only supports darwin_arm64 & linux_amd64) (what about darwin_amd64 & linux_arm64?)

For 1, this PR uses the same approach as https://github.com/valyala/gozstd, otherwise, other repos all need to maintain the workflow to download specific .a based on system env. For 2, yeah we can add them.

OK

btw, linux_musl_arm64.a (used by Alpine) failed to build due to compile error, relevant logs: we can either fix the code or omit this .a file for now.

error[E0463]: can't find crate for `core`
  |
  = note: the `aarch64-unknown-linux-musl` target may not be installed
  = help: consider downloading the target with `rustup target add aarch64-unknown-linux-musl`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

error[E0463]: can't find crate for `compiler_builtins`

error: cannot find macro `include` in this scope
  --> /usr/local/cargo/git/checkouts/zstd-rs-b533f7596c159504/5c0892b/zstd-safe/zstd-sys/src/lib.rs:38:1
   |
38 | include!("bindings_zdict_experimental.rs");
   | ^^^^^^^

error: cannot find macro `include` in this scope
  --> /usr/local/cargo/git/checkouts/zstd-rs-b533f7596c159504/5c0892b/zstd-safe/zstd-sys/src/lib.rs:31:1
   |
31 | include!("bindings_zstd_experimental.rs");
   | ^^^^^^^

For more information about this error, try `rustc --explain E0463`.
error: could not compile `zstd-sys` (lib) due to 4 previous errors

did you run rustup target add aarch64-unknown-linux-musl before compile?

got it. met another issue. will fix it later.

many other incompatible issues built by linux-musl, e.g.,

  cargo:warning=/usr/lib/gcc/aarch64-alpine-linux-musl/13.2.1/include/arm_neon.h:27891:15: error: expected declaration specifiers or '...' before 'uint64_t'
  cargo:warning=27891 | vcreate_bf16 (uint64_t __a)
  cargo:warning=      |               ^~~~~~~~
  cargo:warning=In file included from /usr/lib/gcc/aarch64-alpine-linux-musl/13.2.1/include/syslimits.h:7,
  cargo:warning=                 from /usr/lib/gcc/aarch64-alpine-linux-musl/13.2.1/include/limits.h:34,
  cargo:warning=                 from zstd/lib/legacy/../common/zstd_deps.h:27,
  cargo:warning=                 from zstd/lib/legacy/../common/error_private.h:27:
  cargo:warning=/usr/lib/gcc/aarch64-alpine-linux-musl/13.2.1/include/limits.h:205:75: error: no include path in which to search for limits.h
  cargo:warning=  205 | #include_next <limits.h>                /* recurse down to the real one */
  cargo:warning=      |                                                                           ^
  cargo:warning=zstd/lib/legacy/../common/zstd_deps.h:29:10: fatal error: string.h: No such file or directory
  cargo:warning=   29 | #include <string.h>
  cargo:warning=      |          ^~~~~~~~~~
  cargo:warning=compilation terminated.

btw, need a laptop or vm to build darwin amd64. @HAOYUatHZ

@0xmountaintop
Copy link
Member

BTW, the reason why I think it's not good to make it platform-dependent is:
I encounter this when I build scroll-tech/go-ethereum#794

ld: warning: object file (/Users/chris/go/pkg/mod/github.com/scroll-tech/da-codec@v0.0.0-20240604103336-17c1ba7dd66d/encoding/codecv2/libscroll_zstd_darwin_arm64.a[44](5eeec2f4c1e9b3b6-zstd_v07.o)) was built for newer 'macOS' version (14.4) than being linked (14.0)

@colinlyguo
Copy link
Member

BTW, the reason why I think it's not good to make it platform-dependent is: I encounter this when I build scroll-tech/go-ethereum#794

ld: warning: object file (/Users/chris/go/pkg/mod/github.com/scroll-tech/da-codec@v0.0.0-20240604103336-17c1ba7dd66d/encoding/codecv2/libscroll_zstd_darwin_arm64.a[44](5eeec2f4c1e9b3b6-zstd_v07.o)) was built for newer 'macOS' version (14.4) than being linked (14.0)

yeah. I also met this (building on the fly and running unit tests), the golang linker's platform version is older than os's version. But I think it's not the problem of this method. Using .so files and .a files would also meet this problem.

@colinlyguo colinlyguo closed this Jun 5, 2024
@0xmountaintop 0xmountaintop deleted the feat/staticlib branch June 5, 2024 03:40
@colinlyguo colinlyguo restored the feat/staticlib branch June 5, 2024 04:09
@colinlyguo
Copy link
Member

colinlyguo commented Jun 5, 2024

Previously I decided to change to use this repo (directly using C code): https://github.com/DataDog/zstd thus closing this PR.

Later @georgehao mentioned that directly using C lib will also introduce many incompatible issues, the same as .a and .so files (maybe worse since Go is more multi-platform compatible while C is not). and will make more effort to align future changes to libscroll_zstd.

The above concern makes sense, thus I think we can refine and continue this method.

will add some descriptions in readme like the link: https://github.com/valyala/gozstd?tab=readme-ov-file#faq

@colinlyguo colinlyguo reopened this Jun 5, 2024
@colinlyguo
Copy link
Member

existence and management of .a files in readme is added in 296d9c6.

@colinlyguo colinlyguo merged commit 32bfc9f into main Jun 5, 2024
3 checks passed
@colinlyguo colinlyguo deleted the feat/staticlib branch June 5, 2024 08:08
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.

5 participants