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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support actions syntax tokens #29777

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Mar 13, 2024

TODO:

  • Make folding work, default to folded state
  • Recalculate rendered line numbers to exclude always-hidden lines
  • Figure out whether ::add-matcher and ::remove-matcher have any meaning to us
  • Implement all documented tokens
  • Error annotations ##[error]
  • Add tests
image

Also there seems to be some strange emoji-prefixed stuff which I think comes from either act_runner or act, I wonder what to do with it because I think these are invalid with that 馃挰 prefix.

  馃挰  ::debug::clean = true
  馃挰  ::debug::filter = undefined
  馃挰  ::debug::fetch depth = 1
  馃挰  ::debug::fetch tags = false

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 13, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 13, 2024
@silverwind silverwind marked this pull request as draft March 13, 2024 23:00
@a1012112796
Copy link
Member

full commands list is here: https://github.com/actions/toolkit/blob/main/docs/commands.md#problem-matchers

@silverwind silverwind added the type/enhancement An improvement of existing functionality label Mar 14, 2024
@wolfogre
Copy link
Member

Also there seems to be some strange emoji-prefixed stuff which I think comes from either act_runner or act, I wonder what to do with it because I think these are invalid with that 馃挰 prefix.

I can help remove the emoji prefix if it's really bothering. I noticed that too, but I didn't have the motivation to resolve it.

@silverwind
Copy link
Member Author

silverwind commented Mar 14, 2024

Also there seems to be some strange emoji-prefixed stuff which I think comes from either act_runner or act, I wonder what to do with it because I think these are invalid with that 馃挰 prefix.

I can help remove the emoji prefix if it's really bothering. I noticed that too, but I didn't have the motivation to resolve it.

Please do. I would code this matcher to only work when :: or [command] is at the beginning of the line, and I think such tokens in the middle of the line would also not work on GitHub, but I will check this as well what their renderer does in that case.

@wolfogre
Copy link
Member

wolfogre commented Mar 14, 2024

Also there seems to be some strange emoji-prefixed stuff which I think comes from either act_runner or act, I wonder what to do with it because I think these are invalid with that 馃挰 prefix.

I can help remove the emoji prefix if it's really bothering. I noticed that too, but I didn't have the motivation to resolve it.

Please do. I would code this matcher to only work when :: or [command] is at the beginning of the line, and I think such tokens in the middle of the line would also not work on GitHub, but I will check this as well what their renderer does in that case.

https://gitea.com/gitea/act/pulls/97

And I'll update act_runner later.


https://gitea.com/gitea/act_runner/pulls/515

TKaxv-7S pushed a commit to TKaxv-7S/gitea-act that referenced this pull request Mar 15, 2024
Remove emojis in command outputs; leave others since they don't matter.

Help go-gitea/gitea#29777

Reviewed-on: https://gitea.com/gitea/act/pulls/97
TKaxv-7S pushed a commit to TKaxv-7S/gitea-act that referenced this pull request Mar 25, 2024
Remove emojis in command outputs; leave others since they don't matter.

Help go-gitea/gitea#29777

Reviewed-on: https://gitea.com/gitea/act/pulls/97
@techknowlogick
Copy link
Member

act_runner has now received the updates

@ChristopherHX
Copy link
Contributor

ChristopherHX commented Mar 28, 2024

Figure out whether ::add-matcher and ::remove-matcher have any meaning to us

Those commands needs to be interpreted by act_runner/act.

I would like that ##[group] and ##[endgroup] would work as well

The actions/runner converts them into the alternative (aka azure devops notation) command syntax

https://gitea.com/ChristopherHX/actions_runner/actions/runs/97

##[group]Run actions/checkout@v3
with:
  fetch-depth: 0
  repository: ChristopherHX/actions_runner
  token: ***
  ssh-strict: true
  persist-credentials: true
  clean: true
  sparse-checkout-cone-mode: true
  fetch-tags: false
  lfs: false
  submodules: false
  set-safe-directory: true
##[endgroup]

Both act and actions/runner supports the two different command syntaxes.

Obviously I could tell my third party runner adapter to convert it back, not a big deal after all.

@silverwind
Copy link
Member Author

Figure out whether ::add-matcher and ::remove-matcher have any meaning to us

Those commands needs to be interpreted by act_runner/act.

Ok then we hide those completely on the UI.

@silverwind
Copy link
Member Author

silverwind commented Apr 27, 2024

Adding a note for another thing I discovered in the renderer:

Lines that match a certain regex with filename at the start are highlighted as error, but as seen below that regex of GitHub has a bug where it does not highlight column ranges.

No idea how those error annotations look in the raw format, but I expect there to be some token that can be matched.

image

@ChristopherHX
Copy link
Contributor

ChristopherHX commented Apr 28, 2024

The bug with line ranges is probably in this file https://github.com/actions/setup-go/blob/main/matchers.json

  1. setup-go prints ::add-matcher:: with a container file path
  2. runner reads it from the host by reverse mapping the container path (act could use docker cp like api)
  3. runner scans log output for all problem matchers / rewrites the log file and sends annotations for them as well

I'm not shure how it is possible to avoid replicating the line range bug without implementing builtin problem matchers on either UI or runner side.

E.g. the Gitea Workflow could register it's own problem matcher, which matches for line ranges

@silverwind
Copy link
Member Author

silverwind commented Apr 28, 2024

Interesting, I wouldn't have thought that it was setup-go that holds this regex. I do agree we should not try to fix this bug on our end, but fix in setup-go instead.

BTW, I think the format <linenum>:<colnum>-<colnum> that gopls uses is not ideal because it would not allow to make a multi-line annotation. A format like <linenum>-<colnum>:<linenum>-<colnum> would arguably be better, but I'm not sure whether the LSP protocol would allows it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/js size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants