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

Improve action log display with control chars #23820

Merged
merged 7 commits into from
Apr 1, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 30, 2023

Close #23680

Some CLI programs use "\r" and control chars to print new content in current line.

So, the strings in one line are actually from \rReading...1%\rReading...5%\rReading...100%

This PR tries to make the output better.

@wxiaoguang wxiaoguang force-pushed the improve-action-log-line branch 4 times, most recently from 3ebe3e6 to edde647 Compare March 30, 2023 14:26
@wxiaoguang wxiaoguang force-pushed the improve-action-log-line branch from edde647 to 3799a92 Compare March 30, 2023 14:27
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 30, 2023
@delvh delvh added this to the 1.20.0 milestone Mar 30, 2023
@delvh delvh added the type/enhancement An improvement of existing functionality label Mar 30, 2023
@silverwind
Copy link
Member

silverwind commented Mar 30, 2023

Would this have adverse effects of Windows-style newlines \r\n? Would suggest adding a test for that as well.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 30, 2023

Would this have adverse effects of Windows-style newlines \r\n? Would suggest adding a test for that as well.

Nope, you can check the tests, one of them covers the ending "\r".


'\rabc\rx\r' there is an ending "\r", it won't affect the final result. And the lines have been splitted by "\n", so there wont' be "\n" in the string if I understand correctly, and that's why this function is called processConsoleLine, it only processes one line.

@wxiaoguang
Copy link
Contributor Author

To make code more strict, I make the function remove the trailing "\r\n" or "\n" in 2c72257 , then it doesn't matter whether the line ends with new-line or not.

@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2023

Codecov Report

Merging #23820 (2c72257) into main (f521e88) will decrease coverage by 0.16%.
The diff coverage is 28.98%.

❗ Current head 2c72257 differs from pull request most recent head c4249bd. Consider uploading reports for the commit c4249bd to get more accurate results

@@            Coverage Diff             @@
##             main   #23820      +/-   ##
==========================================
- Coverage   47.14%   46.98%   -0.16%     
==========================================
  Files        1149     1158       +9     
  Lines      151446   153175    +1729     
==========================================
+ Hits        71397    71971     +574     
- Misses      71611    72706    +1095     
- Partials     8438     8498      +60     
Impacted Files Coverage Δ
cmd/dump.go 0.66% <0.00%> (-0.01%) ⬇️
cmd/mailer.go 0.00% <0.00%> (ø)
cmd/manager.go 0.00% <0.00%> (ø)
cmd/manager_logging.go 0.00% <0.00%> (ø)
cmd/migrate_storage.go 5.76% <0.00%> (-0.12%) ⬇️
cmd/restore_repo.go 0.00% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.63% <0.00%> (-0.10%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
... and 65 more

... and 67 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 31, 2023
@yp05327
Copy link
Contributor

yp05327 commented Mar 31, 2023

image: docker:23
image
tested in 2c72257

I prefer add a special process to handle \r or \n which can exchange one line log into several new lines, like:

1 Reading...10%
2 Reading...30%
3 Reading...50%
4 Reading...70%
5 Reading...Done

@wxiaoguang wxiaoguang marked this pull request as draft March 31, 2023 02:27
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 31, 2023

I see, I haven't spent enough time on this problem and it looks like not that simple. The comment TODO: control chars is also necessary for most cases.

So let's mark this PR as WIP.

To make a further improvement, I guess there could be 3 approaches:

  1. Make backend split "\r\n" and "\n" and "\r", then the frontend can render:

    1 Reading... 1%
    2 Reading... 5%
    3 Reading... Done
    4 Next line
    
  2. Make frontend split:

    1 Reading... 1%
      Reading... 5%
      Reading... Done
    2 Next line
    
  3. Wait for more time to support all control chars and test for various cases.

Feel free to help or propose new PRs (while I am trying to implement approach 2)

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 31, 2023

@yp05327 what do you think about approach 2: "Make frontend split" ? I think it's clear and simple enough, and the output is still reasonable: because these strings are indeed rendered in one line if you run the command in terminal.

See 9d35890 and its tests.


If we want to implement approach 1, I am not sure how much work it needs for backend changes (if approach 2 is good enough, then we do not need to change backend's behavior).

If we want to implement approach 3, we need make AnsiToHTML package support Erase Control Chars , or choose other rendering packages.

@wxiaoguang wxiaoguang force-pushed the improve-action-log-line branch 2 times, most recently from 01dcd0b to 400cb2e Compare March 31, 2023 03:18
@wxiaoguang wxiaoguang force-pushed the improve-action-log-line branch from 400cb2e to 9d35890 Compare March 31, 2023 03:20
@yp05327
Copy link
Contributor

yp05327 commented Mar 31, 2023

I agree approach 2 and 3. And using an existing package to handle it looks better, as we don’t need to maintain it by ourselves to support all possible chars and cases.
But I have no idea about it.

@wxiaoguang wxiaoguang marked this pull request as ready for review March 31, 2023 03:57
@wxiaoguang
Copy link
Contributor Author

Thank you. I think 9d35890 could implement approach 2. If it looks good to you, let's move on ~~

I have done a quick test, the "\n" looks good to me 😁

@yp05327
Copy link
Contributor

yp05327 commented Mar 31, 2023

image
image
tested in 9d35890

I found the original log files, and it seems that the reason of this problem is ESC.
image
image

@yp05327
Copy link
Contributor

yp05327 commented Mar 31, 2023

There is a document maybe related?
https://man7.org/linux/man-pages/man4/console_codes.4.html

@yp05327
Copy link
Contributor

yp05327 commented Mar 31, 2023

@wxiaoguang
Copy link
Contributor Author

That's also the comment in code control chars like "\033[" , it should be handled by the AnsiToHTML packages (the color is also provided by these control chars). I will write some new tests to see why AnsiToHTML doesn't work for them in this case.

@wxiaoguang
Copy link
Contributor Author

Hmm, I think it's a bug in AnsiToHTML package.

Without our code, AnsiToHTML itself doesn't work for \033[1A nor \033[1B, it only handles the \033[2K

  const ath = new AnsiToHTML({escapeXML: true});
  expect(ath.toHtml('\x1b[1A\x1b[2K\rtest\r\x1b[1B\x1b[1A\x1b[2K')).toEqual('A\rtest\rBA');
  expect(ath.toHtml('\x1b[1A\x1b[2Ktest\x1b[1B\x1b[1A\x1b[2K')).toEqual('AtestBA');

@wxiaoguang wxiaoguang force-pushed the improve-action-log-line branch 2 times, most recently from fd6aa9a to aaa2a5e Compare March 31, 2023 07:01
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 31, 2023

Although it's related to upstream bug/limitation (https://github.com/rburns/ansi-to-html) , upstream has many open issues/PRs, so I guess we could bypass the bug on our side first. And I guess some behaviors we like couldn't be accepted by upstream easily (eg: treat the [0K or [0J as new line)

So, let's try to use 2604c22 to patch.

@wxiaoguang wxiaoguang force-pushed the improve-action-log-line branch from aaa2a5e to 2604c22 Compare March 31, 2023 07:05
@wxiaoguang wxiaoguang changed the title Improve action log display with "\r" Improve action log display with control chars Mar 31, 2023
@silverwind
Copy link
Member

Maybe there better JS modules around for this. Many parts of GitHub's JS are open source, so I wonder if they may have their terminal rendering also available somewhere 😉.

@yp05327
Copy link
Contributor

yp05327 commented Mar 31, 2023

image
image
image
tested in 2604c22

@wxiaoguang
Copy link
Contributor Author

I think it's much better than before.

Do you prefer to see full progress list or the only one final line?

  • If you prefer to see the progress list, then current approach seems good enough.
  • If you prefer to see the only final line, I think it's still feasible, we can only take the last message after [0K.
    Then the docker pull .... line 3 becomes one line : Digest: .....
    And the apk add ... line 16 becomes one line: Executing busybox-......

@wxiaoguang wxiaoguang force-pushed the improve-action-log-line branch from 3b2e315 to 15dbf40 Compare April 1, 2023 05:29
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 1, 2023
@lunny lunny merged commit aa9c920 into go-gitea:main Apr 1, 2023
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 1, 2023
@wxiaoguang wxiaoguang deleted the improve-action-log-line branch April 1, 2023 13:02
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 3, 2023
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Update JS deps (go-gitea#23853)
  Added close/open button to details page of milestone (go-gitea#23877)
  Check `IsActionsToken` for LFS authentication (go-gitea#23841)
  Prefill input values in oauth settings as intended (go-gitea#23829)
  Display image size for multiarch container images (go-gitea#23821)
  Use clippie module to copy to clipboard (go-gitea#23801)
  Remove assertion debug code for show/hide refactoring (go-gitea#23576)
  [skip ci] Updated translations via Crowdin
  Remove jQuery ready usage (go-gitea#23858)
  Fix JS error when changing PR's target branch (go-gitea#23862)
  Improve action log display with control chars (go-gitea#23820)
  Fix review conversation reply (go-gitea#23846)
  Improve home page template, fix Sort dropdown menu flash (go-gitea#23856)
  Make first section on home page full width (go-gitea#23854)
  [skip ci] Updated translations via Crowdin
  Fix incorrect CORS failure detection logic (go-gitea#23844)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect line number display of process logs in actions
8 participants