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

kconfig: add two scripts to update distro config to sof and sof-dev #68

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

plbossart
Copy link
Member

To enable SOF on a platform, it may be easier to start from the
existing distro configurations.

kconfig-distro-sof-update.sh adds the minimal SOF options needed
kconfig-distro-sof-dev-update.sh: adds the SOF developer options

Most users would need the first script only.

Signed-off-by: Pierre-Louis Bossart [email protected]

@plbossart
Copy link
Member Author

@nklayman can you give this a try?

@plbossart plbossart force-pushed the fix/update-config-scripts branch 4 times, most recently from 3f15255 to e88bc6d Compare February 9, 2022 23:25
@plbossart plbossart requested review from nklayman and marc-hb February 9, 2022 23:27
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.

In decreasing importance order:

https://mywiki.wooledge.org/Quotes

I'm Too Lazy to Read, Just Tell Me What to Do
When in doubt, double-quote every expansion in your shell commands.

$KCONFIG_DIR/hdaudio-codecs-defconfig \
$KCONFIG_DIR/lock-stall-defconfig \
$KCONFIG_DIR/soundwire-defconfig \
$@
Copy link

Choose a reason for hiding this comment

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

Especially this one must be quoted

Suggested change
$@
"$@"

KCONFIG_DIR=$(dirname ${BASH_SOURCE[0]})
echo $KCONFIG_DIR

. $KCONFIG_DIR/kconfig-lib.sh
Copy link

Choose a reason for hiding this comment

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

Is this were $COMMAND comes from? If yes a very brief comment would be useful.

@plbossart
Copy link
Member Author

In decreasing importance order:

  • Add set -e at the top of every script so it stops immediately on error
    thesofproject/sof-test#312
  • These scripts are missing quotes on almost every line. Simply run shellcheck and will tell you where they are missing. Pro tip: if your IDE understands gcc errors and lets you click on them, then shellcheck -f gcc offers the same.

https://mywiki.wooledge.org/Quotes

I'm Too Lazy to Read, Just Tell Me What to Do
When in doubt, double-quote every expansion in your shell commands.

it's a copy of scripts written by others and that are used EVERY DAY by our github workflow. I am not a shell expert and if you want changes patches are welcome for all the scripts in this project.

To enable SOF on a platform, it may be easier to start from the
existing distro configurations.

kconfig-distro-sof-update.sh adds the minimal SOF options needed
kconfig-distro-sof-dev-update.sh: adds the SOF developer options

Most users would need the first script only.

To speed-up compilation time, the scripts rely on 'make
localmodconfig', which assumes that all required modules are loaded
beforehand.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
@plbossart plbossart force-pushed the fix/update-config-scripts branch from e88bc6d to c68d8ca Compare February 10, 2022 19:57
@plbossart plbossart requested a review from nklayman February 10, 2022 19:58
@marc-hb
Copy link

marc-hb commented Feb 10, 2022

it's a copy of scripts written by others

Sorry I thought it was something new based on the commit message. I agree consistency is important.

Please add at least set -e at the top, it's not breaking consistency and it really helps when anything fails.

Recommended by Marc Herbert:
Add set -e at the top of every script so it stops immediately on error

Signed-off-by: Pierre-Louis Bossart <[email protected]>
@plbossart
Copy link
Member Author

it's a copy of scripts written by others

Sorry I thought it was something new based on the commit message. I agree consistency is important.

Please add at least set -e at the top, it's not breaking consistency and it really helps when anything fails.

done

@plbossart plbossart requested a review from marc-hb February 10, 2022 20:09
@plbossart plbossart merged commit 9bba13a into thesofproject:master Feb 15, 2022
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.

3 participants