Skip to content

Conversation

@shenxianpeng
Copy link
Collaborator

@shenxianpeng shenxianpeng commented Oct 11, 2022

Python 3.6 does not has capture_output https://docs.python.org/3.6/library/subprocess.html, use subprocess.PIPE instead can fix this problem.

I have donwload new build wheel and install on my python3.6 env, it works now.

It would be best to include test on python3.6 with GitHub action if we support py36 in later release.

Fix #34

@shenxianpeng shenxianpeng requested a review from 2bndy5 October 11, 2022 09:02
@shenxianpeng shenxianpeng added the bug Something isn't working label Oct 11, 2022
@2bndy5
Copy link
Contributor

2bndy5 commented Oct 11, 2022

python 3.6 is no longer maintained and is deemed deprecated.

try:
result = subprocess.run(
[exe_name, "--version"], capture_output=True, check=True
[exe_name, "--version"], stdout=subprocess.PIPE, check=True
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't totally equivalent, but I think it will work in this case.

According to the docs:

If capture_output is true, stdout and stderr will be captured. When used, the internal Popen object is automatically created with stdout=PIPE and stderr=PIPE. The stdout and stderr arguments may not be supplied at the same time as capture_output. If you wish to capture and combine both streams into one, use stdout=PIPE and stderr=STDOUT instead of capture_output.

Meaning, if you want to apply this change to cpp-linter pkg's use of subprocess.run(), then you'de have to use

result = subprocess.run(cmds, stdout=subprocess.PIPE, stderr=subprocess.PIPE)

because we actually use the stderr output in the cpp-linter pkg.

@shenxianpeng
Copy link
Collaborator Author

python 3.6 is no longer maintained and is deemed deprecated.

You're right. https://endoflife.date/python. OK, we do not need to test on python3.6.

Copy link
Contributor

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

I'm ok with using this patch. Not all users are keen to keeping their python version in the CI up-to-date.

@shenxianpeng shenxianpeng force-pushed the fix-install-failed-py36 branch from 011416f to 2065aaf Compare October 11, 2022 09:23
@codecov-commenter
Copy link

Codecov Report

Merging #36 (2065aaf) into main (aeee5f6) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #36   +/-   ##
=======================================
  Coverage   87.71%   87.71%           
=======================================
  Files           4        4           
  Lines         171      171           
=======================================
  Hits          150      150           
  Misses         21       21           
Impacted Files Coverage Δ
clang_tools/install.py 82.69% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@shenxianpeng
Copy link
Collaborator Author

Updated. sure, it would be nice if users of the old python version also could use it 😃

@shenxianpeng shenxianpeng merged commit ce41d4f into main Oct 11, 2022
@shenxianpeng shenxianpeng deleted the fix-install-failed-py36 branch October 11, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Install with clang-tools command failed on Python 3.6.8

4 participants