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

lib.sh: disable logging with SOF_LOGGING=none env or missing .ldc #813

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Dec 1, 2021

Some configurations don't support the logger. Others do but do not want
to run it for various reasons.

Supersedes #811, see earlier discussions there.

Signed-off-by: Marc Herbert [email protected]

Some configurations don't support the logger. Others do but do not want
to run it for various reasons.

Supersedes thesofproject#811, see earlier discussions there.

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb marc-hb changed the title lib.sh: disable logging with SOF_LOGGING=false env or missing .ldc lib.sh: disable logging with SOF_LOGGING=none env or missing .ldc Dec 1, 2021
@marc-hb marc-hb marked this pull request as ready for review December 1, 2021 02:22
@marc-hb marc-hb requested a review from a team as a code owner December 1, 2021 02:22
@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 1, 2021

Need to fix the commit message.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 1, 2021

https://sof-ci.01.org/softestpr/PR813/build934/devicetest has three known and unrelated issues: ADL NTP sync / too fast boot + very recent (5.6) "preemptible" regression thesofproject/linux#3283 + some rtcwake timeout
EDIT: newer https://sof-ci.01.org/softestpr/PR813/build936/devicetest/ with no code difference has only failure 3283

To really test this in CI (in addition to extensive testing locally) I hardcoded SOF_LOGGING=none in throw-away PR #814. The results in https://sof-ci.01.org/softestpr/PR814/build935/devicetest/ look good: all tests passed or skipped without any FW logs - except for the (very special/different) sof-logger test which passed with FW logs.

# Some firmware/OS configurations do not support logging.
ldcFile=$(find_ldc_file) || {
dlogi '.ldc dictionary file not found, SOF logs collection disabled'
return 0 # 0 is 'true'
Copy link
Contributor

@greg-intel greg-intel Dec 1, 2021

Choose a reason for hiding this comment

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

Not sure this comment is needed, as the script self documents on the line before. Or the comment can be on the line before the function definition, to clarify the outputs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, it's overkill. I will remove but only if someone requests some other changes because it does do not much harm either and uses very little real estate.

fi

# ... across all tests at once.
# In the future we should support SOF_LOGGING=etrace (only), see
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need more clarification. In future we will support only etrace? why? slogger seems useful to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #726 referenced on the next line.

Copy link
Collaborator

@fredoh9 fredoh9 left a comment

Choose a reason for hiding this comment

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

In general, I like this idea. logger_disabled() has 3 options to support

  1. no ldc file in the system
  2. OPT_VAL['s'] == 0
  3. $SOF_LOGGING == none

Copy link
Collaborator Author

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Thanks for the quick reviews. As this (and #811 before it) is eagerly waited, I will merge in 6-7 hours if no one requests any change. Sorry for the short notice (not so short when including earlier discussions in #811)

fi

# ... across all tests at once.
# In the future we should support SOF_LOGGING=etrace (only), see
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #726 referenced on the next line.

# Some firmware/OS configurations do not support logging.
ldcFile=$(find_ldc_file) || {
dlogi '.ldc dictionary file not found, SOF logs collection disabled'
return 0 # 0 is 'true'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, it's overkill. I will remove but only if someone requests some other changes because it does do not much harm either and uses very little real estate.

@marc-hb marc-hb merged commit 982f92d into thesofproject:main Dec 2, 2021
@marc-hb marc-hb deleted the no-logger branch December 2, 2021 04:50
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.

4 participants