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.

Sign up for GitHub

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

Jump to bottom

Add t8n full version printing #612

Merged
merged 2 commits into from
Apr 20, 2023
Merged

Add t8n full version printing #612

merged 2 commits into from
Apr 20, 2023

Conversation

rodiazet
Copy link
Collaborator

@rodiazet rodiazet commented Apr 5, 2023

No description provided.

@rodiazet rodiazet requested a review from chfast April 5, 2023 16:19
@codecov
Copy link

codecov bot commented Apr 5, 2023 •
edited
Loading

Codecov Report

Merging #612 (990ef97) into master (31fd534) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #612      +/-   ##
==========================================
- Coverage   97.29%   97.28%   -0.02%     
==========================================
  Files          78       78              
  Lines        7648     7648              
==========================================
- Hits         7441     7440       -1     
- Misses        207      208       +1
Flag Coverage Δ
blockchaintests 65.16% <ø> (ø)
statetests 63.18% <ø> (ø)
unittests 94.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@rodiazet rodiazet force-pushed the evmone-t8n-version branch 2 times, most recently from 3967cf0 to 68e8278 Compare April 14, 2023 09:56
@rodiazet rodiazet changed the title Add GIT_REV to project version Add t8n full version printing Apr 14, 2023
axic
axic reviewed Apr 14, 2023
View reviewed changes
test/t8n/t8n.cpp
@@ -39,7 +40,7 @@ int main(int argc, const char* argv[])

if (arg == "-v")
{
std::cout << "evmone-t8n " PROJECT_VERSION "\n";
std::cout << "evmone-t8n " EVMONE_VERSION "\n";
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference in output?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PROJECT_VERSION is not defined any longer.
No we print use evmone-t8n 0.10.0-dev+commit.f8f94f51 previously it was evmone-t8n 0.10.0-dev

axic
axic reviewed Apr 14, 2023
View reviewed changes
cmake/cable/buildinfo/buildinfo.c.in
@@ -7,7 +7,7 @@

#include "buildinfo.h"

const struct buildinfo* @FUNCTION_NAME@()
const struct buildinfo* @FUNCTION_NAME@(void)
Copy link
Member

Choose a reason for hiding this comment

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

Why are these needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

/evmone/build/test/t8n/evmone_t8n/buildinfo.h:29:49: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
const struct buildinfo* evmone_t8n_get_buildinfo();
                                                ^
                                                 void

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but why wasn't this a problem before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's enabled by default since clang 15.
https://releases.llvm.org/14.0.0/tools/clang/docs/DiagnosticsReference.html#wstrict-prototypes
https://releases.llvm.org/15.0.0/tools/clang/docs/DiagnosticsReference.html#wstrict-prototypes

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but why is master working?

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you need (void) in the function definition?

FYI this is related to C23 where the meaning of declaration of int f() changes from int f(...) to int f(void). But I think they overdone the warning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So for clang 16.0.0 I need but for appleclang 14.0.0.14000029I don't need. I can stop using newer compiler and we don't need to change anything.

Copy link
Collaborator Author

@rodiazet rodiazet Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

but it looks that it's not only my problem ;) https://app.circleci.com/pipelines/github/ethereum/evmone/4335/workflows/d5b6d7cf-9826-45b1-8586-a9366b89a908/jobs/47373
Ci uses clang 15 which enables this warning by default.

Copy link
Collaborator Author

@rodiazet rodiazet Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think i tested definition too, but will check again on CI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also the same error.
https://app.circleci.com/pipelines/github/ethereum/evmone/4336/workflows/2a430d17-fbc4-428c-ac0e-80aa38298ef5/jobs/47384

chfast
chfast reviewed Apr 18, 2023
View reviewed changes
cmake/cable/buildinfo/buildinfo.c.in
@@ -7,7 +7,7 @@

#include "buildinfo.h"

const struct buildinfo* @FUNCTION_NAME@()
const struct buildinfo* @FUNCTION_NAME@(void)
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you need (void) in the function definition?

FYI this is related to C23 where the meaning of declaration of int f() changes from int f(...) to int f(void). But I think they overdone the warning.

cmake/cable/buildinfo/buildinfo.h.in
@@ -26,7 +26,7 @@ struct buildinfo
const char* build_type;
};

const struct buildinfo* @FUNCTION_NAME@();
const struct buildinfo* @FUNCTION_NAME@(void);
Copy link
Member

Choose a reason for hiding this comment

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

Put changes to cable in separate commit and possibly add a CHANGELOG entry in the cmake file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@rodiazet rodiazet requested a review from chfast April 19, 2023 12:04
chfast
chfast approved these changes Apr 20, 2023
View reviewed changes
cmake/cable/CableBuildInfo.cmake Outdated
@@ -9,6 +9,9 @@
#
# CHANGELOG
#
# 1.0.1 - 2023-04-19
# - Fix building buildinfo source files.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# - Fix building buildinfo source files.
# - Fix -Wstrict-prototypes warning in buildinfo source files.

Same the commit message can be changed.

@rodiazet rodiazet merged commit 4211942 into master Apr 20, 2023
@rodiazet rodiazet deleted the evmone-t8n-version branch April 20, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chfast chfast chfast approved these changes

@axic axic axic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rodiazet @axic @chfast