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

Fix RFC-1014 test #75532

Merged
merged 1 commit into from
Aug 21, 2020
Merged

Fix RFC-1014 test #75532

merged 1 commit into from
Aug 21, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Aug 14, 2020

Use two printlns when testing that writing to a closed stdout does not
panic. Otherwise the test is ineffective, since the current implementation
silently ignores the error during first println regardless.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2020
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 14, 2020

Created in part to test @workingjubilee theory that there is an existing, but undetected issue on wasm32-unknown-emscripten target (although the fixed test does pass for me locally), cc @Lucretiel.

@Lucretiel
Copy link
Contributor

Lucretiel commented Aug 14, 2020

Otherwise the test is ineffective, since the current implementation silently ignores the error during first println regardless.

Do you mean that this first ignore is independent of the handle_ebadf used in Maybe::write? This would be news to me, do you know where this first error ignore is happening?

@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 14, 2020

It is consequence of the fact that flushing is driven internally rather than
externally.

For example, "hello world\n" is written to the internal buffer successfully.
It ends with a newline, and a flush attempt is made. It fails, but since input
is already buffered the writer returns Ok(12) indicating that all data had been
written, while marking that a writer needs to be flushed. On the next write
attempt, flush will happen first and an error is returned.

Maybe saying that return of the error is delayed would be more accurate.

@Lucretiel
Copy link
Contributor

Oh! OH! OHHH! That explains it! That explains why my PR breaks this test, because one of the core design goals is cleaning up the error suppressing behavior of the current LineWriter!

@workingjubilee
Copy link
Member

Excellent synthesis! I hope this proves revealing.

@Lucretiel
Copy link
Contributor

Might be worth changing /src/test/ui/rfcs/rfc-1014-2.rs to match.

@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 14, 2020

Updated rfc-1014-2.rs test case as well.

Use two printlns when testing that writing to a closed stdout does not
panic. Otherwise the test is ineffective, since the current implementation
silently ignores the error during first println regardless.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 16, 2020
Bump minor version of emsdk to 1.38.47

Release Notes:

```
v1.38.47: 10/02/2019
--------------------
 - Add support for FETCH API in WASM backend. This doesn't support FETCH in the
   main thread (`USE_FETCH_WORKER=0` is enforced). rust-lang#9490
 - Redefine errno values to be consistent with wasi. This will let us avoid
   needing to convert the values back and forth as we use more wasi APIs.
   This is an ABI change, which should not be noticeable from user code
   unless you use errno defines (like EAGAIN) *and* keep around binaries
   compiled with an older version that you link against. In that case, you
   should rebuild them. See rust-lang#9545.
 - Removed build option `-s ONLY_MY_CODE` as we now have much better solutions
   for that, like building to a wasm object file or using `STANDALONE_WASM`
   etc. (see
   https://github.com/emscripten-core/emscripten/wiki/WebAssembly-Standalone).
 - Emscripten now supports the config file (.emscripten) being placed in the
   emscripten directory rather that the current user's home directory.
   See rust-lang#9543
```

Motivated by changes to errno values which are currently out of sync with those
in libc crate which uses wasi values already. Helps with rust-lang#72808 and rust-lang#75532.
tmandry added a commit to tmandry/rust that referenced this pull request Aug 16, 2020
Bump minor version of emsdk to 1.38.47

Release Notes:

```
v1.38.47: 10/02/2019
--------------------
 - Add support for FETCH API in WASM backend. This doesn't support FETCH in the
   main thread (`USE_FETCH_WORKER=0` is enforced). rust-lang#9490
 - Redefine errno values to be consistent with wasi. This will let us avoid
   needing to convert the values back and forth as we use more wasi APIs.
   This is an ABI change, which should not be noticeable from user code
   unless you use errno defines (like EAGAIN) *and* keep around binaries
   compiled with an older version that you link against. In that case, you
   should rebuild them. See rust-lang#9545.
 - Removed build option `-s ONLY_MY_CODE` as we now have much better solutions
   for that, like building to a wasm object file or using `STANDALONE_WASM`
   etc. (see
   https://github.com/emscripten-core/emscripten/wiki/WebAssembly-Standalone).
 - Emscripten now supports the config file (.emscripten) being placed in the
   emscripten directory rather that the current user's home directory.
   See rust-lang#9543
```

Motivated by changes to errno values which are currently out of sync with those
in libc crate which uses wasi values already. Helps with rust-lang#72808 and rust-lang#75532.
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 17, 2020

The error number mismatch on the CI was fixed by #75569, and this should be working correctly now.

@Dylan-DPC-zz
Copy link

@bors r=nikomatsakis rollup

@bors
Copy link
Contributor

bors commented Aug 20, 2020

📌 Commit fa8d396 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#75324 (clarify documentation of remove_dir errors)
 - rust-lang#75532 (Fix RFC-1014 test)
 - rust-lang#75664 (Update mailmap for mati865)
 - rust-lang#75727 (Switch to intra-doc links in `core::result`)
 - rust-lang#75750 (Move to intra doc links for std::thread documentation)

Failed merges:

r? @ghost
@bors bors merged commit 87c22f4 into rust-lang:master Aug 21, 2020
@tmiasko tmiasko deleted the rfc-1014 branch August 21, 2020 12:03
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants