Skip to content
This repository has been archived by the owner on Jan 11, 2021. It is now read-only.

Fix build error when fetching git hash #171

Merged
merged 1 commit into from
Oct 11, 2018
Merged

Fix build error when fetching git hash #171

merged 1 commit into from
Oct 11, 2018

Conversation

sunchao
Copy link
Owner

@sunchao sunchao commented Oct 11, 2018

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

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 <version-no>" as the CREATED_BY
string. This should be OK since a crate version should always map to
a unique git hash.
@sunchao sunchao requested a review from sadikovi October 11, 2018 01:45
@coveralls
Copy link

coveralls commented Oct 11, 2018

Pull Request Test Coverage Report for Build 636

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.535%

Totals Coverage Status
Change from base Build 634: 0.0%
Covered Lines: 12409
Relevant Lines: 12989

💛 - Coveralls

Copy link
Collaborator

@sadikovi sadikovi left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the problem. I should have accounted for it.

We can merge this PR and patch the code ASAP, but this was done to make parquet version available in the code, which has a format "parquet x.y.z (build ###)". I suggest we merge the PR and I will work on how we update version in the follow-up.

@sadikovi
Copy link
Collaborator

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

@sunchao
Copy link
Owner Author

sunchao commented Oct 11, 2018

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?

@sunchao sunchao merged commit dcc6e73 into master Oct 11, 2018
@sunchao sunchao deleted the fix-build-rs branch October 11, 2018 07:46
@sadikovi
Copy link
Collaborator

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
Copy link
Owner Author

sunchao commented Oct 11, 2018

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

@sadikovi
Copy link
Collaborator

sadikovi commented Oct 11, 2018 via email

@sunchao
Copy link
Owner Author

sunchao commented Oct 12, 2018

@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
Copy link
Collaborator

All good, thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Depending on parquet 0.4.0 from crates errors with git command failure
3 participants