Skip to content

makefiles/info-global: Remove dependency resolution cache - #14132

Merged
maribu merged 1 commit into
RIOT-OS:masterfrom
leandrolanzieri:pr/makefiles/info-global/remove_dependency_cache
May 25, 2020
Merged

makefiles/info-global: Remove dependency resolution cache#14132
maribu merged 1 commit into
RIOT-OS:masterfrom
leandrolanzieri:pr/makefiles/info-global/remove_dependency_cache

Conversation

@leandrolanzieri

Copy link
Copy Markdown
Contributor

Contribution description

This removes a preliminar dependency resolution that is performed without including features, to skip a full dependency resolution when boards can be proven to be unsupported on an earlier stage.

This was introducing issues on some boards since the blacklisting of some features depends on board information which is not available at the time of performing this resolution.

Some time measurements

Running make info-boards-supported 50 times on my computer results in:

Application Time with cache Time without cache
hello_world 873.0 ms ± 20.5 ms 851.7 ms ± 26.3 ms
tests/driver_ws281x (proposed here) 266.4 ms ± 11.6 ms 263.0 ms ± 18.6 ms

Testing procedure

  • make info-boards-supported should work properly now (it was pointed out in the mailing list that the sodaq boards are not showing up in the list in current master, for instance)

Issues/PRs references

Spotted by @fjmolinas here

This removes a preliminar dependency resolution that is performed
without including features, to skip a full dependency resolution when
boards can be proven to be unsupported on an earlier stage.

This was introducing issues on some boards since the blacklisting of
some features depends on board information which is not available at the
time of performing this resolution.
@leandrolanzieri leandrolanzieri added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 25, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.07 milestone May 25, 2020
@maribu

maribu commented May 25, 2020

Copy link
Copy Markdown
Member

Is it correct, that it is faster without caching? Or is this columns mixed up?

In any case, having this working is a higher priority than having it fast. So even if it would slow things down, we obviously have to accept this.

@leandrolanzieri

leandrolanzieri commented May 25, 2020

Copy link
Copy Markdown
Contributor Author

Is it correct, that it is faster without caching? Or is this columns mixed up?

No, the columns are Ok. When running the benchmark, without the cache took less time.

@maribu

maribu commented May 25, 2020

Copy link
Copy Markdown
Member

OK, with this the following additional boards are listed in examples/hello-world:

  • adafruit-cluearduino-mkr1000
  • arduino-mkrfox1200
  • arduino-mkrwan1300
  • arduino-mkrzero
  • feather-m0
  • nrf52840dongle
  • serpente
  • sodaq-autonomo
  • sodaq-explorer
  • sodaq-one
  • sodaq-sara-aff

I compiled it for each of these, it worked for all.

@maribu maribu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK

@maribu
maribu merged commit 4adb8ab into RIOT-OS:master May 25, 2020
@leandrolanzieri

Copy link
Copy Markdown
Contributor Author

Thanks for the quick review.

@leandrolanzieri
leandrolanzieri deleted the pr/makefiles/info-global/remove_dependency_cache branch May 25, 2020 09:59
@maribu

maribu commented May 25, 2020

Copy link
Copy Markdown
Member

When running the benchmark, without the cache took less time.

Then it is extra good to have this in :-)

(I'm sure it was noticeably faster with the cache on my machine at the time I added this. I wonder why it turns out slowing down things now.)

@fjmolinas

Copy link
Copy Markdown
Contributor

@keestux the issue you mentioned on the mailing list should be fixed now with this PR.

@kaspar030

Copy link
Copy Markdown
Contributor

does this need a backport? @leandrolanzieri

@leandrolanzieri

Copy link
Copy Markdown
Contributor Author

does this need a backport? @leandrolanzieri

Hmm yes, you're right

@leandrolanzieri leandrolanzieri added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label May 25, 2020
@leandrolanzieri

Copy link
Copy Markdown
Contributor Author

Backport provided in #14136

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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants