Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Remove VMTRACE option #5570

Merged
merged 7 commits into from
Jun 17, 2019
Merged

Remove VMTRACE option #5570

merged 7 commits into from
Jun 17, 2019

Conversation

chfast
Copy link
Member

@chfast chfast commented Apr 15, 2019

Fixes #5565.

This removes CMake's VMTRACE options by making --vmtrace always available to testeth. Win-Win.

@winsvega
Copy link
Contributor

Wait the option is still used in debug mode. I use it to trace step by step execution.

@codecov-io
Copy link

codecov-io commented Apr 15, 2019

Codecov Report

Merging #5570 into master will decrease coverage by 0.05%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5570      +/-   ##
==========================================
- Coverage   62.64%   62.59%   -0.06%     
==========================================
  Files         350      350              
  Lines       29684    29690       +6     
  Branches     3344     3344              
==========================================
- Hits        18597    18583      -14     
- Misses       9878     9897      +19     
- Partials     1209     1210       +1

@chfast
Copy link
Member Author

chfast commented Apr 16, 2019

Wait the option is still used in debug mode. I use it to trace step by step execution.

Ok then. We will add --vmtrace option also for aleth.

Anyway, can we merge it now and include in 1.6.0 release to make testeth there more useful? If too big or cannot land without the mentioned flag for aleth, I will do it for 1.7.

@gumb0
Copy link
Member

gumb0 commented Apr 16, 2019

I would prefer not to break vmtrace in aleth in 1.6.0

Copy link
Contributor

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

This removes CMake's VMTRACE options by making --vmtrace always available to testeth. Win-Win.

works

@chfast
Copy link
Member Author

chfast commented Apr 17, 2019

@winsvega Can you elaborate what "works"?
I though you need also tracing logs from running aleth, e.g. aleth --dev. Is that the case?

@winsvega
Copy link
Contributor

From retesteth. Evm debug from the client I propose for the future when retesteth gonna be adapted.

I think @cdetrio Casey was using the trace from clients to check consensus also

@winsvega
Copy link
Contributor

*testeth works with this change

@gumb0
Copy link
Member

gumb0 commented Apr 17, 2019

vmtrace in aleth is useful to debug issues with soltest, and also in case we have a consensus issue during sync.

@chfast
Copy link
Member Author

chfast commented May 6, 2019

I don't have time to continue working on this currently, but it would be huge UX improvement to remove compile time flag.

@gumb0
Copy link
Member

gumb0 commented Jun 12, 2019

I will try to finish this.

Not sure if we need additional --vmtrace flag for aleth, I think without it -v 4 --log-channels vmtrace will also work just fine.

@gumb0
Copy link
Member

gumb0 commented Jun 13, 2019

I've added --log-vmtrace flag to aleth, it now controls the global flag for vmtrace, which is in all respects similar to the old compile-time flag. New flag lives in Log.cpp.

This should be ready, please review @chfast @winsvega

@gumb0 gumb0 requested review from winsvega and halfalicious June 13, 2019 14:16
@chfast
Copy link
Member Author

chfast commented Jun 17, 2019

Rebased. I will merge it after CI builds.

@chfast chfast merged commit 697cdd7 into master Jun 17, 2019
@chfast chfast deleted the vmtrace branch June 17, 2019 09:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

testeth reports not built with -DVMTRACE=1 even when it was
4 participants