Skip to content

Conversation

@aiChaoSONG
Copy link

If only redirect nohup stdout to /dev/null, we will see
'nohup: redirecting stderr to stdout' message from nohup
when manually run './check-kmod-load-unload.sh -l1'.

Which will make the test log looks ugly.

Signed-off-by: Amery Song chao.song@intel.com

If only redirect nohup stdout to /dev/null, we will see
'nohup: redirecting stderr to stdout' message from nohup
when manually run './check-kmod-load-unload.sh -l1'.

Signed-off-by: Amery Song <chao.song@intel.com>
@libinyang
Copy link
Contributor

LGTM

Copy link
Contributor

@Bin-QA Bin-QA left a comment

Choose a reason for hiding this comment

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

I don't this this patch will meet your requirement, because
when you add '&' for nohup command it will create 'nohup.out' file for stdout & stderr

@aiChaoSONG
Copy link
Author

I don't this this patch will meet your requirement, because
when you add '&' for nohup command it will create 'nohup.out' file for stdout & stderr

@Bin-QA Any suggestions? because “nohup: redirecting stderr to stdout” in the test log, which makes the log ugly, and I test this pr on helios, the output of nohup disappeared.

@Bin-QA
Copy link
Contributor

Bin-QA commented Mar 31, 2020

@Bin-QA Any suggestions? because “nohup: redirecting stderr to stdout” in the test log, which makes the log ugly, and I test this pr on helios, the output of nohup disappeared.

because you will create 'nohup.out' at your current folder

@xiulipan
Copy link
Contributor

xiulipan commented Apr 2, 2020

@aiChaoSONG will this bring us trouble with debug? As error message is gone

@aiChaoSONG
Copy link
Author

@Bin-QA When we run sof-test, the repo is cloned to local, so all command is running locally, then why we need a nohup here?

@aiChaoSONG aiChaoSONG changed the title case-lib: redirect nohup stdout and stderr to /dev/null [IN DISCUSSION, NOT MERGE]case-lib: redirect nohup stdout and stderr to /dev/null Apr 2, 2020
@Bin-QA
Copy link
Contributor

Bin-QA commented Apr 2, 2020

@aiChaoSONG because here need recreate pulseaudio command,
Notice: here recreate pulseaudio is all command , so it is not limited the system server
So when you terminal quit, the command attach for your terminal will be killed
the nohup command use to avoid terminal/parent process quit problem

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 10, 2020

Which will make the test log looks ugly.

Test logs are not meant to be beautiful, they have to be informative and not too noisy. That does not seem like a lot of noise, is it?

Logs that are really noisy should be redirected to a file, tests should almost never redirect anything to >/dev/null. In case of a failure any information could be useful.

EDIT: I detailed why I think /dev/null is a bad habit in this unrelated PR: thesofproject/sof#2711 (comment)

I have a larger question: why is this script not using systemctl? https://docs.ubuntu.com/core/en/stacks/audio/pulseaudio/docs/using-pulseaudio
Is this script not running on Ubuntu?

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 14, 2020

I have a larger question: why is this script not using systemctl? https://docs.ubuntu.com/core/en/stacks/audio/pulseaudio/docs/using-pulseaudio

OK, this doc is obsolete, sorry. I found this more accurate one, tested it and verified it is up to date wrt. Ubuntu 18.04
https://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/User/Running/

Why is the nohup needed at all? I ran pulseaudio --start over ssh with no autospawn and it worked without nohup.

Also wondering why use kill -KILL $(ps) instead of a simpler pulseaudio --kill?

Why use ps instead of pulseaudio --check?

Why try to extract command line parameters from ps as opposed to rely on regular config files?

Also why manually restart pulseaudio after restoring the autospawn which already starts it when needed?

@aiChaoSONG
Copy link
Author

@Bin-QA Could you please help to answer Marc's questions?

@Bin-QA
Copy link
Contributor

Bin-QA commented Apr 14, 2020

@marc-hb because we are not sure target machine how to configure the pulseaudio
the end user (current just us) maybe not start pulseaudio or something
so it detect in ps if target machine configure more pulseaudio version,
how to trace them and restore them also is the problem (normally it will refer configure and test pulseaudio)
we want the script in here is more common to use

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 14, 2020

I'm not sure I follow, are you afraid some operating system(s) hide(s) some pulseaudio --parameters outside the normal pulseaudio configuration files?

How many different operating system versions is sof-test regularly used on? I didn't see any mentioned in https://github.com/thesofproject/sof-test/blob/master/README.md

@marc-hb marc-hb marked this pull request as draft July 9, 2020 01:10
@marc-hb marc-hb requested review from lyakh and removed request for wenqingfu July 30, 2020 17:57
@xiulipan
Copy link
Contributor

xiulipan commented Sep 2, 2020

@aiChaoSONG @macchian any update? Do we still need this

@aiChaoSONG
Copy link
Author

No, let's close this, the issue I want to address by this PR is not reproduced. maybe already fixed by other PR

@aiChaoSONG aiChaoSONG closed this Sep 2, 2020
@aiChaoSONG aiChaoSONG deleted the nohup branch October 24, 2022 08:27
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.

5 participants