-
Notifications
You must be signed in to change notification settings - Fork 756
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
[common/building] fix: fuse version should be built outside build.rs #791
Conversation
Inside `build.rs`, the env vars such as `VERGEN_BUILD_SEMVER` is written only to stdout thus it is impossible to reuse these vars to compose `FUSE_COMMIT_VERSION`. This patch fixes the issue by building `FUSE_COMMIT_VERSION` with a `lazy_static` block from env vars in runtime.
Thanks for the contribution! Please review the labels and make any necessary changes. |
@PsiACE This patch should fix your issue? |
use structopt::StructOpt; | ||
use structopt_toml::StructOptToml; | ||
|
||
pub const FUSE_COMMIT_VERSION: &str = env!("FUSE_COMMIT_VERSION"); | ||
lazy_static! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool.
The lazy looks duplicated with fusequery/query/src/configs/config.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye sir.
I left these boilerplate there because I'm not very sure about if it is all right for two binaries sharing a common version.
Moving these to a common lib would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BohuTANG
There is an issue with sharing these common codes:
The option_env
must be evaluated after build.rs
finished.
build.rs
depends on common/building
. Thus common/building
will be compiled without knowing of these env vars.
Then to share the common codes, they must be moved to another crate that is not common/building
.
For now, no other common
crate is suitable for this task.
Thus we have to:
- Let
fusestore
referencefusequery::*::FUSE_COMMIT_VERSION
or reverse. - or create another crate to keep the common codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got
Codecov Report
@@ Coverage Diff @@
## master #791 +/- ##
======================================
Coverage 77% 78%
======================================
Files 317 318 +1
Lines 17416 17426 +10
======================================
+ Hits 13579 13593 +14
+ Misses 3837 3833 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CI Passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @drmingdrmer
I hereby agree to the terms of the CLA available at: https://datafuse.rs/policies/cla/
Summary
[common/building] fix: fuse version should be built outside build.rs
Inside
build.rs
, the env vars such asVERGEN_BUILD_SEMVER
is writtenonly to stdout thus it is impossible to reuse these vars to compose
FUSE_COMMIT_VERSION
.This patch fixes the issue by building
FUSE_COMMIT_VERSION
with alazy_static
block from env vars in runtime.Changelog
Related Issues
#782 (comment)