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.

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

Extend kconfig #93

Merged
merged 5 commits into from
Apr 29, 2024
Merged

Extend kconfig #93

merged 5 commits into from
Apr 29, 2024

Conversation

plbossart
Copy link
Member

follow-up to PR #92 with additions this time.

@plbossart
mach-driver-defconfig: build SOF_DA7219
913a780 
Big miss in our test configuration...

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
amd-defconfig: add missing platforms
5a2dd8f 
Again yet another miss in our compilation test coverage.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
base-defconfig: move top-level options to sof-defconfig and sof-dev-d…
ac25b59 
…efconfig

DYNAMIC_DEBUG is really a requirement for all SOF uses, that's how we
detect issues with sof-test scripts.

The rest is more for developers.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
soundwire-defconfig: split codecs so soundwire-codecs-defconfig
2821509 
This split allows for codecs to be selected only on the basis of
machine drivers selected, or conversely to be all included for
developers.

kconfig-sof-default-nodev.sh now includes only the bare minimum for
SOF.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
marc-hb
marc-hb reviewed Apr 24, 2024
View reviewed changes
Copy link

@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.

Can you add the new script/subset to the README.md?

Also, to be on the safe side please at least one run of daily tests between merging this PR and the previous one.

A bit of a lost cause but since there is a brand new .sh file it wouldn't hurt to make it shellcheck clean.

Off-topic: most .sh have basically the same boilerplate code + just a list of defconfigs. In the future it could be nice to have a more generic.sh script that can read new *-deconfig.list files instead. It would make custom subsets easier. No big deal.

@plbossart
Copy link
Member Author

The README is 3 years old, it should be redone in a separate PR.

The tests were done already with the initial version.

I don't know how to make shell scripts pretty/better. I don't have any knowledge on what is legal/dangerous/simpler...

@marc-hb
Copy link

marc-hb commented Apr 24, 2024

I don't know how to make shell scripts pretty/better. I don't have any knowledge on what is legal/dangerous/simpler...

Shellcheck often does it for you. In this case just missing a lot of quotes (typical)

@plbossart
Copy link
Member Author

I don't know how to make shell scripts pretty/better. I don't have any knowledge on what is legal/dangerous/simpler...

Shellcheck often does it for you. In this case just missing a lot of quotes (typical)

or just cryptic errors

In kconfig-sof-default.sh line 8:
. "$KCONFIG_DIR"/kconfig-lib.sh
  ^---------------------------^ SC1091 (info): Not following: ./kconfig-lib.sh was not specified as input (see shellcheck -x).

marc-hb
marc-hb reviewed Apr 24, 2024
View reviewed changes
Copy link

@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.

Since you're testing LNL, could you also add CONFIG_DRM_XE?

PS: thanks for the shellcheck fixes. I was only suggesting them for the new .sh file, now it's drowing the Kconfig changes... separate, concurrent PR?

kconfig-lib.sh
@@ -1,20 +1,21 @@
#!/bin/bash
Copy link

@marc-hb marc-hb Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this file is only meant to be sourced, not invoked directory so I wouldn't do this.

shellcheck is like checkpatch: it's right most of the time but all the time.

There are ways to silence specific warnings but since shellcheck is not run in CI in this repo then I wouldn't bother.

kconfig-lib.sh Outdated

BUILD_DIR=$(pwd)

# find merge_config in code directory
FILE=scripts/kconfig/merge_config.sh
if test -f "$FILE"; then
COMMAND=$FILE;
export COMMAND=$FILE;
Copy link

Choose a reason for hiding this comment

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

Again I believe this file is meant to be sourced so I don't think this export makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

if I don't use export shellcheck complains that the variable is not used.

In kconfig-lib.sh line 18:
            COMMAND=$SOURCE_DIR/$FILE;
            ^-----^ SC2034 (warning): COMMAND appears unused. Verify use (or export if used externally).

what the heck.

Copy link

Choose a reason for hiding this comment

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

Ideally shellcheck would analyze this file only indirectly when sourced from another file - exactly like how it's being used.

In practice just ignore this warning.

@marc-hb
Copy link

marc-hb commented Apr 24, 2024 •
edited
Loading

or just cryptic errors

Yes following shell "includes" is a bit painful with shellcheck, you need -x and maybe a special # shellcheck source line. See how it's done in sof-test or simpler: ignore those warnings for now - this is (was?) a Kconfig PR ! I was merely suggesting to add quotes to the brand new .sh file, not to make all scripts shellcheck-clean in this repo and certainly not in this PR.

@fredoh9
Copy link

fredoh9 commented Apr 25, 2024

started device tests

  • pr_main_ace : planresultdetail/40302
  • pr_main_cavs : planresultdetail/40303
  • pr_stable_v2_2 : planresultdetail/40305

@plbossart
kconfig*.sh: fix shellcheck errors
abd21d3 
used double-quotes and export

tested with: "shellcheck kconfig-*.sh"

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart requested a review from marc-hb April 25, 2024 22:24
marc-hb
marc-hb reviewed Apr 26, 2024
View reviewed changes
Copy link

@marc-hb marc-hb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good but I'm not familiar with these + the README.md is 3 years old.

Still no CONFIG_DRM_XE?

EDIT: just noticed yesterday's commit 3b1d9da

@plbossart plbossart merged commit 8fee06f into thesofproject:master Apr 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marc-hb marc-hb marc-hb left review comments

@ujfalusi ujfalusi ujfalusi approved these changes

@bardliao bardliao bardliao approved these changes

@fredoh9 fredoh9 Awaiting requested review from fredoh9

@vijendarmukunda vijendarmukunda Awaiting requested review from vijendarmukunda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@plbossart @marc-hb @fredoh9 @bardliao @ujfalusi