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: Benchy test without git code cannot output result #1688

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

lovel8
Copy link
Contributor

@lovel8 lovel8 commented Mar 30, 2023

Description: When the benchy program is generated on the compilation server and copied to a non-git code server for testing, the report result cannot be output.

Copy link
Contributor

@vmx vmx left a comment

Choose a reason for hiding this comment

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

What about making the Git metadata optional at

.

It could be changed to Option<GitMetadata> and if retrieving the data fails, it would just be None.

@lovel8
Copy link
Contributor Author

lovel8 commented Mar 31, 2023

What about making the Git metadata optional at

.
It could be changed to Option<GitMetadata> and if retrieving the data fails, it would just be None.

If the git information is empty, the purpose of the code corresponding to this parameter will not be achieved.
Using precompilation to build git information into the program in advance feels like a good choice ( this modify PR)

@vmx
Copy link
Contributor

vmx commented Apr 3, 2023

If the git information is empty, the purpose of the code corresponding to this parameter will not be achieved.
Using precompilation to build git information into the program in advance feels like a good choice ( this modify PR)

Thanks for the explanation. I only though about the case where you have something like a source tarball without a .git directory where you also kind of know the version as you compile it. I haven't thought of compiled binaries.

fil-proofs-tooling/Cargo.toml Outdated Show resolved Hide resolved
@lovel8 lovel8 force-pushed the debug_git_log_inner branch 2 times, most recently from 3643141 to 166d1b8 Compare April 4, 2023 02:29
fil-proofs-tooling/src/metadata.rs Outdated Show resolved Hide resolved
fil-proofs-tooling/build.rs Outdated Show resolved Hide resolved
fil-proofs-tooling/Cargo.toml Outdated Show resolved Hide resolved
fil-proofs-tooling/Cargo.toml Show resolved Hide resolved
@lovel8 lovel8 force-pushed the debug_git_log_inner branch 3 times, most recently from 2661b1b to 320d5b2 Compare April 6, 2023 01:21
Copy link
Contributor

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Thanks! Things look good. Once you've removed the benchy binary from the commit, it's good to go.

benchy Outdated Show resolved Hide resolved
@vmx vmx merged commit e69f234 into filecoin-project:master Apr 11, 2023
@vmx vmx mentioned this pull request May 26, 2023
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.

2 participants