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

Add new local_config.bash files to solve device-specific issues #1234

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Sep 23, 2024

Source additional files /etc/sof/local_config.bash and
sof-test/case-lib/local_config.bash.

This helps solving device-specific issues like #1233 and many others
before that - for instance git blame the "ignore_str" variable in
sof-kernel-log-check.sh

No functional change.

- Get rid of the "if $ignore_str" condition because it's not going to be
  empty anytime soon. It will be a very good problem to have when it is!
  Then it will be easy to turn it into an array like in the next commit.

- Don't check whether the $err string is empty but use the `grep` exit
  status directly.

Signed-off-by: Marc Herbert <[email protected]>
Source additional files /etc/sof/local_config.bash and
sof-test/case-lib/local_config.bash.

This helps solving device-specific issues like thesofproject#1233 and many others
before that - for instance git blame the "ignore_str" variable in
sof-kernel-log-check.sh

Signed-off-by: Marc Herbert <[email protected]>
Leverage the new local_config.bash files to implement device-specific
excludes.

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb marc-hb requested a review from a team as a code owner September 23, 2024 23:01
@marc-hb marc-hb changed the title Local config Add new local_config.bash files to solve device-specific issues Sep 23, 2024
@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 24, 2024

Expected failures.

Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Looks good @marc-hb . I have some concerns, but I think they are more about the usage of this feature. Probably this should be merged to give the option, but just like to record my concern as a comment.

# - can be arbitrarily complex. Best avoided but only on specific systems anyway.
# shellcheck disable=SC2154
if &>/dev/null declare -p sof_local_extra_kernel_ignores; then
dlogw "Ignoring extra errors on this particular system:"
Copy link
Contributor

Choose a reason for hiding this comment

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

One concern I have with this facility is that when adding local exceptions becomes easier, it can be used too much and people reading the test logs might not realize some DUTs have local exceptions others don't have. The global ignore list has drawbacks, but at least one needs to make a git commit with proper description and leave something that can be search through git history. OTOH, this dlogw does make local exception use clear in the log, which alleviates my concern. It really depends on how this will be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review!

This is a risk but historically, the abuse has been the other way around: the global list has been abused to add issues with "personal" devices. Just look at the git blame, log and past PRs. I expect the existence of this new feature to be missed and the existing abuse to continue :-) Also, it may seem quick to edit files on one device but what 5 or 6?

Both situations exist, I really think the code should have offered both possibilities a long time ago...

@marc-hb marc-hb merged commit 83f5e41 into thesofproject:main Oct 8, 2024
5 of 7 checks passed
@marc-hb marc-hb deleted the local_config branch October 8, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants