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

test-driver.py: remove bufsize=1 from Popen calls #101538

Merged
merged 1 commit into from Oct 25, 2020

Conversation

KamilaBorowska
Copy link
Member

According to Python documentation, bufsize=1 is only meaningful in text mode. As we don't pass in an argument called universal_newlines, encoding, errors or text the file objects aren't opened in text mode, which means the argument is ignored with a warning in Python 3.8.

line buffering (buffering=1) isn't supported in binary mode, the default buffer size will be used

This commit removes this warning that appared when using interactive test driver built with -A driver. This is done by removing bufsize=1 from Popen calls.

The default parameter when unspecified for bufsize is -1 which according to the documentation will be interpreted as io.DEFAULT_BUFFER_SIZE. As mentioned by a warning, Python already uses default buffer size when providing buffering=1 parameter for file objects not opened in text mode.

Motivation for this change

Fixing a warning.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@tfc
Copy link
Contributor

tfc commented Oct 24, 2020

Looks good, thank you.

I don't feel good merging this without having the bot posting success on a few test runs here. Can you start some test runs on this?

@KamilaBorowska
Copy link
Member Author

@ofborg test bittorrent installer

@Ma27 Ma27 requested a review from flokli October 24, 2020 15:56
@tfc
Copy link
Contributor

tfc commented Oct 25, 2020

@xfix can you please have a look why the tests.installer test failed on aarch64 linux?

@KamilaBorowska
Copy link
Member Author

It failed because there is no test called installer.

@ofborg test bittorrent installer.simple acme

@KamilaBorowska
Copy link
Member Author

Oh right, I need to rebase newest master to fix tests because of bc2188b.

According to Python documentation [0], `bufsize=1` is only meaningful in
text mode. As we don't pass in an argument called `universal_newlines`,
`encoding`, `errors` or `text` the file objects aren't opened in text
mode, which means the argument is ignored with a warning in Python 3.8.

    line buffering (buffering=1) isn't supported in binary mode,
    the default buffer size will be used

This commit removes this warning that appared when using
interactive test driver built with `-A driver`. This is done by
removing `bufsize=1` from Popen calls.

The default parameter when unspecified for `bufsize` is `-1` which
according to the documentation will be interpreted as
`io.DEFAULT_BUFFER_SIZE`. As mentioned by a warning, Python already
uses default buffer size when providing `buffering=1` parameter for
file objects not opened in text mode.

[0]: https://docs.python.org/3/library/subprocess.html#subprocess.Popen
@KamilaBorowska
Copy link
Member Author

@ofborg test bittorrent installer.simple acme

@KamilaBorowska
Copy link
Member Author

machine # error: The option `virtualisation.qemu' does not exist. Definition values:
machine # - In `/nix/var/nix/profiles/per-user/root/channels/nixos/nixos/modules/testing/test-instrumentation.nix':
machine #     {
machine #       consoles = [ ];
machine #       package = {
acme # [    7.972112] systemd[1]: Starting Rebuild Journal Catalog...
machine #         _type = "override";
machine #         content = <derivation /nix/store/q72h2cdcb9zjgiay5gdgzwddjkbjr7xq-qemu-host-cpu-only-for-vm-tests-5.1.0.drv>;
machine #     ...
machine # (use '--show-trace' to show detailed location information)
acme # [    7.976564] systemd[1]: Starting Update UTMP about System Boot/Shutdown...
webserver # [    7.915537] py3bq0l0ph88jak56zjk02gvbyqnjq8y-audit-disable[603]: No rules
machine: output:
Test "Perform the installation" failed with error: "command `nixos-install < /dev/null >&2` failed (exit code 1)"
error:
Traceback (most recent call last):
  File "/nix/store/k9kdg525z6dcwqixvb5zxzw4nnayc0rv-nixos-test-driver/bin/.nixos-test-driver-wrapped", line 899, in run_tests
    exec(tests, globals())
  File "<string>", line 1, in <module>
  File "<string>", line 50, in <module>
  File "/nix/store/k9kdg525z6dcwqixvb5zxzw4nnayc0rv-nixos-test-driver/bin/.nixos-test-driver-wrapped", line 422, in succeed
    raise Exception(
Exception: command `nixos-install < /dev/null >&2` failed (exit code 1)

Okay, I think this may have been caused by bc2188b actually.

@KamilaBorowska
Copy link
Member Author

KamilaBorowska commented Oct 25, 2020

Okay installer.simple may be fixed by #101645, this isn't related to this pull request.

@tfc tfc merged commit f4254fb into NixOS:master Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants