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
Merged

Conversation

@xfix
Copy link
Contributor

@xfix xfix commented Oct 24, 2020

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 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?

@xfix
Copy link
Contributor Author

@xfix xfix commented Oct 24, 2020

@ofborg test bittorrent installer

Ma27
Ma27 approved these changes Oct 24, 2020
@Ma27 Ma27 requested a review from flokli Oct 24, 2020
flokli
flokli approved these changes Oct 24, 2020
@tfc
Copy link
Contributor

@tfc tfc commented Oct 25, 2020

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

@xfix
Copy link
Contributor Author

@xfix xfix commented Oct 25, 2020

It failed because there is no test called installer.

@ofborg test bittorrent installer.simple acme

@xfix
Copy link
Contributor Author

@xfix xfix commented Oct 25, 2020

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
@xfix xfix force-pushed the test-driver-bufsize branch from 526b166 to 254d30d Oct 25, 2020
@xfix
Copy link
Contributor Author

@xfix xfix commented Oct 25, 2020

@ofborg test bittorrent installer.simple acme

@xfix
Copy link
Contributor Author

@xfix xfix commented Oct 25, 2020

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.

@xfix
Copy link
Contributor Author

@xfix xfix 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
19 checks passed
19 checks passed
@github-actions[bot]
tests
Details
@github-actions[bot]
action
Details
@ofborg[bot]
tests.acme, tests.bittorrent, tests.installer.simple on aarch64-linux Failure
Details
@ofborg[bot]
tests.acme, tests.bittorrent, tests.installer.simple on x86_64-linux Failure
Details
@ofborg[bot]
Evaluation Performance Report Evaluator Performance Report
Details
@github-actions[bot]
Wait for ofborg
Details
@ofborg[bot]
grahamcofborg-eval ^.^!
Details
@ofborg[bot]
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
@ofborg[bot]
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
@ofborg[bot]
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="254d30d"; rev="254d30d4c95f35d6d0f54c2b1c2a76339ae76de9"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
@ofborg[bot]
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="254d30d"; rev="254d30d4c95f35d6d0f54c2b1c2a76339ae76de9"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="254d30d"; rev="254d30d4c95f35d6d0f54c2b1c2a76339ae76de9"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="254d30d"; rev="254d30d4c95f35d6d0f54c2b1c2a76339ae76de9"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="254d30d"; rev="254d30d4c95f35d6d0f54c2b1c2a76339ae76de9"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="254d30d"; rev="254d30d4c95f35d6d0f54c2b1c2a76339ae76de9"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="254d30d"; rev="254d30d4c95f35d6d0f54c2b1c2a76339ae76de9"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
@ofborg[bot]
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants