Skip to content

mcuboot.mk: Add mcuboot targets to BUILDDEPS.#10492

Merged
cladmi merged 1 commit intoRIOT-OS:masterfrom
jcarrano:mcuboot-builddeps-fix
Dec 5, 2018
Merged

mcuboot.mk: Add mcuboot targets to BUILDDEPS.#10492
cladmi merged 1 commit intoRIOT-OS:masterfrom
jcarrano:mcuboot-builddeps-fix

Conversation

@jcarrano
Copy link
Copy Markdown
Contributor

@jcarrano jcarrano commented Nov 28, 2018 •
edited by cladmi
Loading

Read this before reviewing

The first two commits are taken from #10461 and are not part of this PR. They are there just to force an error.

Contribution description

The mcuboot targets were not being added to BUILDDEPS and so the mcuboot targets was being made simultaneous with the clean. See the failed murdock output from #10461.

Testing procedure

In tests/mcuboot run

BUILD_IN_DOCKER=1 DOCKER="sudo docker" make clean all

EDIT: the test command is running this merged with #10461

make -C tests/mcuboot clean all -j

Issues/PRs references

This is one of the issues derived from #10459 .

@jcarrano jcarrano requested a review from cladmi November 28, 2018 09:47
This was referenced Nov 28, 2018
Merged
Closed
@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: OTA Area: Over-the-air updates Area: build system Area: Build system labels Nov 30, 2018
@cladmi cladmi added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Dec 4, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 4, 2018

I added the needs squashing label to prevent merging before removing the test commits.

@cladmi cladmi added this to the Release 2019.01 milestone Dec 4, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 4, 2018

The test is not enough to show the issue with only the reference commits. I used this one instead:

make -C tests/mcuboot clean all -j

And with it this does not currently fix it.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 4, 2018

The issue is that BUILDDEPS is used here for clean

RIOT/Makefile.include

Line 486 in 90db0bf

all $(BASELIBS) $(USEPKG:%=$(RIOTPKG)/%/Makefile.include) $(BUILDDEPS): clean

and mcuboot.inc.mk is included here:

RIOT/Makefile.include

Lines 720 to 721 in 90db0bf

# include mcuboot support
include $(RIOTMAKE)/mcuboot.mk

The last line should be moved before. I would imagine after processing all modules like after

RIOT/Makefile.include

Line 362 in 90db0bf

-include $(EXTERNAL_MODULE_DIRS:%=%/Makefile.include)

The problem was already here before.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 4, 2018

It really follows the big issue that variables should be defined first and rules after.

@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented Dec 4, 2018

@cladmi push the fix to my branch,

@cladmi cladmi mentioned this pull request Dec 4, 2018
Open
6 tasks
cladmi
cladmi suggested changes Dec 4, 2018
View reviewed changes
makefiles/mcuboot.mk Outdated

export IMAGE_HDR_SIZE ?= 512

BUILDDEPS += $(MCUBOOT_KEYFILE) $(MCUBOOT_BIN) mcuboot
Copy link
Copy Markdown
Contributor

@cladmi cladmi Dec 4, 2018

Choose a reason for hiding this comment

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

However, we discussed it IRL, none of these files should be declared as BUILDDEPS they are not build dependencies, they just need to be done after clean if it is there.

The reason being that they are in the BINDIR directory that can be deleted.

I will propose a pre-first fix to go in the direction of adding this BINDIR variable and target.

@cladmi cladmi added the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 4, 2018
@jcarrano jcarrano mentioned this pull request Dec 5, 2018
Closed
4 tasks
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 5, 2018

After some more discussions and verification, putting a directory as target dependency forces rebuild whenever a file is put inside. The correct way is to declare it as order-only target. But order-only targets and clean at the same time cannot work properly, for good reasons.
There could be a different fix with a file dependency, but it is a different task. So now, I just applied the same handling there too.

I added an updated commit message in the squash! commit body.

My tests commands were verifying the creation commands are indeed done after clean for both following commands:

make -C tests/mcuboot clean all -j

and

make -C tests/mcuboot clean mcuboot-flash -j

@jcarrano jcarrano force-pushed the mcuboot-builddeps-fix branch from 7aa7eda to 221f719 Compare December 5, 2018 15:32
@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented Dec 5, 2018

Squashed and removed the test commits.

@jcarrano jcarrano removed State: waiting for other PR State: The PR requires another PR to be merged first CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Dec 5, 2018
@cladmi cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 5, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 5, 2018 •
edited
Loading

The squashed message is wrong as it still references BUILDDEPS. I do not have my previous commit however… EDIT it is there 49687c0#diff-b2237d11e56a16c26885ccf16af19669

@jcarrano jcarrano force-pushed the mcuboot-builddeps-fix branch from 221f719 to 356a41b Compare December 5, 2018 15:44
@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented Dec 5, 2018

I amended the commit message.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 5, 2018

Commit title still references BUILDDEPS.

@jcarrano jcarrano force-pushed the mcuboot-builddeps-fix branch from 356a41b to 46da35e Compare December 5, 2018 15:48
@jcarrano
mcuboot.mk: make mcuboot targets depend on clean.
46da35e 
When doing `make -j clean all' the directories can be cleaned after
files are made. To ensure files are created after clean, those targets
are made conditionally dependent on the clean target.

This copies the handling done in Makefile.include.
cladmi
cladmi approved these changes Dec 5, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

ACK I re-tested the squashed version with #10461 and the test commands from #10492 (comment) and it is working as expected

You can merge when murdock is happy

@cladmi cladmi merged commit fb65d37 into RIOT-OS:master Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cladmi cladmi cladmi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: OTA Area: Over-the-air updates CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2019.01

Development

Successfully merging this pull request may close these issues.

3 participants

@jcarrano @cladmi @miri64