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: fallback to build protobuf from source #67

Merged
merged 7 commits into from
Jul 1, 2023

Conversation

tisonkun
Copy link
Contributor

No description provided.

1. Avoid bundle binaries
2. Leverage C++ portability

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun requested review from BusyJay and hicqu June 19, 2023 16:33
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@BusyJay
Copy link
Member

BusyJay commented Jun 20, 2023

I'm afraid building from source would require extra dependencies like C/C++ compilers.

@tisonkun
Copy link
Contributor Author

I'm afraid building from source would require extra dependencies like C/C++ compilers.

This patch include three changes:

  1. Support new protoc version string.
  2. Prost build respects bundled protoc
  3. Using protobuf-src over binaries to support macos aarch64 fallback

If you'd prefer add macos aarch64 binary to resolve 3 and agree on the changes for 1 and 2, I can find time to restore and update the binaries.

Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

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

I believe protoc-osx-aarch64 was omitted because it exceeded the size limit (10MiB at that time) of a crate that can be published to crates.io. Better check if the problem still exists.

src/lib.rs Outdated
let caps = ver_re.captures(from_utf8(&o.stdout).unwrap()).unwrap();
let major = caps.get(1).unwrap().as_str().parse::<i16>().unwrap();
let minor = caps.get(2).unwrap().as_str().parse::<i16>().unwrap();
if (major == 3 && minor >= 1) || (major >= 20) {
Copy link
Member

Choose a reason for hiding this comment

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

How about (major, minor) >= (3, 1)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the link in commit c1462c6.

I tried to follow protobuf's version policy that they change the version pattern only after 20.x. But it should be physically OK to use (major, minor) >= (3, 1).

@BusyJay
Copy link
Member

BusyJay commented Jun 20, 2023

Note protobuf-src may not work for Windows.

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 20, 2023

Note protobuf-src may not work for Windows.

OK. Then I'll try the restore and update binaries way. Perhaps I can run cargo publish --dry-run locally to check the limitation but I'm not sure if dry-run requires push permission to the crate.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

@BusyJay Updated. I use 23.3 protoc and check it works well on osx-aarch_64 env.

@tisonkun tisonkun requested a review from BusyJay June 20, 2023 18:07
@BusyJay
Copy link
Member

BusyJay commented Jun 21, 2023

The resulting package is clearly larger than 10MiB, I'm afraid this change can't be published to crates.io.

@tisonkun
Copy link
Contributor Author

cargo publish --dry-run
    Updating crates.io index
   Packaging protobuf-build v0.14.1 (/Users/tison/Brittani/protobuf-build)
   Verifying protobuf-build v0.14.1 (/Users/tison/Brittani/protobuf-build)
    Updating crates.io index
   Compiling memchr v2.5.0
   Compiling protobuf v2.28.0
   Compiling bytes v1.4.0
   Compiling regex-syntax v0.7.2
   Compiling bitflags v1.3.2
   Compiling aho-corasick v1.0.2
   Compiling regex v1.8.4
   Compiling protobuf-codegen v2.28.0
   Compiling protobuf-build v0.14.1 (/Users/tison/Brittani/protobuf-build/target/package/protobuf-build-0.14.1)
    Finished dev [unoptimized + debuginfo] target(s) in 13.22s
    Packaged 31 files, 53.8MiB (17.7MiB compressed)
   Uploading protobuf-build v0.14.1 (/Users/tison/Brittani/protobuf-build)
warning: aborting upload due to dry run

It seems dry-run doesn't test against it.

cc @nrc could you please take a look and check the cargo limitation?

cc @weihanglo

@weihanglo
Copy link

crates.io has its own stricter limitations that Cargo doesn't check at this time. Part of the reason Cargo has less checks because Cargo is not only for crates.io. We would like to see a new publish check API but the work is not on priority so far I can tell.

The 10MiB limit is documented here.

See also

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 21, 2023

@BusyJay Perhaps we drop the support to linux-ppcle_64 and linux-x86_32 (or perhaps linux-aarch_64, I don't know) as they are least widely used? And if we can add a feature flag to support users to fallback to protobuf-src on their demand.

-rwxr-xr-x@  1 tison  staff   7.8M Jan  1  1980 protoc-linux-aarch_64
-rwxr-xr-x@  1 tison  staff   9.1M Jan  1  1980 protoc-linux-ppcle_64
-rwxr-xr-x@  1 tison  staff   8.5M Jan  1  1980 protoc-linux-x86_32
-rwxr-xr-x@  1 tison  staff   7.8M Jan  1  1980 protoc-linux-x86_64
-rwxr-xr-x@  1 tison  staff   6.1M Jun 14 23:12 protoc-osx-aarch_64
-rwxr-xr-x@  1 tison  staff   5.6M Jun 14 23:12 protoc-osx-x86_64
-rwxr-xr-x@  1 tison  staff   8.8M Jan  1  1980 protoc-win32.exe

-rw-r--r--  1 tison  staff    18M Jun 21 12:56 target/package/protobuf-build-0.14.1.crate

@tisonkun
Copy link
Contributor Author

@weihanglo ... and Thank you!

@BusyJay
Copy link
Member

BusyJay commented Jun 24, 2023

I think fallback to build from source may not be a bad idea giving that the community prefers not to use bundled binaries (like prost-build recently did). But we still need to think a way to support Windows.

@BusyJay
Copy link
Member

BusyJay commented Jun 24, 2023

linux-ppcle_64 and linux-aarch_64 are mainly used on server side.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

@BusyJay Updated. As we discussed offline -

  1. Bundle win only;
  2. Others compile from source.

@tisonkun
Copy link
Contributor Author

linux-ppcle_64 and linux-aarch_64 are mainly used on server side.

Yeah. I suspect on server side they're used less, while aarch_64 can be a trend :D

Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

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

Rest LGTM

src/lib.rs Show resolved Hide resolved
Signed-off-by: tison <wander4096@gmail.com>
Copy link
Member

@sunxiaoguang sunxiaoguang left a comment

Choose a reason for hiding this comment

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

LGTM

@sunxiaoguang sunxiaoguang merged commit d4c4831 into master Jul 1, 2023
3 checks passed
@sunxiaoguang sunxiaoguang deleted the use-protobuf-src branch July 1, 2023 11:11
@tisonkun
Copy link
Contributor Author

tisonkun commented Jul 1, 2023

Maybe we can release a 0.15.0 now and upgrade downsteams like raft-rs and tikv.

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