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

No Check Annotation for compile error in C# project #17

Closed
rainersigwald opened this issue Aug 14, 2019 · 10 comments · Fixed by #63
Closed

No Check Annotation for compile error in C# project #17

rainersigwald opened this issue Aug 14, 2019 · 10 comments · Fixed by #63
Assignees

Comments

@rainersigwald
Copy link
Contributor

rainersigwald commented Aug 14, 2019

I introduced a simple compile error, which seems to have been matched by the problem matcher (it turned red in the console output view and is prefixed with ##[error]), but there was no annotation linking the error to the file/line number information that is contained in the line.

https://github.com/rainersigwald/bumblebee/pull/6/checks?check_run_id=192916618#step:4:32

I hoped for the sort of linking in the Files Changed tab of the PR that are described in the Checks CI docs.

This might be because the error includes only a relative-to-the-project filename, rather than relative-to-the-repo-root?

@rainersigwald
Copy link
Contributor Author

Configuring the compiler to emit absolute paths as here did not resolve the problem.

@damccorm
Copy link
Contributor

@ericsciple could you take a look? It looks to me like the regex should match this so I'm not totally sure why we're seeing this behavior.

@ericsciple
Copy link
Contributor

@rainersigwald sorry i didnt see this issue before now.

The relative path not lining up makes sense. The runner attempts to root relative paths to the repo root, and if that path doesnt exist then the file property gets dropped.

iirc the runner sets an environment variable github=true, so defaulting to absolute when that is set, might be something to consider.

However, I am surprised that emitting absolute paths did not resolve the problem. As long as the path is under the repo root, that should work. Thanks for the links, i'll fork your repro and play around with it.

@ericsciple
Copy link
Contributor

@ZEisinger looking at the matcher for v1, it looks like it should look something more like this:

"pattern": [
  {
    "regexp": "^\\s*([^:]+)\\((\\d+),(\\d+)\\): (error|warning) ([^:]+): (.*) \\[(.+)\\]$",
    "file": 1,
    "line": 2,
    "column": 3,
    "severity": 4,
    "code": 5,
    "message": 6,
    "fromPath": 7
  }
]

frompath will root the relative file

also i noticed location isnt a thing, so line/column instead

@ericsciple
Copy link
Contributor

...although the annotation still didnt come out correct... debugging the runner now

@ericsciple
Copy link
Contributor

@ZEisinger I opened a pr to fix fromPath in the runner. There is a bug in the runner where it currently does not trim the file name (csproj) before rooting the file. I'll sync w/ folks tomorrow to get a better idea when we can roll the fix out to prod.

You may not want to update the matcher configuration to include fromPath, until the runner fix is deployed.

@ericsciple
Copy link
Contributor

ericsciple commented Dec 14, 2019

The runner has rolled out everywhere now and the fromPath field can be used. Sorry for the delay we had a freeze for a while and was waiting for the new runner to roll out.

@ericsciple
Copy link
Contributor

@ZEisinger i'm unable to assign a different assignee. The runner changes are in now.

@StanleyGoldman
Copy link
Contributor

I tested against this regex: https://regex101.com/r/MkisH3/4

@rainersigwald
Copy link
Contributor Author

@StanleyGoldman Thanks! I updated my test PR to use the latest master and everything looks good with and without configuring MSBuild to emit absolute paths.

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 a pull request may close this issue.

4 participants