Add crate version and commit hash to ldk-server#229
Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
| let (shutdown_tx, shutdown_rx) = tokio::sync::watch::channel(false); | ||
|
|
||
| info!("Starting up..."); | ||
| info!("Starting ldk-server version {FULL_VERSION}"); |
There was a problem hiding this comment.
Should we also do this when we rotate the logs ?
|
|
||
| fn main() { | ||
| println!("cargo:rerun-if-changed=build.rs"); | ||
| println!("cargo:rerun-if-env-changed=GIT_HASH"); |
There was a problem hiding this comment.
Can we make sure to update GIT_HASH if HEAD changes after the first from-clean build. Here it seems HEAD could change, and we'd ship a binary with a wrong rev ?
Or perhaps cargo install always re-runs build.rs so we might be good here.
There was a problem hiding this comment.
Also trying to understand how this triggers. After build.rs sets GIT_HASH, what other thing could cause GIT_HASH to change, and trigger build.rs again ?
There was a problem hiding this comment.
Made it so it watches the git hash file so if it changes then we recalc
benthecarman
force-pushed
the
commit-hash
branch
from
9de46b4 to
5d6e906
Compare
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
vibed with codex, pushed the suggestions in a commit, let me know what you think @benthecarman
The findings:
_ Findings
1. High: inherited GIT_HASH overrides the real repository hash in both build scripts: ldk-server/build.rs:33, ldk-server-cli/build.rs:33. Your report is correct. If the shell exports GIT_HASH, the script skips cargo:rustc-env, so env!("GIT_HASH") compiles the caller_s stale value. I reproduced:
GIT_HASH=stale-from-env cargo run -q -p ldk-server-cli -- --version -> ldk-server-cli 0.1.0 (stale-from-env), and same for ldk-server.
Suggested fix: always compute from git rev-parse HEAD when git is available, then emit cargo:rustc-env=GIT_HASH=.... Only fall back to the environment when git is unavailable, or use a package-specific override variable.
2. Medium: packed branch refs can also leave the hash stale: ldk-server/build.rs:28, ldk-server-cli/build.rs:28, ldk-server/build.rs:54. If the current branch exists only in .git/packed-refs, watch_if_exists skips .git/refs/heads/<branch>. The next commit creates that loose ref, but HEAD and
packed-refs do not change, so Cargo does not rerun the build script. I reproduced this in a throwaway clone: after an empty commit advanced HEAD from 5d6e906... to 80648b9..., ldk-server-cli --version still printed 5d6e906....
Suggested fix: print cargo:rerun-if-changed for the symbolic ref path even when it does not exist yet. I validated that Cargo then reruns when Git creates the file.
Checks run: cargo fmt --all -- --check, cargo check -p ldk-server, cargo check -p ldk-server-cli, configured cargo clippy --all-features -- -D warnings -A clippy::drop_non_drop, cargo test -p ldk-server-cli, and cargo test -p ldk-server all passed.
8e8e2ca to
079d5f9
Compare
|
@tankyleo thanks I like those changes. Squashed them into my commits and added you as co-author |
benthecarman
force-pushed
the
commit-hash
branch
from
079d5f9 to
cdff07e
Compare
We will now log the version number and commit hash when starting up. Also will give the full commit hash when doing ldk-server --version to better help debugging things. Co-authored-by: Leo Nash <hello@leonash.net>
Co-authored-by: Leo Nash <hello@leonash.net>
benthecarman
force-pushed
the
commit-hash
branch
from
cdff07e to
f92e959
Compare
3 participants
We will now log the version number and commit hash when starting up. Also will give the full commit hash when doing ldk-server --version to better help debugging things.
Also does the same to the cli