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

[cling-cpt] Replaced subprocess.popen calls with os.popen calls [skip-ci] #10863

Closed
wants to merge 1 commit into from

Conversation

saisoma123
Copy link
Contributor

This Pull request: Replaced the subprocess.popen calls with os.popen calls. Using the os module in general will make build time faster.

Changes or fixes: Replaced 11 subprocess.popen calls with os.popen calls, so with my previous pull request regarding the subprocess calls and this one, this entirely replaces the subprocess module use in the cpt.

Checklist:

  • tested changes locally
  • [NA] updated the docs (if necessary)

This PR fixes the issue mentioned in #406 (root-project/cling#406)

@saisoma123 saisoma123 requested a review from Axel-Naumann as a code owner June 29, 2022 22:24
@phsft-bot
Copy link

Can one of the admins verify this patch?

@vepadulano
Copy link
Member

Why are these changes needed? In general it is recommended to switch from os to subprocess, not the other way around

@saisoma123
Copy link
Contributor Author

That is correct, but I was told to replace the subprocess module use in the cpt, so I resorted to using the os module. I noticed however that the build time was quite a bit faster than before.

@vgvassilev
Copy link
Member

That is correct, but I was told to replace the subprocess module use in the cpt, so I resorted to using the os module. I noticed however that the build time was quite a bit faster than before.

The idea behind the replacement was where possible. For example when we use subprocess to call tar or wget which have the relevant python support.

@saisoma123 saisoma123 closed this Jul 18, 2022
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