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

Deprecate and replace calls to back_tick. #126

Merged
merged 2 commits into from
Sep 22, 2021

Conversation

HexDecimal
Copy link
Collaborator

Related to #125

The non-specific timeout check in back_tick has been removed. It was that or provide a specific timeout time.

All internal functions and tests now use subprocess.run. This might change some of the exceptions raised by those functions except for lipo_fuse which was specifically tested to raise RuntimeError.

Relevant tests have been cleaned up.

Relevant functions have had type annotations added/updated.

Ready to merge after review.

The non-specific timeout check has been removed.  It was that or
provide a specific timeout time.

All internal functions and tests now use subprocess.run.

Relevant tests have been cleaned up.

Relevant functions have had type annotations added/updated.
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2021

Codecov Report

Merging #126 (735be64) into master (2b2e6bf) will increase coverage by 0.18%.
The diff coverage is 97.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   94.89%   95.07%   +0.18%     
==========================================
  Files          13       13              
  Lines         999      995       -4     
==========================================
- Hits          948      946       -2     
+ Misses         51       49       -2     
Impacted Files Coverage Δ
delocate/tools.py 94.81% <97.56%> (+0.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b2e6bf...735be64. Read the comment docs.

Copy link
Owner

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

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

Maybe consider the subprocess convenience functions to simplify calls to subprocess.run? Otherwise, looks very good, thanks.

@@ -278,15 +297,20 @@ def set_install_name(filename, oldname, newname, ad_hoc_sign=True):
if oldname not in names:
raise InstallNameError('{0} not in install names for {1}'.format(
oldname, filename))
back_tick(['install_name_tool', '-change', oldname, newname, filename])
subprocess.run(
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't this just subprocess.check_call?

@@ -304,7 +328,12 @@ def set_install_id(filename, install_id, ad_hoc_sign=True):
"""
if get_install_id(filename) is None:
raise InstallNameError('{0} has no install id'.format(filename))
back_tick(['install_name_tool', '-id', install_id, filename])
subprocess.run(
Copy link
Owner

Choose a reason for hiding this comment

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

subprocess.check_call again?

@@ -385,12 +414,17 @@ def add_rpath(filename, newpath, ad_hoc_sign=True):
ad_hoc_sign : {True, False}, optional
If True, sign file with ad-hoc signature
"""
back_tick(['install_name_tool', '-add_rpath', newpath, filename])
subprocess.run(
Copy link
Owner

Choose a reason for hiding this comment

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

subprocess.check_call?

@@ -402,7 +436,12 @@ def zip2dir(zip_fname, out_dir):
"""
# Use unzip command rather than zipfile module to preserve permissions
# http://bugs.python.org/issue15795
back_tick(['unzip', '-o', '-d', out_dir, zip_fname])
subprocess.run(
Copy link
Owner

Choose a reason for hiding this comment

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

check_call again?

@@ -506,8 +545,15 @@ def get_archs(libname):
if not exists(libname):
raise RuntimeError(libname + " is not a file")
try:
stdout = back_tick(['lipo', '-info', libname])
except RuntimeError:
lipo = subprocess.run(
Copy link
Owner

Choose a reason for hiding this comment

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

subprocess.check_output?

in_fname1, in_fname2,
'-output', out_fname])
try:
lipo = subprocess.run(
Copy link
Owner

Choose a reason for hiding this comment

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

subprocess.check_output?

@@ -565,11 +624,15 @@ def replace_signature(filename, identity):
identity : str
The signing identity to use.
"""
back_tick(['codesign', '--force', '--sign', identity, filename],
raise_err=True)
subprocess.run(
Copy link
Owner

Choose a reason for hiding this comment

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

subprocess.check_call?

out, err = back_tick(['codesign', '--verify', filename],
ret_err=True, as_str=True, raise_err=False)
if not err:
codesign = subprocess.run(
Copy link
Owner

Choose a reason for hiding this comment

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

subprocess.check_output?

@HexDecimal
Copy link
Collaborator Author

subprocess.check_output
subprocess.check_call

See https://docs.python.org/3/library/subprocess.html, it seems since Python 3.5 the official recommendation is to use subprocess.run for all cases.

The recommended approach to invoking subprocesses is to use the run() function for all use cases it can handle. For more advanced use cases, the underlying Popen interface can be used directly.

@matthew-brett
Copy link
Owner

OK - all good then.

@matthew-brett matthew-brett merged commit 2ecefa3 into matthew-brett:master Sep 22, 2021
@HexDecimal HexDecimal deleted the subprocess-run branch September 22, 2021 23:04
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.

None yet

3 participants