search

sunchao / parquet-rs

Apache Parquet implementation in Rust
Apache License 2.0
149 stars 20 forks source link

Fix build error when fetching git hash #171

Closed sunchao closed 6 years ago

sunchao commented 6 years ago

When the parquet-rs crate is used in a third-party library, the "git" command in the build.rs will fail. This fixes it by skipping the git hash and just use "parquet-rs version " as the CREATED_BY string. This should be OK since a crate version should always map to a unique git hash.

Fixes #170

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 636

Totals Coverage Status
Change from base Build 634: 0.0%
Covered Lines: 12409
Relevant Lines: 12989
đź’› - Coveralls
sadikovi commented 6 years ago

One of the ways of fixing it is having a script that updates the hardcodes the version during the release process.

sunchao commented 6 years ago

Sure @sadikovi . Thanks.

One of the ways of fixing it is having a script that updates the hardcodes the version during the release process.

What's the issue with the current approach? we are getting the version from CARGO_PKG_VERSION. Is that not the right approach?

sadikovi commented 6 years ago

Hello. You are correct - it is the right approach, but parquet version is generally “parquet x.y.z (build abc123)”. Having just the version is correct, though it is not full version, and there could be code written in other processing engine that relies on it. Your fix is fast and on point, thank you for fixing it. But long term we should add something that injects full version of parquet - I will try coming up with the least painful for development and release approach.

sunchao commented 6 years ago

I see. You mean we should still keep the (build abc123) part for compatibility reason, right? That makes sense. Thanks!

sadikovi commented 6 years ago

It is not necessary, but this is what cpp and mr do. We will fix it later. On Thu, 11 Oct 2018 at 10:07 AM, Chao Sun notifications@github.com wrote:

I see. You mean we should still keep the (build abc123) part for compatibility reason, right? That makes sense. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sunchao/parquet-rs/pull/171#issuecomment-428860833, or mute the thread https://github.com/notifications/unsubscribe-auth/AHbY3q80hM5XZZnnrSCkj3sk82igNt_Yks5ujvwpgaJpZM4XWn3h .

sunchao commented 6 years ago

@sadikovi : I'm going to release 0.4.1 with this fix. After you have a more complete fix we can do another minor release. Is that OK?

sadikovi commented 6 years ago

All good, thanks.