Skip to content

Conversation

@marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Mar 31, 2022

The entire purpose of replacing the older shell script with Python was
to achieve cross platform compatibility. shell=True would lose that.

See also
https://docs.python.org/3/library/subprocess.html#security-considerations

Signed-off-by: Marc Herbert marc.herbert@intel.com

The entire purpose of replacing the older shell script with Python was
to achieve cross platform compatibility. shell=True would lose that.

See also
https://docs.python.org/3/library/subprocess.html#security-considerations

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb marked this pull request as ready for review March 31, 2022 21:52
@marc-hb marc-hb requested a review from aborisovich March 31, 2022 21:52
@lgirdwood
Copy link
Member

@aborisovich any comments and can you confirm this is good on Windows ? Thanks.

@aborisovich
Copy link
Contributor

@aborisovich any comments and can you confirm this is good on Windows ? Thanks.

Just build TGL as a test, works fine. I am not sure we should add "manual code restrictions" like this one.
I think it would be better to just prevent shell=True on code reviews but ok, it does not matter that much.

@lgirdwood lgirdwood merged commit 93f20d5 into thesofproject:main Apr 4, 2022
@marc-hb marc-hb deleted the zepbuild-noshell branch April 4, 2022 16:19
@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 4, 2022

I think it would be better to just prevent shell=True on code reviews but ok

Thanks @aborisovich

Code reviews are critical but also... human: unreliable, not immediate, scarce,... :-)

If there is some real shell=True need later (none yet AFAIK), then this should provide a simple solution:

if os == windows:
    log(windows_cmd)
    subprocess.run(windows_cmd, shell=True,...)

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.

4 participants