euphony build success, even if test code has problem

yeonns commented 3 years ago

Environment

Windows 10
Android Studio 4.2.2
git pull euphony master code

How to reproduce

git pull euphony master
build euphony with no devices connected (build successful)
modify test code
- test code: euphony/src/main/cpp/tests/base16Test.cpp
- change line 32 like below:
```
TestParamType(std::vector<u_int8_t>{ 0x61 }, "621"),
```
connect android device (Galaxy S20 FE)
build euphony with device connected
build is successful (It should be failed!!)

Question

Why build success, even if test code has problem? How can I resolve this?

seohy02 commented 3 years ago

I had the same problem and found a solution!

git pull euphony master
build euphony with no devices connected (build successful)
modify test code
- test code: euphony/src/main/cpp/tests/base16Test.cpp
- change line 32 like below:
```
TestParamType(std::vector<u_int8_t>{ 0x61 }, "621"),
```
connect android device (Galaxy S20 FE)

And remove the directory euphony/.cxx Then, if you build it again, build will fail!

The native test runs by building only when a device(android device or AVD) is connected. If the device connection changes, you must delete the directory euphony/.cxx before building. I hope it works for you! Thanks!!

yeonns commented 3 years ago

@seoh02h :open_mouth: It's good solution! As you said, after deleting .cxx directory, build issue was resolved! Thank you :heart: However, if I delete directory and rebuild it, build speed is too slow. Any other good idea? :sob:

YoungSeokHong commented 3 years ago

I think it because your computer execute ADB not at every build but at only first build. A command executed with execute_process is only executed at very first time, and isn't executed after .cxx directory is made unless code is changed. We need to use POST_BUILD option in add_custom_command to execute ADB after the first build.

In your case, you can just simply add SEND ERROR in https://github.com/euphony-io/euphony/blob/54966925eb8a78d72e47eb31acd03c270f4c9694/euphony/src/main/cpp/tests/CMakeLists.txt#L43

like

message(SEND_ERROR "NO ABI OR MORE THAN ONE DEVICE/EMULATOR")

to notice Android Studio. But, it couldn't be a perfect solution. Another error would occur if you do what you did twice And, for now, if I execute build with x86(or something) device after disconnect arm device, an error occur because program doesn't know changing of devices

Therefore, I moved CMakeList.txt bottom part to cmake.run.test.script. It is a little ignorant way, but we can fix your problem and the error that I said. Here is my soulution

CMakeList.txt

cmake_minimum_required(VERSION 3.4.1)

set(TEST_EUPHONY testEuphony)

# Include GoogleTest library
set (GOOGLETEST_ROOT ${ANDROID_NDK}/sources/third_party/googletest)
add_library (gtest STATIC ${GOOGLETEST_ROOT}/src/gtest_main.cc ${GOOGLETEST_ROOT}/src/gtest-all.cc)
target_include_directories (gtest PRIVATE ${GOOGLETEST_ROOT})
target_include_directories (gtest PUBLIC ${GOOGLETEST_ROOT}/include)

add_executable(
        ${TEST_EUPHONY}
        asciiCharsetTest.cpp
        base2Test.cpp
        base16Test.cpp
        defaultCharsetTest.cpp
        FFTTest.cpp
        FSKTest.cpp
        hexVectorTest.cpp
        packetTest.cpp
        packetBuilderTest.cpp
        packetErrorDetectorTest.cpp
        packetWithFSKTest.cpp
        waveTest.cpp
        waveBuilderTest.cpp
        waveRendererTest.cpp
)

target_link_libraries(${TEST_EUPHONY} PUBLIC euphony gtest)

set(TARGET_TEST_DIR /data/local/tmp/${TEST_EUPHONY}) # Directory on device to push tests.
set(TARGET_TEST_LIB_DIR ${TARGET_TEST_DIR}/${ANDROID_ABI})
set(LIBCPP_SHARED_PATH ${ANDROID_NDK}/sources/cxx-stl/llvm-libc++/libs/${ANDROID_ABI}/libc++_shared.so)

get_target_property( OBOE_LIBRARY_PATH oboe::oboe IMPORTED_LOCATION)
find_program(ADB NAMES adb PATHS ${ANDROID_SDK_ROOT}/platform-tools) # Verified to be working on Linux.

        # Run gtest & get the result.
add_custom_command(TARGET ${TEST_EUPHONY} POST_BUILD
        COMMAND ${CMAKE_COMMAND} -DOBOE_LIBRARY_PATH:STRING="${OBOE_LIBRARY_PATH}" -DANDROID_ABI=${ANDROID_ABI} -DADB:STRING="${ADB}" -DTARGET_TEST_LIB_DIR:STRING="${TARGET_TEST_LIB_DIR}" -DLIBCPP_SHARED_PATH:STRING="${LIBCPP_SHARED_PATH}" -DTEST_EUPHONY:STRING="${TEST_EUPHONY}" -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake.run.test.script
        )

cmake.run.test.script

execute_process(COMMAND ${ADB} shell getprop ro.product.cpu.abi
        OUTPUT_VARIABLE ADB_DEVICE_ABI
        RESULT_VARIABLE ADB_DEVICE_ABI_RESULT)

if( ${ADB_DEVICE_ABI_RESULT} )
    message(SEND_ERROR "NO ABI OR MORE THAN ONE DEVICE/EMULATOR")
else()
    message(${ADB_DEVICE_ABI} ${ANDROID_ABI})
    string(STRIP ${ADB_DEVICE_ABI} ADB_DEVICE_ABI)
    if(${ANDROID_ABI} STREQUAL ${ADB_DEVICE_ABI})
        execute_process(COMMAND ${ADB} push $<TARGET_FILE:euphony> ${TARGET_TEST_LIB_DIR}/)
        execute_process(COMMAND ${ADB} push ${OBOE_LIBRARY_PATH} ${TARGET_TEST_LIB_DIR}/)
        execute_process(COMMAND ${ADB} push ${LIBCPP_SHARED_PATH} ${TARGET_TEST_LIB_DIR}/)
        execute_process(COMMAND ${ADB} push $<TARGET_FILE:${TEST_EUPHONY}> ${TARGET_TEST_LIB_DIR}/)
        execute_process(COMMAND ${ADB} shell chmod 755 ${TARGET_TEST_LIB_DIR}/${TEST_EUPHONY})

        execute_process(COMMAND ${ADB} shell LD_LIBRARY_PATH=${TARGET_TEST_LIB_DIR} ${TARGET_TEST_LIB_DIR}/testEuphony
                    OUTPUT_VARIABLE GTEST_EUPHONY_OUTPUT
                    RESULT_VARIABLE GTEST_RESULT
                    ERROR_VARIABLE GTEST_ERROR_OUTPUT
                    )

        if( ${GTEST_RESULT} GREATER 0 )
            string(STRIP ${GTEST_EUPHONY_OUTPUT} GTEST_EUPHONY_OUTPUT)
            string(REPLACE "\n\n" "\r[NEXT]\r" GTEST_EUPHONY_OUTPUT ${GTEST_EUPHONY_OUTPUT})
            string(REPLACE "\n" "\r" GTEST_EUPHONY_OUTPUT ${GTEST_EUPHONY_OUTPUT})
            message(STATUS ${TARGET_TEST_LIB_DIR})
            message(STATUS "HOHO")
            message(FATAL_ERROR "** Gtest Failure (${GTEST_RESULT}) **\r${GTEST_EUPHONY_OUTPUT}")
        else()
           message("**** Gtest Success ****")
        endif()
    endif()
endif()

yeonns commented 3 years ago

@YoungSeokHong That's a good approach! 👍 But with your solution, if there is no device connected, build always fails even if native source code has no problem. When there's no connected device, I think it seems appropriate to proceed only with the build for the native source code without executing the script for gtest. So, how about removing message(SEND_ERROR "NO ABI OR MORE THAN ONE DEVICE/EMULATOR") of your cmake.run.test.script.

And, I found one more issue of your approach. I tried below process: CASE1

no device connected
build success
1 device connect (android smartphone)
build success (Its result is normal, but it didn't run gtest!!)

CASE2

no device connected
build success
1 device connect (android smartphone)
modify test code to fail (line 32 of euphony/src/main/cpp/tests/base16Test.cpp)
build success (It didn't run gtest, so build success even if test code has problem)

I want gtest to run when the device is connected 😢

judemin commented 3 years ago

After first delete of .cxx folder and project rebuild, my code works properly even I changed base16 test code.

Build has succeeded

After changing base16 test code to "621", build fails properly.

yeonns commented 3 years ago

@judemin In your first screenshot, the test code doesn't seem to have changed, so it seems normal to succeed. Even there was a problem with test code and after deleting .cxx, was there a case where the build success? 😮

kuro11pow2 commented 3 years ago

@judemin FYI, I succeeded in build and passed the test case. After that, I modified the test case to fail and removed the .cxx file. And when the project is rebuilt, the test case does not fail. it seems that the unit test is not running at all.

aiclaudev commented 3 years ago

@kuro11pow2

1) no device connected 2) build success 3) 1 device connect 4) modify code to fail 5) remove .cxx file 6) bulid success

Have you done the above process? It's really weird. When I tried it in the order above, build is failed. What about check your device connection?

If there's something I misunderstood, please tell me :)

seohy02 commented 3 years ago

@judemin What you wrote is correct. I think because there was no device connection change.

But, did you try to build with that whole process @aiclaudev wrote except for step 5 "remove .cxx file"? If the device connection is changed but .cxx directory is not deleted, the connection change is not applied properly.

Also, in the situation as when you wrote that comment(1 device connected, test code modified, build failed properly), If you disconnect the device and build without deleting .cxx directory, build will fail with the error message below.

So, whenever the device connection changes, you have to delete .cxx directory to build properly. This is the problem. 😥😥

judemin commented 3 years ago

@judemin In your first screenshot, the test code doesn't seem to have changed, so it seems normal to succeed. Even there was a problem with test code and after deleting .cxx, was there a case where the build success? 😮

Thanks for your reply. I misunderstood the mainstream of this issue. When I tested the code I didn't change the status of devices. @seoh02h's reply might be proper approach of this issue.

YoungSeokHong commented 3 years ago

Remove gradle cache in CMakeList

After several tests in this situation, I conclude that it is because of that Android Studio uses the cache made from previous build when building.

Gradle use a cache made from previous build when building, unless codes are changed or environment is changed. Unfortunately, the current gradle doesn't know device changing and builds using the old cache, so this problem is occurring.

To solve this problem, I chose to clear gradle's cache directly in CMakeList. The way is simple.

1. Make a variable in `build.gradle(:euphony)` and put cache path in it

Thanks to @seoh02h , I could find ${project.gradle.gradleUserHomeDir}
externalNativeBuild {
cmake {
...
arguments "-DBUILD_CACHE=${project.gradle.gradleUserHomeDir}\\.gradle\\caches\\transforms-2\\files-2.1\\2fc06640f22f0b2c2519ff4c360c6065.bin"
...
}
}
2. Add the code below to the bottom of CMakeList.
file(REMOVE ${BUILD_CACHE})

After that, it would work well 👍

We still have two problems. First, typing 2fc06640f22f0b2c2519ff4c360c6065.bin in code is not a general way. This method can only solve this problem, so we need find more "general" way. Second, using this method, we always remove the cache regardless of device changing. It cause slow-building. I think this matter could be solved by using "file(remove -)" only when error not occur. And all ADB works need to be moved to cmake.run.test.script in this case.

yeonns commented 3 years ago

@YoungSeokHong @seoh02h Found nice solution! Thank you ❤️ To generalize gradle cache clearing, how about deleting ${GRADLE_USER_HOME}/.gradle/caches/transfoms-2/files-2.1/** ? And is it possible to write cmake that detects device changes and clearing cache in order not to slow down the build?

YoungSeokHong commented 3 years ago

To generalize gradle cache clearing, how about deleting ${GRADLE_USER_HOME}/.gradle/caches/transfoms-2/files-2.1/** ?

Thanks to your idea, I made a general way to remove all .bin files in .gradle\caches\transforms-2\files-2.1.

build.gradle(:euphony)

externalNativeBuild {
            cmake {
                  ...
                  arguments "-DBUILD_CACHE=${project.gradle.gradleUserHomeDir}\\.gradle\\caches\\transforms-2\\files-2.1"
                  ...
            }
}

CMakeList.txt

file(GLOB REMOVING_FILES ${BUILD_CACHE}/*.bin)
file(REMOVE "${REMOVING_FILES}")

However, It doesn't work. I think it is because the file(GLOB - ) doesn't split files with " "(blank), but with ";".

This is the output of message("${REMOVING_FILES}")

I think we can fix it if we split the bin files with " " instead of ";". Anyone have a good idea?

ps. we also can remove all directories, but a very complicate error occur when we do that with CMakeList

YoungSeokHong commented 3 years ago

And is it possible to write cmake that detects device changes and clearing cache in order not to slow down the build?

I'm not sure, but I think it is possible if we move ADB works to cmake.run.test.script from CMakeList.txt, like what I said 6days ago. https://github.com/euphony-io/euphony/issues/46#issuecomment-922266305

seohy02 commented 3 years ago

@YoungSeokHong @yeonns Thanks! 👍👍

arguments "-DBUILD_CACHE=${project.gradle.gradleUserHomeDir}\\caches\\transforms-2\\files-2.1"

file(GLOB REMOVING_FILES ${BUILD_CACHE}/*.bin)
string(REPLACE ";" " " OUTPUT "${REMOVING_FILES}")
message("OUTPUT :: " "${OUTPUT}")
file(REMOVE "${OUTPUT}")

I tried using REPLACE The results are as follows.

But it doesn't work...

And I have some questions.

arguments "-DBUILD_CACHE=${project.gradle.gradleUserHomeDir}\\.gradle\\caches\\transforms-2\\files-2.1\\2fc06640f22f0b2c2519ff4c360c6065.bin"

It works only when \\.gradle is subtracted for me, as in discussion.

arguments "-DBUILD_CACHE=${project.gradle.gradleUserHomeDir}\\caches\\transforms-2\\files-2.1\\2fc06640f22f0b2c2519ff4c360c6065.bin"

And ${GRADLE_USER_HOME} doesn't work😥 Does It need extra setting?? Thanks!!!

seohy02 commented 3 years ago

build.gradle

arguments "-DBUILD_CACHE=${project.gradle.gradleUserHomeDir}\\caches\\transforms-2\\files-2.1"

CMakeLists.txt

file(GLOB REMOVING_FILES ${BUILD_CACHE}/*.bin)
foreach(removing_file IN LISTS REMOVING_FILES)
file(REMOVE ${removing_file})
endforeach()

@YoungSeokHong It works!!

yeonns commented 3 years ago

@seoh02h I represented project.gradle.gradleUserHomeDir as ${GRADLE_USER_HOME}. So you don't need to care about it. Sorry for confusing you 😅

And, I'm same with you. It works without .gradle

arguments "-DBUILD_CACHE=${project.gradle.gradleUserHomeDir}\\caches\\transforms-2\\files-2.1\\2fc06640f22f0b2c2519ff4c360c6065.bin"

seohy02 commented 3 years ago

@seoh02h I represented project.gradle.gradleUserHomeDir as ${GRADLE_USER_HOME}. So you don't need to care about it. Sorry for confusing you 😅

And, I'm same with you. It works without .gradle
arguments "-DBUILD_CACHE=${project.gradle.gradleUserHomeDir}\\caches\\transforms-2\\files-2.1\\2fc06640f22f0b2c2519ff4c360c6065.bin"

Oh!! Sorry! Thank you~!😁😁

designe commented 3 years ago

@YoungSeokHong @seoh02h Interesting Result! :) 👍 👍 👍

designe commented 3 years ago

I also found a solution :)

add_custom_command(TARGET ${TEST_EUPHONY} POST_BUILD
        COMMAND rm -rf ${CMAKE_BINARY_DIR}/CMakeFiles/3.10.2/CMakeSystem.cmake
        )

when above code placed on bottom of https://github.com/euphony-io/euphony/blob/master/euphony/src/main/cpp/tests/CMakeLists.txt, it will be working well!

like this.

cmake_minimum_required(VERSION 3.4.1)

set(TEST_EUPHONY testEuphony)

# Include GoogleTest library
set (GOOGLETEST_ROOT ${ANDROID_NDK}/sources/third_party/googletest)
add_library (gtest STATIC ${GOOGLETEST_ROOT}/src/gtest_main.cc ${GOOGLETEST_ROOT}/src/gtest-all.cc)
target_include_directories (gtest PRIVATE ${GOOGLETEST_ROOT})
target_include_directories (gtest PUBLIC ${GOOGLETEST_ROOT}/include)

add_executable(
        ${TEST_EUPHONY}
        asciiCharsetTest.cpp
        base2Test.cpp
        base16Test.cpp
        defaultCharsetTest.cpp
        FFTTest.cpp
        FSKTest.cpp
        hexVectorTest.cpp
        packetTest.cpp
        packetBuilderTest.cpp
        packetErrorDetectorTest.cpp
        packetWithFSKTest.cpp
        waveTest.cpp
        waveBuilderTest.cpp
        waveRendererTest.cpp
)

target_link_libraries(${TEST_EUPHONY} PUBLIC euphony gtest)

set(TARGET_TEST_DIR /data/local/tmp/${TEST_EUPHONY}) # Directory on device to push tests.
set(TARGET_TEST_LIB_DIR ${TARGET_TEST_DIR}/${ANDROID_ABI})
set(LIBCPP_SHARED_PATH ${ANDROID_NDK}/sources/cxx-stl/llvm-libc++/libs/${ANDROID_ABI}/libc++_shared.so)

get_target_property( OBOE_LIBRARY_PATH oboe::oboe IMPORTED_LOCATION)
find_program(ADB NAMES adb PATHS ${ANDROID_SDK_ROOT}/platform-tools) # Verified to be working on Linux.

execute_process(COMMAND ${ADB} shell getprop ro.product.cpu.abi
        OUTPUT_VARIABLE ADB_DEVICE_ABI
        RESULT_VARIABLE ADB_DEVICE_ABI_RESULT)

if( ${ADB_DEVICE_ABI_RESULT} )
    message("NO ABI OR MORE THAN ONE DEVICE/EMULATOR")
else()
    string(STRIP ${ADB_DEVICE_ABI} ADB_DEVICE_ABI)
    if(${ANDROID_ABI} STREQUAL ${ADB_DEVICE_ABI})
        # Prepare gtest for unit-test
        add_custom_command(TARGET ${TEST_EUPHONY} POST_BUILD
                COMMAND ${ADB} shell mkdir -p ${TARGET_TEST_LIB_DIR}

                # Push necessary libraries
                COMMAND ${ADB} push $<TARGET_FILE:euphony> ${TARGET_TEST_LIB_DIR}/
                COMMAND ${ADB} push ${OBOE_LIBRARY_PATH} ${TARGET_TEST_LIB_DIR}/
                COMMAND ${ADB} push ${LIBCPP_SHARED_PATH} ${TARGET_TEST_LIB_DIR}/

                # Push Euphony Test Binary
                COMMAND ${ADB} push $<TARGET_FILE:${TEST_EUPHONY}> ${TARGET_TEST_LIB_DIR}/
                COMMAND ${ADB} shell chmod 755 ${TARGET_TEST_LIB_DIR}/${TEST_EUPHONY}
                )

        # Run gtest & get the result.
        add_custom_command(TARGET ${TEST_EUPHONY} POST_BUILD
                COMMAND ${CMAKE_COMMAND} -DADB:STRING="${ADB}" -DTARGET_TEST_LIB_DIR:STRING="${TARGET_TEST_LIB_DIR}" -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake.run.test.script
                )
    endif()
endif()

# Remove cache of CMakeSystem file.
add_custom_command(TARGET ${TEST_EUPHONY} POST_BUILD
        COMMAND rm -rf ${CMAKE_BINARY_DIR}/CMakeFiles/3.10.2/CMakeSystem.cmake
        )

Please refer to it! gradle cache solution is really interesting. Please make a PR and let me know! :)

designe commented 3 years ago

@YoungSeokHong @seoh02h https://github.com/euphony-io/euphony/blob/master/HOWTOBUILD.md Is there some necessary update at above link?

seohy02 commented 3 years ago

@designe Yes!! I think native test doesn't run with ./gradlew test. Is it right?? And all parts should be updated referred to discussion #45

yeonns commented 3 years ago

@seoh02h How about creating an "update HOWTOBUILD doc" issue and contribute this?

designe commented 3 years ago

@designe Yes!! I think native test doesn't run with ./gradlew test. Is it right?? And all parts should be updated referred to discussion #45

Right, native test is not related to ./gradlew test :) With regard to this solution including #47 issue, we need to discuss about documentation for native unit-test operating guide

seohy02 commented 3 years ago

@yeonns @designe Ok!! I'll create the issue!

euphony-io / euphony