-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Make sure that 'tox -erust' fails on bad RC. #13753
base: main
Are you sure you want to change the base?
Conversation
Previously, a non-zero exit code from the subprocess would not fail the tox run like we'd expect.
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13035694187Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just left a small comment inline. It doesn't really matter in this context though.
tox.ini
Outdated
@@ -41,7 +41,7 @@ commands = | |||
python -c '\ | |||
import os, subprocess, sysconfig;\ | |||
os.environ["LD_LIBRARY_PATH"] = os.pathsep.join(filter(None, [sysconfig.get_config_var("LIBDIR"), os.getenv("LD_LIBRARY_PATH")]));\ | |||
subprocess.run(["cargo", "test", "--no-default-features"])' | |||
subprocess.run(["cargo", "test", "--no-default-features"]).check_returncode()' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that it particularly matters in this context. But what I normally do is:
subprocess.run(["cargo", "test", "--no-default-features"]).check_returncode()' | |
subprocess.run(["cargo", "test", "--no-default-features"], check=True)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pylint has a lint against failing to check the return code of subprocess.run
- it might be a motivation to move this inline script into a proper file in the tools
directory so it's subject to lint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good idea, and should let us use the same script from tox and CI.
Done in 663eaa8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM for the most part. I like having the script in a dedicated file now. Just one question/suggestion inline.
os.environ["LD_LIBRARY_PATH"] = os.pathsep.join( | ||
filter(None, [sysconfig.get_config_var("LIBDIR"), os.getenv("LD_LIBRARY_PATH")]) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not just do?:
os.environ["LD_LIBRARY_PATH"] = os.pathsep.join( | |
filter(None, [sysconfig.get_config_var("LIBDIR"), os.getenv("LD_LIBRARY_PATH")]) | |
) | |
os.environ["LD_LIBRARY_PATH"] = os.pathsep.join([ | |
sysconfig.get_config_var("LIBDIR"), | |
os.getenv("LD_LIBRARY_PATH") | |
]) |
I've always found the filter(None, ...)
pattern a bit odd when you can just use a generator expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I did that was to filter out None
in the case that "LD_LIBRARY_PATH"
isn't actually set to anything. I can do something different if you prefer!
Summary
Previously, a non-zero exit code from the subprocess would not fail the tox run like we'd expect.