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.

Sign up for GitHub

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

[sonic-py-common] Add 'universal_newlines=True' arg to all Popen calls to properly support Python 3 #5919

Merged
merged 1 commit into from
Nov 17, 2020
Merged

[sonic-py-common] Add 'universal_newlines=True' arg to all Popen calls to properly support Python 3 #5919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/sonic-py-common/sonic_py_common/device_info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ def get_system_mac(namespace=None):
hw_mac_entry_cmds = [mac_address_cmd]

for get_mac_cmd in hw_mac_entry_cmds:
proc = subprocess.Popen(get_mac_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
proc = subprocess.Popen(get_mac_cmd, shell=True, universal_newlines=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the detailed investigation! So the best method should be:

  1. No extra argument for python 2
  2. Add text=True for python 3

I see you tried hard to maintain same statement for different version, actually it is not that important. Instead, I suggest you use a kwargs variables, add options based on python version, and pass it to Popen constructor.

Copy link
Contributor Author

@jleveque jleveque Nov 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's worth this much effort and refactoring, as we will be removing support for Python 2 soon. I think we should start with this solution as it's backward-compatible, then change universal_newlines=True to text=True once we support Python 3 only. This is a bug that affects the Python 3 version of the library and needs to get fixed ASAP; this PR fixes it in a simple, backward-compatible manner.

(mac, err) = proc.communicate()
if err:
continue
Expand Down Expand Up @@ -439,6 +439,7 @@ def get_system_routing_stack():
proc = subprocess.Popen(command,
stdout=subprocess.PIPE,
shell=True,
universal_newlines=True,
stderr=subprocess.STDOUT)
stdout = proc.communicate()[0]
proc.wait()
Expand Down Expand Up @@ -473,7 +474,7 @@ def is_warm_restart_enabled(container_name):
def is_fast_reboot_enabled():
fb_system_state = 0
cmd = 'sonic-db-cli STATE_DB get "FAST_REBOOT|system"'
proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE)
proc = subprocess.Popen(cmd, shell=True, universal_newlines=True, stdout=subprocess.PIPE)
(stdout, stderr) = proc.communicate()

if proc.returncode != 0:
Expand Down
1 change: 1 addition & 0 deletions src/sonic-py-common/sonic_py_common/multi_asic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ def get_current_namespace():
proc = subprocess.Popen(command,
stdout=subprocess.PIPE,
shell=True,
universal_newlines=True,
stderr=subprocess.STDOUT)
try:
stdout, stderr = proc.communicate()
Expand Down