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

.murdock: disable Kconfig hash comparison #18399

Closed
wants to merge 1 commit into from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Aug 4, 2022

Contribution description

This has been randomly broken on CI for at least half a year.
So far nobody could figure out what to do about it, so let's disable the check for now to get builds past CI again.

Testing procedure

CI run should succeed on the first try.

Issues/PRs references

@benpicco benpicco requested a review from kaspar030 as a code owner August 4, 2022 09:36
@github-actions github-actions bot added the Area: CI Area: Continuous Integration of RIOT components label Aug 4, 2022
@benpicco benpicco added CI: full build disable CI build filter CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 4, 2022
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

It would be better to just to a module/packages compare, this way we won't reintroduce mismatches. The hash just allows us to catch any injected configurations or missed CFLAGS, which probably won't create so much work when reintroducing it. The most work is getting the modules and packages to match.

@miri64 miri64 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 4, 2022
@miri64
Copy link
Member

miri64 commented Aug 4, 2022

Agreeing with @MrKevinWeiss. If we ever want to finish up Kconfig migration, we should make sure it generates the same binaries and find out what is happening if they don't match. Furthermore, @MrKevinWeiss is investigating the issue in the CI at the moment: #18396

@benpicco benpicco added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Area: Kconfig Area: Kconfig integration labels Aug 4, 2022
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Maybe something more like this

diff --git a/.murdock b/.murdock
index 906799a045..c23d7064a8 100755
--- a/.murdock
+++ b/.murdock
@@ -452,16 +452,18 @@ compile() {
 
     if get_supported_kconfig_board_app "${board}" "${appdir}"; then
         should_check_kconfig_hash=1
-        BOARD=${board} make -C${appdir} clean
-        CCACHE_BASEDIR="$(pwd)" BOARD=${board} TOOLCHAIN=${toolchain} RIOT_CI_BUILD=1 TEST_KCONFIG=1 \
-                        make -C${appdir} all test-input-hash -j${JOBS:-4}
-        RES=$?
-        if [ $RES -eq 0 ]; then
-            kconfig_hashes="$(cat ${BINDIR}/test-input-hash.sha1)"
-        else
-            kconfig_hashes="kconfig-build-failed"
-            echo "An error occurred while compiling using Kconfig";
-        fi
+        # Disabled until non-reproducible build issues are fixed!
+
+        # BOARD=${board} make -C${appdir} clean
+        # CCACHE_BASEDIR="$(pwd)" BOARD=${board} TOOLCHAIN=${toolchain} RIOT_CI_BUILD=1 TEST_KCONFIG=1 \
+        #                 make -C${appdir} all test-input-hash -j${JOBS:-4}
+        # RES=$?
+        # if [ $RES -eq 0 ]; then
+        #     kconfig_hashes="$(cat ${BINDIR}/test-input-hash.sha1)"
+        # else
+        #     kconfig_hashes="kconfig-build-failed"
+        #     echo "An error occurred while compiling using Kconfig";
+        # fi
     fi
 
     # compile without Kconfig
@@ -470,23 +472,25 @@ compile() {
         make -C${appdir} all test-input-hash -j${JOBS:-4}
     RES=$?
 
-    no_kconfig_hashes="$(cat ${BINDIR}/test-input-hash.sha1)"
+    # no_kconfig_hashes="$(cat ${BINDIR}/test-input-hash.sha1)"
     # test hash is used to cache test results, not for comparing binaries
     # generated with and without KConfig
     test_hash=$(test_hash_calc "$BINDIR")
 
-
-    if [ ${should_check_kconfig_hash} != 0 ]; then
-        if [ "${kconfig_hashes}" != "${no_kconfig_hashes}" ]; then
-            echo "Hashes of binaries with and without Kconfig mismatch for ${appdir} with ${board}";
-            echo "Please check that all used modules are modelled in Kconfig and enabled";
-            echo "Input without KConfig:"
-            echo "${no_kconfig_hashes}"
-            echo "Input with KConfig:"
-            echo "${kconfig_hashes}"
-            kconfig_module_packages_diff ${board} ${appdir}
-            RES=1
-        fi
+    # if [ ${should_check_kconfig_hash} != 0 ]; then
+    #     if [ "${kconfig_hashes}" != "${no_kconfig_hashes}" ]; then
+    #         echo "Hashes of binaries with and without Kconfig mismatch for ${appdir} with ${board}";
+    #         echo "Please check that all used modules are modelled in Kconfig and enabled";
+    #         echo "Input without KConfig:"
+    #         echo "${no_kconfig_hashes}"
+    #         echo "Input with KConfig:"
+    #         echo "${kconfig_hashes}"
+    #         kconfig_module_packages_diff ${board} ${appdir}
+    #         RES=1
+    #     fi
+    if get_supported_kconfig_board_app "${board}" "${appdir}"; then
+        kconfig_module_packages_diff ${board} ${appdir}
+        RES=$(( $RES | $? ))
     fi
 
     # run tests

@gschorcht
Copy link
Contributor

gschorcht commented Aug 4, 2022

At the moment, it seems that about 3 out of 4 of my CI compilations fail and the repeated compilations waste a lot of Murdock time. Nevertheless, I totally agree with @MrKevinWeiss about not disabling the hash mismatch test.

The problem seems to be some kind of timing issue. What we do know is that it occurs frequently for one application (tests/pkg_cryptoauthlib_internal-tests), usually exactly once per compilation test run, and probably for a board that takes some time to compile (often for any ESP32 board).

@MrKevinWeiss
Copy link
Contributor

I did notice that compiling the ESPs take quite some time (they were the ones that timed-out justing the first test). That could be the reason why we see them more often, they just take longer?

@benpicco
Copy link
Contributor Author

benpicco commented Aug 8, 2022

Maybe something more like this

Want to PR that? Then I can give you an ACK.

This has been randomly broken on CI for at least half a year.
So far nobody could figure out what to do about it, so let's
disable the check for now to get builds past CI again.
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 21, 2022
@github-actions github-actions bot removed the Area: Kconfig Area: Kconfig integration label Aug 21, 2022
@benpicco
Copy link
Contributor Author

Rebased as the issue still persists.

@MrKevinWeiss
Copy link
Contributor

Is it the same issue or a different on? It is intentionally on nightlies as we still do not have reproducible builds and should probably figure out why. As for the PR errors #18470 tries to address it.

@MrKevinWeiss
Copy link
Contributor

somehow solved in a very complex way!

@benpicco benpicco deleted the kconfig-hash branch August 30, 2022 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI Area: Continuous Integration of RIOT components CI: full build disable CI build filter CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants