Skip to content

makefiles: select default prng _after_ recursive dependency resolution #21060

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 3, 2024
Merged

makefiles: select default prng _after_ recursive dependency resolution #21060

merged 2 commits into from
Dec 3, 2024

Conversation

mguetschow
Copy link
Contributor

Contribution description

The PRNG is supposed to be set only if there is none selected at the end of the recursive dependency resolution. However, this is currently not the case: a transitive dependency setting the PRNG (encountered with psa_crypto which selects prng_sha256prng) will lead to a user-facing dependency clash.

The first commit extends tests/build_system/external_module to showcase the issue and the second one fixes it by moving the automatic prng selection to defaultmodules_deps.inc.mk.

Disclaimer: I'm not sure if this is the right approach to follow, but it seems to work. Any suggestions from people with more insights on the build system are welcome.

Testing procedure

with only the first commit:

$ make -C tests/build_system/external_module_dirs info-modules
make: Entering directory '/home/mikolai/TUD/Code/RIOT-build/tests/build_system/external_module_dirs'
Currently the following prng modules are used: prng_musl_lcg prng_xorshift
/home/mikolai/TUD/Code/RIOT-build/sys/random/Makefile.include:14: *** Only one implementation of PRNG should be used..  Stop.
make: Leaving directory '/home/mikolai/TUD/Code/RIOT-build/tests/build_system/external_module_dirs'

with both commits:

$ make -C tests/build_system/external_module_dirs info-modules
make: Entering directory '/home/mikolai/TUD/Code/RIOT-build/tests/build_system/external_module_dirs'
auto_init
auto_init_random
board
board_common_init
core
core_idle_thread
core_init
core_lib
core_msg
core_panic
core_thread
cpu
external_module
libc
luid
native_drivers
periph
periph_common
periph_cpuid
periph_gpio
periph_gpio_linux
periph_hwrng
periph_init
periph_init_cpuid
periph_init_gpio
periph_init_gpio_linux
periph_init_hwrng
periph_init_led0
periph_init_led1
periph_init_led2
periph_init_led3
periph_init_led4
periph_init_led5
periph_init_led6
periph_init_led7
periph_init_leds
periph_init_pm
periph_init_uart
periph_pm
periph_uart
preprocessor
preprocessor_successor
prng
prng_xorshift
random
stdio_native
sys
transitive
make: Leaving directory '/home/mikolai/TUD/Code/RIOT-build/tests/build_system/external_module_dirs'

@mguetschow mguetschow requested review from benpicco and maribu December 2, 2024 16:25
@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: build system Area: Build system labels Dec 2, 2024
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 2, 2024
maribu
maribu approved these changes Dec 2, 2024
View reviewed changes
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.

lgtm.

If you would reorder the commits (first the fix than the test) every commit would paas the CI, which is nice for bisecting. But that is purely optional.

maribu
maribu approved these changes Dec 2, 2024
View reviewed changes
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.

lgtm.

If you would reorder the commits (first the fix than the test) every commit would paas the CI, which is nice for bisecting. But that is purely optional.

@mguetschow mguetschow force-pushed the make-random-prng-default branch from 71fbb66 to d5783d7 Compare December 3, 2024 09:29
@mguetschow
Copy link
Contributor Author

Reordered the commits as proposed :)

@mguetschow mguetschow force-pushed the make-random-prng-default branch from d5783d7 to 9f2755a Compare December 3, 2024 09:36
@riot-ci
Copy link

riot-ci commented Dec 3, 2024

Murdock results

✔️ PASSED

9f2755a tests/build_system/external_module: custom prng in transitive dep

Success Failures Total Runtime
10249 0 10249 17m:58s

Artifacts

@maribu maribu added this pull request to the merge queue Dec 3, 2024
Merged via the queue into RIOT-OS:master with commit 598d575 Dec 3, 2024
25 checks passed
@mguetschow mguetschow deleted the make-random-prng-default branch December 3, 2024 14:57
@mguetschow
Copy link
Contributor Author

Thank you!

@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@benpicco benpicco Awaiting requested review from benpicco

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: doc Area: Documentation Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2025.01
Development

Successfully merging this pull request may close these issues.
4 participants
@mguetschow @riot-ci @maribu @MrKevinWeiss