makefiles/tests: use BINFILE for hash comparision instead of ELFFILE by maribu · Pull Request #18216 · RIOT-OS/RIOT

maribu · 2022-06-16T08:02:36Z

Contribution description

Let's consider firmwares as identical if their binfiles are matching, e.g. in case the .debug section ends up being different for whatever reason.

This hopefully makes sure false positives on hash mismatches are a thing of the past.

Testing procedure

Murdock should be happy. Even for ESP32 builds, every time.

Issues/PRs references

None

maribu · 2022-06-16T08:04:46Z

Note that creation of the binfile is one (rather simple) step more. But I do think that many ESP32 builds currently do get rebuild within the bounds of three failures due to this issue, and much less re-runs will be needed if the false positives don't happen anymore.

MrKevinWeiss · 2022-06-16T08:26:09Z

Does everything have a BINFILE? I think maybe some might only have a HEXFILE, I think that FLASHFILE takes either the BINFILE or HEXFILE depending on the requirements for the board... Only an off-the-top-of-my-head answer.

gschorcht · 2022-06-16T08:27:36Z

makefiles/tests/tests.inc.mk

 # RIOTNOLINK="".
 ifeq (,$(RIOTNOLINK))
-test-input-hash: $(TESTS) $(TESTS_WITH_CONFIG) $(TESTS_AS_ROOT) $(ELFFILE) $(TEST_EXTRA_FILES)
+test-input-hash: $(TESTS) $(TESTS_WITH_CONFIG) $(TESTS_AS_ROOT) $(BINFILE) $(TEST_EXTRA_FILES)


If I'm right, BINFILE is not built in CI:

RIOT/Makefile.include

Lines 689 to 695 in 55b57e1

# By default always build ELFFILE, BINFILE and FLASHFILE

ifeq ($(RIOT_CI_BUILD),1)

# Don't build BINFILE on the CI to save some computation time

BUILD_FILES += $(ELFFILE) $(FLASHFILE)

else

BUILD_FILES += $(ELFFILE) $(BINFILE) $(FLASHFILE)

endif

But if it is added as prerequisite, it should be built

Hm, I tried it for AVR, only the FLASHFILE (.hex) is generated when RIOT_CI_BUILD is set.

MrKevinWeiss · 2022-06-16T08:34:20Z

Whatever we change it to we may want to add a REMOVEME commit that prevents silent failures.

gschorcht · 2022-06-16T08:37:59Z

Does everything have a BINFILE? I think maybe some might only have a HEXFILE, I think that FLASHFILE takes either the BINFILE or HEXFILE depending on the requirements for the board... Only an off-the-top-of-my-head answer.

Even though BINFILE is defined for any platform in $(RIOTBASE)/Makefile.include as

BINFILE ?= $(ELFFILE:.elf=.bin)

FLASHFILE is also generated in CI and would help to solve the problem for ESP32 since in ESPs the FLASHFILE corresponds to the BINFILE.

maribu · 2022-06-16T08:42:55Z

So we should go for FLASHFILE instead?

Given that the hash is used to guard tests runs, it may actually be quite beneficial to not run tests again if the machine code is unchanged. E.g. a whitespace change will result in a different ELF file with debug symbols, as the mapping of source lines to machine code will differ. Still, running the test again will (hopefully!) not yield a different result.

MrKevinWeiss · 2022-06-16T08:47:16Z

Sure, maybe add a remove me commit before the has that sets FLASHFILE?=$RANDOM to be sure there is already something in there and it should fail if there isn't?

maribu · 2022-06-16T09:33:16Z

@MrKevinWeiss: Added and ifeq (,$(FLASHFILE)) $(error) endif instead. That is something we should be able to keep

gschorcht · 2022-06-16T11:09:38Z

makefiles/tests/tests.inc.mk

+  ifeq (,$(FLASHFILE))
+    $(error "FLASHFILE is not set!")
+  endif


I guess this check isn't needed here because this check is already done here

RIOT/Makefile.include

Lines 685 to 687 in 55b57e1

ifeq (,$(FLASHFILE))

$(error FLASHFILE is not defined for this board: $(FLASHFILE))

endif

before makefile/tests/test.inc.mk is included. Right?

BTW, using the undefined FLASHFILE variable in the error message doesn't make sense 😉

The intention was probably to use $(BOARD)

maribu · 2022-06-17T06:45:40Z

I changed back to BINFILE. The issue of mismatching hashes of the ELFFILE is not limited to ESP32 and other targets do use the ELFFILE as FLASHFILE.

Let's use consistently the BINFILE, sot hat we finally have the issue solved.

Also note that it will result in faster CI times when tests are running, as right now white-space-only changes will result in tests being re-run (due to changes in .debug of the ELFFILE), while it will no longer be the case with BINFILE.

maribu · 2022-06-17T07:06:00Z

Cool, no BINFILE for MIPS. I am using the HEXFILE on MIPS now instead, that should also be robust against changes in debug symbols.

gschorcht · 2022-06-17T15:49:37Z

Please squash.

Let's consider firmwares as identical if their flash files are matching. This will have the side effect that hash mismatches for ESP32 due to different .debug sections in the ELFFILE are prevented, as for ESP32 the BINFILE is used.

gschorcht · 2022-06-18T12:42:38Z

@maribu Thanks for solving that problem.

maribu · 2022-06-18T14:15:07Z

Thx for the reviews!

benpicco · 2022-06-19T15:44:48Z

It appears as the issue is still present

maribu · 2022-06-19T16:01:20Z

Shit. I will try to take a look soonish

gschorcht · 2022-06-19T16:07:29Z

Might it be that the PR has to be rebased?

benpicco · 2022-06-19T16:14:24Z

Murdock will rebase automatically before building

maribu · 2022-06-20T06:48:56Z

I think I know what the issue is:

$ rm bin -rf && \
make BOARD=esp32-heltec-lora32-v2 BUILD_IN_DOCKER=1 RIOT_CI_BUILD=1 TEST_KCONFIG=1 test-input-hash && \
cat bin/esp32-heltec-lora32-v2/test-input-hash.sha1

make: *** No rule to make target '/home/maribu/Repos/software/RIOT/tests/driver_mhz19/bin/esp32-heltec-lora32-v2/tests_driver_mhz19.bin', needed by 'test-input-hash'.  Stop.

Since there is no explicit rule to create the binfile, make cannot now for what job it has to wait to finish before starting to compare the hash value.

Doing a make all will create that binfile. And the hashes are reproducible identical.

maribu · 2022-06-20T06:58:06Z

False alarm, it does work fine without BUILD_IN_DOCKER=1. When manually starting the docker container and calling make BOARD=esp32-heltec-lora32-v2 RIOT_CI_BUILD=1 TEST_KCONFIG=1 test-input-hash in there, it does just work.

maribu · 2022-06-21T10:26:20Z

I opened #18236 in the hope it can shed some light on the remaining issue. I cannot reproduce mismatching values in test-input-hash.sha1 anymore and I have the feeling that the issue is something like reading it out before its contents have been fully written to or so.

gschorcht · 2022-06-21T11:02:37Z

I cannot reproduce mismatching values in test-input-hash.sha1 anymore

Me too.

and I have the feeling that the issue is something like reading it out before its contents have been fully written to or so.

What confirms this assumption is that the problem seems to occur only sporadically. Some compilations have already been successful, others still show the problem. It seems to occur mostly with esp32-heltec-lora32-v2. Might there be a dependency problem of BINFILE?

maribu mentioned this pull request Jun 16, 2022

cpu/esp32: enforce MAXTHREADS is at least 3 #18210

Merged

github-actions bot added the Area: tests Area: tests and testing framework label Jun 16, 2022

gschorcht reviewed Jun 16, 2022

View reviewed changes

maribu force-pushed the makefiles/tests/tests.inc.mk branch from 7eb283c to 181dd44 Compare June 16, 2022 09:26

gschorcht reviewed Jun 16, 2022

View reviewed changes

maribu changed the title ~~makefiles/tests: use BINFILE for hash comparision instead of ELFFILE~~ makefiles/tests: use FLASHFILE for hash comparision instead of ELFFILE Jun 16, 2022

maribu force-pushed the makefiles/tests/tests.inc.mk branch from 181dd44 to 55320dd Compare June 16, 2022 12:36

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 16, 2022

maribu changed the title ~~makefiles/tests: use FLASHFILE for hash comparision instead of ELFFILE~~ makefiles/tests: use BINFILE for hash comparision instead of ELFFILE Jun 17, 2022

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: MIPS Platform: This PR/issue effects MIPS-based platforms labels Jun 17, 2022

maribu added 2 commits June 17, 2022 18:31

Makefile.include: fix typo

a247e87

maribu force-pushed the makefiles/tests/tests.inc.mk branch from 9b6393a to a247e87 Compare June 17, 2022 16:31

gschorcht approved these changes Jun 18, 2022

View reviewed changes

gschorcht merged commit e6823ed into RIOT-OS:master Jun 18, 2022

maribu deleted the makefiles/tests/tests.inc.mk branch June 18, 2022 14:16

chrysn added this to the Release 2022.07 milestone Aug 25, 2022

	# By default always build ELFFILE, BINFILE and FLASHFILE
	ifeq ($(RIOT_CI_BUILD),1)
	# Don't build BINFILE on the CI to save some computation time
	BUILD_FILES += $(ELFFILE) $(FLASHFILE)
	else
	BUILD_FILES += $(ELFFILE) $(BINFILE) $(FLASHFILE)
	endif

	ifeq (,$(FLASHFILE))
	$(error FLASHFILE is not defined for this board: $(FLASHFILE))
	endif

Conversation

maribu commented Jun 16, 2022

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu commented Jun 16, 2022

Uh oh!

MrKevinWeiss commented Jun 16, 2022

Uh oh!

gschorcht Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

kaspar030 Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

gschorcht Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss commented Jun 16, 2022

Uh oh!

gschorcht commented Jun 16, 2022

Uh oh!

maribu commented Jun 16, 2022

Uh oh!

MrKevinWeiss commented Jun 16, 2022

Uh oh!

maribu commented Jun 16, 2022

Uh oh!

gschorcht Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

gschorcht Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

gschorcht Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

maribu commented Jun 17, 2022

Uh oh!

maribu commented Jun 17, 2022

Uh oh!

gschorcht commented Jun 17, 2022

Uh oh!

gschorcht commented Jun 18, 2022

Uh oh!

maribu commented Jun 18, 2022

Uh oh!

benpicco commented Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maribu commented Jun 19, 2022

Uh oh!

gschorcht commented Jun 19, 2022

Uh oh!

benpicco commented Jun 19, 2022

Uh oh!

maribu commented Jun 20, 2022

Uh oh!

maribu commented Jun 20, 2022

Uh oh!

maribu commented Jun 21, 2022

Uh oh!

gschorcht commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

benpicco commented Jun 19, 2022 •

edited

Loading