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: use relative paths to working directory in annotations #116

Conversation

mskrajnowski
Copy link

pyright outputs absolute paths in diagnostics and that's what is currently provided when creating GitHub annotations.

When a working directory is provided, which is outside of the GitHub workspace path, annotations would show up in the workflow summary, but wouldn't appear in the diff.

This PR changes the absolute paths in annotations to relative to the specified working directory to fix this problem.

Absolute paths will still be reported when working-directory is omitted. I tried to change that, but some tests failed and I didn't want to make broad changes.

@mskrajnowski mskrajnowski force-pushed the fix/relative-paths-in-annotations branch 2 times, most recently from c4abf54 to 2c0260d Compare May 29, 2024 11:07
@jakebailey
Copy link
Owner

Can you please undo the formatting changes? Not sure why the code was entirely reformatted.

Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3bdde3b) to head (f481bae).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #116   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          664       666    +2     
  Branches       137       166   +29     
=========================================
+ Hits           664       666    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mskrajnowski mskrajnowski force-pushed the fix/relative-paths-in-annotations branch from 2c0260d to f90ec1c Compare May 29, 2024 11:12
@jakebailey
Copy link
Owner

There are still a lot of random formatting changes which obscure the fix.

@mskrajnowski mskrajnowski force-pushed the fix/relative-paths-in-annotations branch from f90ec1c to f481bae Compare May 29, 2024 11:15
@mskrajnowski
Copy link
Author

mskrajnowski commented May 29, 2024

Yes, sorry, had prettier enabled by default in VSCode, should be good now

@jakebailey
Copy link
Owner

Thanks.

It's not clear to me how this is correct; the working directory option is pyright-action specific, so how does GitHub understand anything's about the annotations being relative to any random dir like this? I could be in a deeply nested dir and emit an annotation for main.py and this PR seemingly makes it annotate main.py versus /path/to/main.py, which doesn't seem specific enough... There could be another file with that name somewhere else.

@mskrajnowski
Copy link
Author

In my case I'm running pyright-action from within a container that already has all the dependencies and code in it, e.g. in /opt/example and I'm using working-directory to point pyright-action there so it doesn't use the default workspace path.

I agree that this wouldn't work for running pyright in nested directories though. Would you consider adding a separate input to change the project root path, which would be used for relative paths instead?

@mskrajnowski
Copy link
Author

Also, thanks for the very quick response

@jakebailey
Copy link
Owner

Can you provide a link to a workflow where this isn't working today?

Would you consider adding a separate input to change the project root path, which would be used for relative paths instead?

I'm not 100% sure what you mean by this, but it sounds like you're asking for a flag which just does that this PR does conditionally, but it's not clear to me how this relative path stuff even works period.

I can imagine needing to do things relative to the original working directory for some weird reasons, but no matter what that sure sounds like a workaround for a GitHub bug or something...

@mskrajnowski
Copy link
Author

Will create a small example repo and get back to you. I'll switch the PR to draft for now.

@mskrajnowski
Copy link
Author

mskrajnowski commented May 29, 2024

Added demo PRs:

Let me know if you can access workflow logs. In both you can see the errors in the workflow summary, but only the second one has the annotations in the diff.

Basically, GitHub has no way of knowing that /opt/demo/demo.py is ./demo.py in the repository.

@mskrajnowski
Copy link
Author

I assume GitHub actions has some magic that converts absolute paths within the workspace to relative paths, which is why it works now.

@mskrajnowski
Copy link
Author

Alternative solution, which adds workspace-directory input:

@jakebailey
Copy link
Owner

Ah, so this is actually to do with the fact that this workflow is running within a docker container?

I can see how this wouldn't be working right, yeah. I guess GitHub's own annotation examples don't indicate that the paths can be absolute. There may be a more general fix that needs to happen here; working-directory is an option specific to pyright-action, but maybe I should actually not do this at all and instead always make paths relative to $GITHUB_WORKSPACE...

@jakebailey
Copy link
Owner

The problem I can see is that it's possible to say, actions/checkout to a directory. What kind of path is GitHub expecting there?

@jakebailey
Copy link
Owner

Is there any chance you could rerun the "broken" workflow again with debug logging enabled? There should be a button for it at the top right of https://github.com/codequest-eu/pyright-action/actions/runs/9286266122/job/25552623528?pr=3

@mskrajnowski
Copy link
Author

Is there any chance you could rerun the "broken" workflow again with debug logging enabled?

done https://github.com/codequest-eu/pyright-action/actions/runs/9286266122/job/26018045227

maybe I should actually not do this at all and instead always make paths relative to $GITHUB_WORKSPACE...

The problem I can see is that it's possible to say, actions/checkout to a directory. What kind of path is GitHub expecting there?

Looking at actions/checkout docs they only support relative paths from the $GITHUB_WORKSPACE.

actions/checkout itself is currently quite broken when using containers and a non-root user (e.g. actions/checkout#1487, actions/checkout#1575), which is why I'm putting everything that's needed for the workflow in the image and ignoring the $GITHUB_WORKSPACE.

However, if you decide that this is not a supported use case then that's perfectly fine.

In the meantime, I'll try the suggestion from actions/checkout#841 (comment) to use the root user in the container, which might fix actions/checkout and then I should be able to run pyright-action from within the workspace.

@mskrajnowski
Copy link
Author

mskrajnowski commented Jun 10, 2024

Ran my workflow as root with actions/checkout in a container and everything seems to be working fine without passing a working-directory to pyright-action. Annotations in the diff are present.

I don't like running containers as root, but in this context it might be good enough. Closing the PR, since it might overcomplicate this action for a niche use case.

@jakebailey
Copy link
Owner

done codequest-eu/pyright-action/actions/runs/9286266122/job/26018045227

Thanks for running that; annoyingly, they don't seem to log anything about how they're handling the annotations.

Looking at actions/checkout docs they only support relative paths from the $GITHUB_WORKSPACE.

I was mainly meaning that in context of the annotations, but it's definitely telling that they seem to want you to keep your code in GITHUB_WORKSPACE, yeah.

I do think there's a real problem here, though; definitely need to figure this out, but will definitely take me attempting to reverse engineer it a bit...

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.

2 participants