Skip to content

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

Merged
maribu merged 1 commit intoRIOT-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 intoRIOT-OS:masterfrom
leandrolanzieri:pr/makefiles/info-global/remove_dependency_cache

Conversation

@leandrolanzieri
Copy link
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
Copy link
Member

maribu commented May 25, 2020

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
Copy link
Contributor Author

leandrolanzieri commented May 25, 2020

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
Copy link
Member

maribu commented May 25, 2020

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.

Copy link
Member

@maribu maribu left a comment

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
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
Copy link
Member

maribu commented May 25, 2020

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
Contributor

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

@kaspar030
Copy link
Contributor

does this need a backport? @leandrolanzieri

@leandrolanzieri
Copy link
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
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