Skip to content
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

[Backport v3.5-branch] scripts: check_init_priorities: usability updates #64772

Closed
wants to merge 2 commits into from

Conversation

zephyrbot
Copy link
Collaborator

@zephyrbot zephyrbot commented Nov 3, 2023

Backport dd178ce~2..dd178ce from #64486.

Fixes #64386

Since bb590b5 introduced ordinals in the priority sequence, the "same
priority" case cannot happen anymore, furthermore the priority value in
the script is now the position of the function in the init sequence, so
if two devices have the same priority there's something real bad going
on.

Drop all the "same priority" handling code and tests, convert the case
into ane exception instead. Drop the init stubs as well from the test,
they are not required anymore.

Signed-off-by: Fabio Baltieri <[email protected]>
(cherry picked from commit 2a70c31)
The current error messages are a bit cryptic, rework them to make them
more meaningful:

- add an extra message on the first error to explain what the errors
  refer to.
- rework the error message to be more explicit.
- rework the priority string print to use a LEVEL+offset format to
  somehow highlight that the number is the offset from the level, not
  the actual priority.
- print the init function name in addition to the devicetree path.

Signed-off-by: Fabio Baltieri <[email protected]>
(cherry picked from commit dd178ce)
@henrikbrixandersen
Copy link
Member

Hi @fabiobaltieri! We usually only backport bugfixes, but #64386 is an enhancement request?

@fabiobaltieri
Copy link
Member

Hi @fabiobaltieri! We usually only backport bugfixes, but #64386 is an enhancement request?

Yeah this is a bit unconventional, not a bugfix per se but since this makes build failure message clearer and there's a fairly high chances downstream users are going to hit this while upgrading, I think it may be worth backporting anyway.

What do you folks think? I'm fine with dropping it if anyone feels strongly about it.

@fabiobaltieri fabiobaltieri assigned kartben and unassigned tejlmand Nov 3, 2023
@fabiobaltieri
Copy link
Member

Assigning to @kartben since he filed the original issue.

@nordicjm
Copy link
Collaborator

nordicjm commented Nov 3, 2023

From the original PR:

Change looks okay, I do not think it should be backported to 3.5 though, it removes a Kconfig (even if the Kconfig does nothing it still removes it) and changes output

Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

Ya, leaning towards -1 myself. It's a bit too much of a change, and hopefully the status quo in 3.5 is not too terrible for users.

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

this is an enhacement, not a bug fix.

@fabiobaltieri
Copy link
Member

Cool by me! Thanks for the feedback.

@nashif nashif deleted the backport-64486-to-v3.5-branch branch February 13, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: Kconfig Backport Backport PR and backport failure issues
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants