-
Notifications
You must be signed in to change notification settings - Fork 2k
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
tests/riotboot: fix test running test in docker. #12446
Conversation
Can you take a look at this one? It should fix the tests. |
@fjmolinas I will look at it. Could you already run it on murdock to check the output? |
I think it will still compile the header and concatenated file while testing as it uses a different It would fix the issue anyway with |
Yes it will, but the hdr is always re-compiled from what I understand, because of the I was thinking of changing that as well, but it felt like a much bigger change to do before for a realease fix. |
I will try finding time this afternoon or so. |
do what? |
To review |
Thanks! |
Even if you changed all the |
>: ( |
I think I shouldn't have removed the elf-files from |
0fbd170
to
b3febae
Compare
There is one riotboot test that is not passing. But it seems to have been an issue wth a worker, the following did pass:
|
The test worked on my boards with the setup used for the initial issue and the release testing with I tested So on the code, adding the One conceptual question, would it make sense to still define the I will do a local testing of why |
Second thought I would also generate the ELF slots and so keep them in |
Do you want me to re-trigger a build to get the error message? |
@fjmolinas The murdock queue is empty so could be good yes. |
Can I squash right away? |
Yeah sure. It is easy to know what you changed. |
3ac7510
to
62dbe13
Compare
Hmm without docker it requires the |
I try to find out why. |
I am stupid, it was the line in the original comment. I did not realize it was only defined when not building in docker. |
As the test is compiling again, it will of course look at the dependencies of the So we need to send the |
62dbe13
to
4d898f8
Compare
The new version works The test from #12446 (comment)
|
The comment is clear enough for me, but it will be more the maintainer that reviews it that will have an opinion. |
@aabadie can you take a look at this one? |
I confirm this PR is fixing the mentioned issue. Comparing PR with master shows the testing procedure is fixed. |
- When running `BUILD_IN_DOCKER=1 make -C tests/riotboot test` new slot binaries (fw + hdr) need to be generated. `%.bin: %.elf` is no defined when building in docker, so the fw binaries $(SLOT_RIOT_ELFS:%.elf=%.bin) are added to BUILD_FILES
4d898f8
to
57b09f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
I added the Needs backport label, since it's marked as a bug. |
Please provide a backport @fjmolinas. |
Backport provided in #12499 |
Contribution description
This PR fixes
riotboot
test when building in docker.I'm not sure this is necessarily the best fix, since the issue actually comes from:
RIOT/Makefile.include
Lines 491 to 521 in 8a1e78b
So it comes from the fact that the following target is not defined when calling
make test
andBUILD_IN_DOCKER
is set.Although this is not actually a build instruction it does require the tool-chain.
I think the proposed solution might be better since it is not very intrusive and can be easily introduced and backported without changing the default handling of for
BUILD_IN_DOCKER
. I think the handling itself is flaudI also think this should be back-ported.
Testing procedure
As reported in #12003, the following command fails in master:
master
This PR
Issues/PRs references
Related to #12003