Closes #797: Comment output of cachegrind in PR #932
Conversation
|
Please review this PR @rakita :) |
.github/workflows/cachegrind.yml
Outdated
| valgrind --tool=cachegrind target/release/snailtracer | ||
|
|
||
| # Extract relevant information from cachegrind.out file | ||
| cg_annotate cachegrind.out.* > cachegrind_results.txt |
There was a problem hiding this comment.
We don't need this file but just the output of the Valgrind.
Something like this:
valgrind --tool=cachegrind target/release/snailtracer 2>&1 | tail -n 1 | tee cachegrind_results.txt
Otherwise it looks nice
There was a problem hiding this comment.
why just the first line?
There was a problem hiding this comment.
It is more resonable then 64k and most important, but i like what you did in the end.
There was a problem hiding this comment.
it was just the first 64k characters not lines!
looks less spammy :P
.github/workflows/cachegrind.yml
Outdated
|
|
||
| - name: Comment on PR | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
idk but shouldn't we have permission here for token to work? https://docs.github.com/en/actions/managing-issues-and-pull-requests/commenting-on-an-issue-when-a-label-is-added
There was a problem hiding this comment.
works perfectly on my fork. i wonder if it would continue to work on another PR post merge?
There was a problem hiding this comment.
0x0elliot#1 (comment) here is the comment from my test PR
There was a problem hiding this comment.
Yeap we can check after it is merged.
There is --edit-last flag that could stop the spam. Did you check it out?
https://cli.github.com/manual/gh_pr_comment
0x0elliot
force-pushed
the
0x0elliot/output-cachegrind
branch
2 times, most recently
from
320cc6c
to
acd1553
Compare
|
amending on my commit to avoid commit spam as i try to test this :) |
0x0elliot
force-pushed
the
0x0elliot/output-cachegrind
branch
from
acd1553
to
e7d1409
Compare
|
This looks pretty neat now 
|
0x0elliot
force-pushed
the
0x0elliot/output-cachegrind
branch
from
e7d1409
to
c31726a
Compare
|
@rakita done. we can debug post merge. 
|
|
ah yeah --edit-last fails if a comment hasn't already been made. let me fix that. |
|
should be fine now. let me test with user permissions on my fork further so that we don't have to debug. |
0x0elliot
force-pushed
the
0x0elliot/output-cachegrind
branch
2 times, most recently
from
bafc712
to
e68ad97
Compare
|
okay, maybe try adding a PAT post merge? @rakita my fork also encounters a same error when my friend makes a PR with his account. my PRs work fine with this workflow. this person has the same bug. |
0x0elliot
force-pushed
the
0x0elliot/output-cachegrind
branch
from
e68ad97
to
cc374bb
Compare
Okay, will merge this, and check if permissions work. If not, will add |
Tried a bunch of things. Dealing with file uploads didn't seem so simple in comments. There is a character limit (~65k characters), so the simplest thing to do was to comment the first 65k characters :)
Let me know if that's not good enough and we can make the experience with PRs better. I am toying around with adding in pastebin API there to make dev UX better but I don't want you guys to deal with it.
cli/cli#4465 (comment) Sadly this restricted me further. I am open to using third party APIs if you're comfortable to make it simple for us. Open to suggestions and feedback :)
Another solution is using
-- Auto-annotated source:as a delimiter and commenting every time we see it in the output. That might lead to a spam of comments however!