Skip to content

Shellcheck check xrun injection - #1058

Merged
marc-hb merged 2 commits into
thesofproject:mainfrom
greg-intel:shellcheck-check-xrun-injection
Jun 26, 2023
Merged

marc-hb merged 2 commits into
thesofproject:mainfrom
greg-intel:shellcheck-check-xrun-injection

Conversation

@greg-intel

Copy link
Copy Markdown
Contributor

Various shellcheck quoting issues and other fixes.

Signed-off-by: Greg Galloway greg.galloway@intel.com

The previous source reference for lib.sh triggers shellcheck errors.

Added quotes to all shellcheck suggested lines.


Signed-off-by: Greg Galloway <greg.galloway@intel.com>
SC2233 (style): Remove superfluous (..) around condition to
avoid subshell overhead.

SC2181 (style): Check exit code directly with e.g.
'if ! mycmd;', not indirectly with $?.

SC2219 (style): Instead of 'let expr', prefer (( expr )) .

Signed-off-by: Greg Galloway <greg.galloway@intel.com>
@greg-intel
greg-intel requested a review from a team as a code owner June 14, 2023 21:58
@marc-hb

marc-hb commented Jun 14, 2023

Copy link
Copy Markdown
Collaborator

No test has been run yet after 1h, this is not normal:

Screenshot 2023-06-14 at 15 57 34

@greg-intel

greg-intel commented Jun 15, 2023 •

Copy link
Copy Markdown
Contributor Author

SOFCI TEST

Saw a strange issue with the previous "sof_test_pr" Jenkins run:

FATAL: Channel "hudson.remoting.Channel@214ff76d:vm-pipeline-runner": 
Remote call on vm-pipeline-runner failed. The channel is closing down or has closed down
java.io.EOFException

@greg-intel

Copy link
Copy Markdown
Contributor Author

I should have looked more closely. It ends up check-xrun-injection.sh isn't called as part of the daily or PR tests, it's only part of run-all-tests.

@greg-intel

Copy link
Copy Markdown
Contributor Author

Test failures are unrelated, this test doesn't run during PR testing.

@marc-hb

marc-hb commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

Test failures are unrelated, this test doesn't run during PR testing.

So you gave it a decent round of testing locally, correct?

@greg-intel

Copy link
Copy Markdown
Contributor Author

So you gave it a decent round of testing locally, correct?

With pretty much every little change I made.

@marc-hb
marc-hb requested a review from a team June 23, 2023 21:49
@marc-hb

marc-hb commented Jun 23, 2023

Copy link
Copy Markdown
Collaborator

Thanks! Let's merge this first thing on Monday

sof-process-state.sh "$pid" >/dev/null || {
dloge "aplay/arecord process is in an abnormal status"
kill -9 $pid && wait $pid 2>/dev/null
kill -9 "$pid" && wait "$pid" 2>/dev/null

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This code seems incompatible with set -e but - off-topic.

@marc-hb
marc-hb merged commit c8d7a73 into thesofproject:main Jun 26, 2023
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