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

Execution: Add stderr block #2397

Merged
merged 1 commit into from
Nov 28, 2024
Merged

Execution: Add stderr block #2397

merged 1 commit into from
Nov 28, 2024

Conversation

Alx-Lai
Copy link
Collaborator

@Alx-Lai Alx-Lai commented Oct 6, 2024

Add a stderr block.
Pros:

  • we can make use of dbg macro in our code Cons:
  • there's a limitation that the compile message also shows

Applies patches from rust-lang/mdBook#1315 since the original change was not merged by rust-lang.

Issue: #531

@mgeisler
Copy link
Collaborator

mgeisler commented Oct 6, 2024

Hi @Alx-Lai, oh wow, this patch looks great! Thanks a lot for looking into this, I would very much like to have this feature implemented.

I gave the PR a quick test run locally and this is how it looks:

Screencast.from.2024-10-06.23-12-20.mp4

This is nearly perfect. A few comments:

  • Could you match and remove the three lines that appear when things compile correctly? The lines which say:

       Compiling playground v0.0.1 (/playground)
        Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.60s
         Running `target/debug/playground`
    

    By removing them in the happy case, we end up with the same output as today, which helps limit the amount of vertical space used. This is important for a live class where it's very disturbing to scroll up and down :)

  • Could you avoid showing the stderr block immediately when compilation starts? Ideally, there would be a single output with the combination of stdout and stderr, but I understand that the Playground delivers the data in two separate fields.

  • Similarly, the block with the stderr output should be hidden/cleared when the code is recompiled.

I looked briefly at the full Playground and discovered that they use a WebSocket now to stream the output line by line! They include markers to show if this is a stdout or stderr:

image

I would be happy to merge this PR as a first step — I don't know how difficult it will be to adapt the mdbook code to use a WebSocket as well.

@Alx-Lai
Copy link
Collaborator Author

Alx-Lai commented Oct 6, 2024

Ok, I'll spend time looking into it. Thanks for the feedback!

Add a stderr block.
Pros:
  - we can make use of dbg macro in our code
Cons:
  - there's a limitation that the compile message also shows

To be improved:
  - compile message regex may change overtime
  - can use websocket to replace the current approach

Applies patches from rust-lang/mdBook#1315 since the original change
was not merged by rust-lang.

Signed-off-by: Alx-Lai <[email protected]>
@Alx-Lai
Copy link
Collaborator Author

Alx-Lai commented Oct 7, 2024

Updated the commit.

  • Remove compile message with a not-that-elegant way, the compile error message would not be replaced.
  • hide and clear stderr block before compiling the code.

@mgeisler
Copy link
Collaborator

Hi @Alx-Lai, thanks for the update! I haven't forgotten about it, I'm just slow to review the change.

I'll try asking internally if someone else can help review this.

@gotoextreme
Copy link

I just tested the updated version, and it works great!

Screencast.from.2024-11-20.16-12-30.webm

@mgeisler
Copy link
Collaborator

Thanks a lot, @gotoextreme! That's super helpful :) I think we can merge this now and then we'll soon have proper testing support (#2462) to ensure that things say stable as we refactor and extend this further.

Copy link
Collaborator

@mgeisler mgeisler left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this is an exciting step forward! It will allow us to use dbg! throughout the course, which I've wanted for a long time.

@mgeisler mgeisler merged commit beacea7 into google:main Nov 28, 2024
35 checks passed
@mgeisler
Copy link
Collaborator

Hey @Alx-Lai, I saw this working when testing locally, but now that it's deployed to https://google.github.io/comprehensive-rust/, I see

POST https://play.rust-lang.org/execute 500 (Internal Server Error)

every time I try running some code. Can you check if you see the same? If so, then we'll have to revert this for now and figure out what is happening.

mgeisler added a commit that referenced this pull request Nov 28, 2024
mgeisler added a commit that referenced this pull request Nov 28, 2024
Reverts #2397

I'm seeing

```
POST https://play.rust-lang.org/execute 500 (Internal Server Error)
```

on every request when trying to run code in the Playground. I'm not sure
why, but we need to revert this ASAP if others see the same.
@mgeisler
Copy link
Collaborator

Alright, @Alx-Lai confirmed that he sees the same error, so I reverted this in #2479. I would still love to see this feature, so I hope we can figure out why it didn't work on the published version.

@mgeisler
Copy link
Collaborator

mgeisler commented Dec 6, 2024

I think we can add this again, please see #2503.

mgeisler added a commit that referenced this pull request Dec 6, 2024
Reverts #2479, which is a revert of #2397.

I think the problem was not related to @Alx-Lai's change, the
[Playground was
slow](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/playground.20incident.202024-12-03)
for everyone.
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.

3 participants