scripts: xtensa-build-all.sh: Refactor and improve script by crazoes · Pull Request #2711 · thesofproject/sof

crazoes · 2020-04-04T09:25:32Z

Use getopts for simpler, robust argument parsing
Exit if an unknown platform is specified
Derive $ROOT from $HOST instead of explicit assignment
Use switch case instead of if else for testing string

Signed-off-by: Shreeya Patel shreeya.patel23498@gmail.com

crazoes · 2020-04-04T09:26:33Z

Furthers #2631

crazoes · 2020-04-04T11:08:08Z

Jenkins probably uses ./scripts/xtensa-build-all.sh -j $PLATFORM as well, although can't seem to find any jenkinsfile in the repo.

lgirdwood · 2020-04-05T18:49:09Z

SOFCI TEST

marc-hb · 2020-04-06T18:49:51Z

I have also seen this typo, but since we are just now refactoring the old code this can be done in a separate PR. But now that you pointed it I think we can add a separate patch in this PR.
@crazoes Remeber that in a PR like this fixes come first, then the actual refactoring.

I like to treat typos the same as indentation/whitespace: don't obscure actual changes with them. However if that line changes ANYWAY in git for some other and good reason then it's a good opportunity to fix typos at the same time.

@marc-hb @dbaluta I have maintained the order here, fixing first and then moving the code around.
In the process I have noticed that when we order moving of code after fixes, we will always end up with merge conflicts after editing the fix commit. So what is the rationale behind ordering fixes first and refactors later?

marc-hb · 2020-04-06T19:19:09Z

These are associative arrays in disguise. Maybe we could add a dependency on bash version 4 in the future (not in this PR which is already fixing ~~many~~ other things).

Yeah, I just checked out associative arrays, this whole thing can be made into a nested associative array, with keys platform names, and values as another associative array containing PLATFORM, ARCH, etc..

marc-hb

Using PLATFORMS[@] as an array when it's not is really bad and should be fixed and tested very first thing before refactoring anything[*]. Either make PLATFORMS an actual array and fix all the missing quotes, or make it a string with whitespace. Please not a bit of both any more. I suspect actually making it an array means fewer changed lines. It's a bit overkill because I don't think platform names will ever whitespace in them but it's more consistent with SUPPORTED_PLATFORMS[@].

[*] Moving code that works by coincidence is like moving a large pile of glassware.

marc-hb · 2020-04-06T21:38:20Z

It's a miracle this works considering PLATFORMS is not an array. This works by combining two bugs that cancel each other out:
To list each element of the array it should be for platform in "${PLATFORMS[@]}" with double quotes
but PLATFORMS is not an array but a string with whitespace
https://mywiki.wooledge.org/BashGuide/Arrays
Demo, ./scripts/xtensa-build-all.sh byt ABC apl with this:
--- a/scripts/xtensa-build-all.sh+++ b/scripts/xtensa-build-all.sh@@ -90,6 +90,19 @@ else done fi +# Here's an actual array+declare -p SUPPORTED_PLATFORMS+printf 'SPi=%s\n' "${SUPPORTED_PLATFORMS[@]}"++# Not an array, just pretending+declare -p PLATFORMS+# yet bash expands this a string with whitespace, amazing+printf 'Pi=%s\n' "${PLATFORMS[@]}"+printf 'Pi=%s\n' "${PLATFORMS[@]}"++# now something even weirder+printf '?#?=%s\n' "${#PLATFORMS[@]}"+ # check target platform(s) have been passed in if [ ${#PLATFORMS[@]} -eq 0 ]; then @@ -97,6 +110,8 @@ then exit 1 fi +exit 0+ if [ "x$USE_PRIVATE_KEY" == "xyes" ]

crazoes · 2020-04-13T14:42:15Z

I don't know why but https://github.com/thesofproject/sof/pull/2711/commits doesn't show the right order of commit, rather ordered by dates. Making it difficult to read the actual sequence of the commits. I have checked the reviews again and verified I haven't missed any comment. However, there have been many amending and merge conflicts here so please let me know if I missed anything or if something might have lost due to wrongful merge conflict resolution on my part.

`PLATFORMS` variable was used as an array while it is actually a string used incorrectly making it work accidentally. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

marc-hb · 2020-04-13T17:07:53Z

I think you meant: if type xtensa-bxt-elf-gcc; then.
This explains the Travis failure.

Either way HOST is never set to xtensa-bxt-elf-gcc which is what is being set as the compiler. Currently unable to get the docker image because of network issues so cannot reproduce locally. Anyways, I will try removing the ! and see if it works
The CMAKE_C_COMPILER: xtensa-bxt-elf-gcc is not a full path and was not found in the PATH.

huh, this does fix it. But doesn't work well with what I said earlier. Maybe the CMAKE_C_COMPILER is not derived from TOOLCHAIN which is what is using the HOST value?

bxt and apl were technically different but have been used somewhat interchangeably in some contexts. If you look at commit c7edd73 you can see this logic is only backward compatibility to support who built a toolchain with the older bxt name. All SOF documentation now refers to apl

marc-hb · 2020-04-13T18:46:25Z

I don't know why but https://github.com/thesofproject/sof/pull/2711/commits doesn't show the right order of commit, rather ordered by dates. Making it difficult to read the actual sequence of the commits.
I have checked the reviews again and verified I haven't missed any comment. However, there have been many amending and merge conflicts here so please let me know if I missed anything or if something might have lost due to wrongful merge conflict resolution on my part.

Wow, I think you discovered a new github limitation with force pushes, thanks! I added it to the list:
zephyrproject-rtos/zephyr#14444 (comment)

In the process I have noticed that when we order moving of code after fixes, we will always end up with merge conflicts after editing the fix commit. So what is the rationale behind ordering fixes first and refactors later?

Yes, re-ordering commits that touch the same parts of the code will always cause painful conflicts. The rationale is that version history is sometimes painfully written once by one person (you), but then it's immutable and read, bisected, backported and used in many other ways by many other people and many tools. This "clean history" rationale is a very common rationale among git users, apparently much less common among github users, github seems to care less about history.

ordering fixes first and refactors later?

It's not always like this, it depends. In other cases a zero-functional change refactor is a prelude to shorten and clarify a bug fix coming immediately next and making the fix much easier to revert in case it regresses something else. In this PR the hopefully small fix(es) were completely unrelated to the refactor, so better not increase and obscure the distance between the bugs and their fixes by shuffling code in between.

PS: besides root-causing the Travis failure, I haven't reviewed the latest version yet.

Use getopts to parse the arguments instead of manual parsing to leave no room for errors, bugs, and inconsistent conventions introduced by custom implementation. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

Exits if an unknown platform is specified. Currently, there are no warnings or errors. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

Currently, "set -e" is being ignored for type command using redirections and "A && true". "A && true" is a no-op and a peculiar case where "set -e" ignores the exit code of A. Exit code of the "type" command is used in an if statement which is exactly what the function of "if" command is. Also, "set -e" ignores exit codes of commands used in if statements. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

Use switch case for testing strings instead of if statements for each case. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

Derive $ROOT from $HOST instead of explicit assignments. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

xtensa-build-all.sh expects number of cores to be used when -j option is specified. By default, it uses all the cores available which is what we want, hence, omitting the option altogether. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

lgirdwood · 2020-04-15T21:07:02Z

SOFCI TEST

xiulipan · 2020-04-16T03:45:39Z

@crazoes What the change about the -j option? Why you would like to remove it now?

crazoes · 2020-04-16T04:12:02Z

@crazoes What the change about the -j option? Why you would like to remove it now?

@xiulipan Explained in another discussion here : #2711 (comment)

xiulipan · 2020-04-16T07:31:06Z

@crazoes I will try to remove the -j option in our Jenkins CI scripts.

lgirdwood

@xiulipan do you need to update CI before or after this is merged ?

lgirdwood · 2020-04-21T13:55:49Z

CI known issues.

marc-hb · 2020-07-11T00:08:51Z

This PR missed one $j -> $platform replacement:

$ shellcheck scripts/xtensa-build-all.sh In scripts/xtensa-build-all.sh line 273:

case$platformin
...
TOOLCHAIN=$HOST
PATH=$pwd/../$HOST/bin:$OLDPATH
COMPILER="gcc"
case $j in
^-- SC2154: j is referenced but not assigned.
byt|cht|cnl|sue) DEFCONFIG_PATCH="_gcc";;
*) DEFCONFIG_PATCH="";;
esac

I think this means the wrong _defconfig files have been used for byt|cht|cnl|sue for the last 3- months?

(unrelated but large shellcheck noise reduced in unrelated PR #3163 )

crazoes · 2020-07-13T05:01:04Z

@marc-hb you are right. I will send a fix.

(Done in PR #3167)

crazoes requested a review from jajanusz as a code owner April 4, 2020 09:25

crazoes force-pushed the getopts-xtensa-build-all branch from 89ec368 to b4b3ae4Compare April 4, 2020 09:35

crazoes changed the title ~~scripts: xtensa-build-all.sh: Refactor and improve script~~[WIP] scripts: xtensa-build-all.sh: Refactor and improve scriptApr 4, 2020

crazoes requested review from dbaluta, lbetlej, lgirdwood, mmaka1, plbossart and tlauda as code owners April 4, 2020 10:03

crazoes changed the title ~~[WIP] scripts: xtensa-build-all.sh: Refactor and improve script~~scripts: xtensa-build-all.sh: Refactor and improve scriptApr 4, 2020

crazoes force-pushed the getopts-xtensa-build-all branch from 240c626 to 9b9370cCompare April 4, 2020 10:11

lgirdwood reviewed Apr 4, 2020
View reviewed changes

Comment thread.travis.yml Outdated

lgirdwood approved these changes Apr 5, 2020
View reviewed changes

dbaluta reviewed Apr 5, 2020
View reviewed changes

Comment threadscripts/xtensa-build-all.sh Outdated

crazoes force-pushed the getopts-xtensa-build-all branch from 9b9370c to 73a4cf9Compare April 6, 2020 14:23

dbaluta approved these changes Apr 6, 2020
View reviewed changes

marc-hb requested changes Apr 6, 2020
View reviewed changes

This was referenced Apr 6, 2020
scripts: build-all: warn when ignoring unknown arguments #2625
Merged
[IN DISCUSSION, NOT MERGE]case-lib: redirect nohup stdout and stderr to /dev/null thesofproject/sof-test#159
Closed

crazoes force-pushed the getopts-xtensa-build-all branch from 73a4cf9 to b029dedCompare April 13, 2020 14:19

monocodusBot approved these changes Apr 13, 2020
View reviewed changes

crazoes force-pushed the getopts-xtensa-build-all branch from b029ded to f2f0185Compare April 13, 2020 14:33

monocodusBot approved these changes Apr 13, 2020
View reviewed changes

crazoes force-pushed the getopts-xtensa-build-all branch from f2f0185 to ebce03dCompare April 13, 2020 14:45

monocodusBot approved these changes Apr 13, 2020
View reviewed changes

scripts: xtensa-build-all.sh: Make PLATFORMS a proper array
e6b37a0
`PLATFORMS` variable was used as an array while it is actually a string used incorrectly making it work accidentally. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

crazoes force-pushed the getopts-xtensa-build-all branch from ebce03d to dd29d2aCompare April 13, 2020 14:50

monocodusBot approved these changes Apr 13, 2020
View reviewed changes

marc-hb reviewed Apr 13, 2020
View reviewed changes

crazoes force-pushed the getopts-xtensa-build-all branch from dd29d2a to 53532dfCompare April 13, 2020 19:08

monocodusBot approved these changes Apr 13, 2020
View reviewed changes

crazoes added 6 commits April 14, 2020 01:00

scripts: xtensa-build-all.sh: Use getopts
f03615c
Use getopts to parse the arguments instead of manual parsing to leave no room for errors, bugs, and inconsistent conventions introduced by custom implementation. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

scripts: xtensa-build-all.sh: Exit if unknown platform specified
2f7aedb
Exits if an unknown platform is specified. Currently, there are no warnings or errors. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

scripts: xtensa-build-all.sh: Use switch case
b60aeb3
Use switch case for testing strings instead of if statements for each case. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

scripts: xtensa-build-all.sh: Derive $ROOT from $HOST
febd405
Derive $ROOT from $HOST instead of explicit assignments. Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

crazoes force-pushed the getopts-xtensa-build-all branch from 53532df to 6c1f2daCompare April 13, 2020 19:31

monocodusBot approved these changes Apr 13, 2020
View reviewed changes

marc-hb self-requested a review April 13, 2020 21:35

marc-hb approved these changes Apr 13, 2020
View reviewed changes

lgirdwood approved these changes Apr 16, 2020
View reviewed changes

crazoes mentioned this pull request Apr 21, 2020
Zephyr API bridge #2813
Merged

lgirdwood merged commit 38af37a into thesofproject:masterApr 21, 2020

marc-hb requested a review from ranj063 July 11, 2020 00:08

marc-hb mentioned this pull request Jul 11, 2020
remove SRC for Baytrail and CherryTrail #3080
Merged

crazoes mentioned this pull request Jul 13, 2020
xtensa-build-all: Fix missed $j -> $platform renaming #3167
Merged

Uh oh!

Conversation

crazoes commented Apr 4, 2020

Uh oh!

crazoes commented Apr 4, 2020

Uh oh!

crazoes commented Apr 4, 2020

Uh oh!

Uh oh!

lgirdwood commented Apr 5, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marc-hbApr 6, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-hb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marc-hbApr 6, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazoes commented Apr 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hbApr 13, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Apr 13, 2020

Uh oh!

lgirdwood commented Apr 15, 2020

Uh oh!

xiulipan commented Apr 16, 2020

Uh oh!

crazoes commented Apr 16, 2020

Uh oh!

xiulipan commented Apr 16, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Apr 21, 2020

Uh oh!

marc-hb commented Jul 11, 2020

Uh oh!

crazoes commented Jul 13, 2020• edited by marc-hb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

marc-hbApr 6, 2020•
edited
Loading

marc-hb left a comment •
edited
Loading

marc-hbApr 6, 2020•
edited
Loading

marc-hbApr 13, 2020•
edited
Loading

crazoes commented Jul 13, 2020•
edited by marc-hb
Loading