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

fix: trin cli flag displays wrong version info #1615

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

KolbyML
Copy link
Member

@KolbyML KolbyML commented Dec 26, 2024

What was wrong?

This PR supercedes #1609 and fixes #995 and #1581 (comment) . #1609 seems more like a temporary patch to get things working, but I need this information for my ping extensions PR.

I need to properly get the client version and such for my ping extensions PR so I am inclined to get fetching the version and commit properly. My PR fixes #995 which I need, #1609 seems more like a temporary fix due to cargo publish being broken, which is respectable I would do the same, I decided to implement the full solution as I needed it for my other PR

Basically when you build Trin it will report the wrong commit hash. And the shadow setup breaks publishing the ethportal-api crate.

I fixed the problem by removing shadow and using vergen instead.

I ran cargo publish --dry-run and it ran successfully so publish works with this PR

I also ran cargo run -p trin -- --version

and it works
I get trin v0.1.1-92334364 linux-x86_64 rustc1.81.0

which is the commit of the build which is good that is what we want

image

How was it fixed?

using vergen


use vergen::EmitBuilder;

fn main() -> Result<(), Box<dyn Error>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're going to either have to set a GIT_HASH env var here, or update our 2 dockerfiles so that it'll pick up the env var here

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/sigp/lighthouse/blob/stable/Dockerfile
https://github.com/paradigmxyz/reth/blob/main/Dockerfile

both Lighthouse and Reth copy the whole folder, if we do that it would resolve this issue and we could remove

trin/docker/Dockerfile

Lines 8 to 11 in ddbd9ff

# Docker build command *SHOULD* include --build-arg GIT_HASH=...
# eg. --build-arg GIT_HASH=$(git rev-parse HEAD)
ARG GIT_HASH=unknown
ENV GIT_HASH=$GIT_HASH

and passing the git hash in with the CI,

As only copying in the crates would be problematic for vergen

Reth uses this https://github.com/LukeMathWalker/cargo-chef?tab=readme-ov-file#benefits-of-cargo-chef package, which I think will resolve our caching problems we were trying to solve by copying crates individually, this should also make it so we have to update our crates less so the code is more maintainable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will make a PR updating our dockerfiles with what I said above in mind

Copy link
Member Author

Choose a reason for hiding this comment

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

#1619

Here is a PR which resolves your concern, once both of these PR's are merged it will work as expected

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

// /// The version of the programming language used to build the binary.
pub const PROGRAMMING_LANGUAGE_VERSION: &str = env!("VERGEN_RUSTC_SEMVER");

pub const VERSION: &str = const_format::formatcp!(
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
pub const VERSION: &str = const_format::formatcp!(
pub const FULL_VERSION: &str = const_format::formatcp!(

@KolbyML KolbyML merged commit 1a684e9 into ethereum:master Jan 7, 2025
9 checks passed
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.

trin --version should report the same as web3_clientVersion
3 participants