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

image_builder.py: Do not use os.system, as it encodes the return value #823

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

f14h
Copy link

@f14h f14h commented Dec 17, 2024

Pull Request Details

Description

os.system's return value is the process's return value, but encoded in the wait format on Linux which means, that the result is multiplied by 256. Thus a return value of 1 of the executed command results in a ret_val of 256, an return value of 2 means 512, and so on. If this value is forwarded without further handling, and naively used as "normal" exit status, then the value overflows and always results in a 0 exit code.

Instead of implementing a Windows/Linux dependant workaround, using subprocess instead of os.system is better anyway, as the usage of os.system is discouraged.

Related Issue

Which devices/areas does this affect?

Testing Done

I have built the X3x0 design with various failures on purpose.

Checklist

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project. See CODING.md.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes, and all previous tests pass.
  • I have checked all compat numbers if they need updating (FPGA compat,
    MPM compat, noc_shell, specific RFNoC block, ...)

f14h added 2 commits December 17, 2024 14:45
… it encodes the return value

os.system's return value is the process's return value, but encoded in the wait format
on Linux which means, that the result is multiplied by 256. Thus a return value of 1
of the executed command results in a ret_val of 256, an return value of 2 means 512,
and so on. If this value is forwarded without further handling, and naively used as
"normal" exit status, then the value overflows and always results in a 0 exit code.

Instead of implementing a Windows/Linux dependant workaround, using subprocess
instead of os.system is better anyway, as the usage of os.system is discouraged.

Signed-off-by: Frederik Pfautsch <frederik.pfautsch@missinglinkelectronics.com>
Signed-off-by: Frederik Pfautsch <frederik.pfautsch@missinglinkelectronics.com>
@f14h f14h force-pushed the mle/upstream/fix_return_val_handling_on_error branch from f84563f to 0e179bd Compare December 17, 2024 13:50
@joergho
Copy link
Contributor

joergho commented Dec 18, 2024

recheck

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