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

Code Coverage #310

Merged
merged 6 commits into from
Aug 16, 2018
Merged

Code Coverage #310

merged 6 commits into from
Aug 16, 2018

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Aug 14, 2018

Closes #309

@chfast
Copy link
Collaborator Author

chfast commented Aug 14, 2018

Looks like the evm2wasm build get messed up when building inside Aleth. I'm not sure why. Probably the easiest approach would be to enabled -Wsign-conversion in evm2wasm and fix all warnings.


set(CMAKE_TOOLCHAIN_FILE ${CMAKE_CURRENT_SOURCE_DIR}/cmake/toolchains/cxx11-pic.cmake CACHE FILEPATH "CMake toolchain file")
Copy link
Member

Choose a reason for hiding this comment

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

Ned to remove the extra toolchain file then.

Copy link
Member

Choose a reason for hiding this comment

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

Done.

CMakeLists.txt Outdated
@@ -19,6 +19,9 @@ endif()

project(hera)

cable_configure_compiler()
add_compile_options(-Wno-pedantic -Wno-sign-conversion)
Copy link
Member

Choose a reason for hiding this comment

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

Does cable adds -Wpedantic so that we need to negative it here?

Any point keeping -Wno-unknown-pragmas?

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove -Wno-sign-conversion and update evm2wasm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The -Wno-sign-conversion is also needed in Hera.

Copy link
Member

Choose a reason for hiding this comment

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

Tracked in #311.

@axic
Copy link
Member

axic commented Aug 15, 2018

Probably the easiest approach would be to enabled -Wsign-conversion in evm2wasm and fix all warnings.

It seems that the better way would be fixing evm2wasm, many of those seem to be actual bugs. In a separate PR we could introduce the warning and fix it.

@codecov-io
Copy link

codecov-io commented Aug 15, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@2fbaebc). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master    #310   +/-   ##
========================================
  Coverage          ?   65.7%           
========================================
  Files             ?       5           
  Lines             ?     866           
  Branches          ?     122           
========================================
  Hits              ?     569           
  Misses            ?     268           
  Partials          ?      29

@chfast
Copy link
Collaborator Author

chfast commented Aug 15, 2018

This is ready.

@@ -14,7 +14,8 @@ defaults:
echo Generator: $GENERATOR
echo CMake options: $CMAKE_OPTIONS
$CXX --version
$CXX --version > compiler.version
$CXX --version >> toolchain
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the first one be > to clear the file if it exists?

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 could be, but this a CI and we start from clean environment.

@@ -14,7 +14,8 @@ defaults:
echo Generator: $GENERATOR
echo CMake options: $CMAKE_OPTIONS
$CXX --version
$CXX --version > compiler.version
$CXX --version >> toolchain
echo $GENERATOR >> toolchain
Copy link
Member

Choose a reason for hiding this comment

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

Also instead could just include the generator checksum in the key separately.

@@ -224,14 +230,14 @@ jobs:
- *fetch-tests
- *test

linux-clang-shared-debug:
linux-gcc-shared-coverage:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe debug-coverage?

Copy link
Member

Choose a reason for hiding this comment

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

On a second thought, maybe we shouldn't do coverage on debug, because we get the non existant coverage on the debug namespace for which we won't have test anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. coverage implies build with debugging symbols.
  2. Checking coverage in release builds is not doable. Let's have a base first, then we can start tweaking the config. There are other ways of dealing with such issues like blacklisting some files.
  3. Also, I don't think we have a capacity to run debug and coverage builds at the moment.


set(CMAKE_TOOLCHAIN_FILE ${CMAKE_CURRENT_SOURCE_DIR}/cmake/toolchains/cxx11-pic.cmake CACHE FILEPATH "CMake toolchain file")
cable_configure_toolchain(DEFAULT cxx11-pic)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this getting the toolchain from cable instead? Seems not since I've removed it from the cmake/toolchains directory and it is failing now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I taught Cable to also check ${PROJECT_SOURCE_DIR}/cmake/toolchains dir.

@chfast
Copy link
Collaborator Author

chfast commented Aug 16, 2018

@axic, what have you done that it is failing now?

@axic axic merged commit 1724607 into master Aug 16, 2018
@axic axic deleted the coverage branch August 16, 2018 14:28
@axic axic removed the in progress label Aug 16, 2018
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.

3 participants