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

make -j flash fails due to missing make dependencies or make flash-only rebuilds the .elf #16385

Closed
iosabi opened this issue Apr 24, 2021 · 6 comments
Assignees
Labels
Area: build system Area: Build system Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@iosabi
Copy link
Contributor

iosabi commented Apr 24, 2021

Description

Many boards have FLASHFILE as ELFFILE and can directly flash the FLASHFILE setting the right FFLAGS; however in some cases boards need additional files or additional conversions expressed in makefile rules.

For example:

  1. adafruit-nrfutil based boards like BOARD=adafruit-clue add the $(HEXFILE).zip to FLASHDEPS (here) and the %.hex.zip target depends on the %.hex target as defined in that file. Also, the %.hex depends on the %.elf as defined here:
    %.hex: %.elf

This means that some targets in FLASHDEPS (the .hex.zip file) depend on the ELFFILE. This causes make flash-only to rebuild the ELFFILE because in the Makefile.include this elf target depends on FORCE

  1. The esptool based boards like BOARD=esp32-wroom-32 (in the CI) have a phony esp-image-convert target that uses the $(FLASHFILE) to generate the extra files needed for flashing which are then used by the flash-only target. The issue here is that esp-image-convert does not depend on $(FLASHFILE), so if you run make flash -j for this board one of three things can happen: a) you are lucky and the esp-image-convert target magically runs after the build is done (unlikely), b) you are unlucky and the first time you run make -j flash the missing $(FLASHFILE) just makes the build fail with an error, or c) you are extra unlucky and the esp-image-convert command runs reading an older version of the $(FLASHFILE) because the new one is still waiting for some .o to be compiled (the common case). In this last case you are left scratching your head why your board behaves like if the change you just made and compiled fine didn't happen.

The issue here is actually one of two different issues:

  1. Either the files added to FLASHDEPS depend on $(ELFFILE) when they need to (example 1), but then the flash-only command is actually building as well (example 1), or
  2. The added FLASHDEPS are phony targets that don't depend on $(ELFFILE) even if they use it but in that case make flash -j command doesn't work.

I didn't find a way to properly set up a board so that it compiles in parallel, flashes and passes the CI.

Note that in the CI having "flash-only" rebuild can cause problems. I tried adding the ELFFILE dependency in the second example but that fails because it tries to re-link the .elf file with an error that it didn't find the eps32 g++ compiler.

Steps to reproduce the issue

Example 1:

make -C tests/od BOARD=adafruit-clue all
make -C tests/od BOARD=adafruit-clue flash-only

Example 2:

make -C tests/od BOARD=esp32-wroom-32 all flash-only -j

Expected results

Example 1: "flash-only" command doesn't rebuild the .elf file.

Example 2: board flashes.

Actual results

Example 1: The second command (flash-only) should not rebuild the .elf

Example 2: The build very likely fails because the needed ELFFILE was not built when esp-image-convert starts running.

Versions

                 make: GNU Make 4.2.1
@iosabi
Copy link
Contributor Author

iosabi commented Apr 24, 2021

A maybe hacky solution I can think of is to add $(FLASHDEPS): $(FLASHFILE) when all or flash are in the $(MAKECMDGOALS), around line 750 of Makefile.include; and maybe add a .PHONY: $(FLASHDEPS) when flash-only is there.

The more correct solution is to remove FORCE from ELFFILE, but I don't know why was it added on the first place.

@fjmolinas
Copy link
Contributor

It seems this was not properly tested in #16073. I'm looking into the issue.

@fjmolinas
Copy link
Contributor

Is this a duplicate of #13492?

@fjmolinas
Copy link
Contributor

Is this a duplicate of #13492?

Hmm no its somehow more generic than #13492.

@iosabi
Copy link
Contributor Author

iosabi commented Apr 26, 2021

Yeah this is causing #13492, but the problem is more generic than esp*. It just so happens that not many boards use FLASHDEPS and it is not really possible to have FLASHDEPS depend on the ELFFILE. I think this is currently broken for adafruit-nrfutil boards, they wouldn't pass CI, I see them building but I didn't see them running which would trigger a rebuild of the .elf file in the test worker.

I think we need a generic solution, like having FLASHDEPS added to the BUILD_FILES and to TEST_EXTRA_FILES in Makefile.include instead of having flash: $(FLASHDEPS) dependency. I can try that manually for the esptool (not adding the flash dependencies to FLASHDEPS but instead add them to the other two variables) but it will be a problem for any programmer that needs dependencies that may depend on ELFFILE. If we don't have that generic solution then we need to document that method and say that FLASHDEPS may not depend on the ELFFILE.

@jeandudey jeandudey added Area: build system Area: Build system Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels May 14, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@maribu maribu self-assigned this Jan 5, 2023
@maribu
Copy link
Member

maribu commented May 17, 2023

make all flash-only -j

indeed doesn't work, as flash-only intentionally has no dependencies and will start in parallel with the building of the ELFFILE. IMO, this is acceptable.

make flash -j

Does work (at least with master) and builds fully parallel the flash file, then flashes it. IMO the build system works correctly here.

Please re-open, if you disagree.

@maribu maribu closed this as completed May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

No branches or pull requests

5 participants