bloomberg / memray

Memray is a memory profiler for Python
https://bloomberg.github.io/memray/
Apache License 2.0
13.17k stars 392 forks source link

pre-commit autoupdate #501

Closed cclauss closed 10 months ago

cclauss commented 10 months ago

Issue number of the reported bug or feature request: #

Describe your changes A clear and concise description of the changes you have made. % pre-commit autoupdate

Testing performed Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned. % pre-commit run --all-files

Additional context Add any other context about your contribution here.

codecov-commenter commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (138d9c5) 92.24% compared to head (e9bc3d7) 92.24%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #501 +/- ## ======================================= Coverage 92.24% 92.24% ======================================= Files 91 91 Lines 11033 11033 Branches 1524 1524 ======================================= Hits 10177 10177 Misses 851 851 Partials 5 5 ``` | [Flag](https://app.codecov.io/gh/bloomberg/memray/pull/501/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | Coverage Δ | | |---|---|---| | [cpp](https://app.codecov.io/gh/bloomberg/memray/pull/501/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `85.72% <ø> (ø)` | | | [python_and_cython](https://app.codecov.io/gh/bloomberg/memray/pull/501/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `95.48% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

godlygeek commented 10 months ago

Whoops, both of us made a change at the same time there.

@cclauss Why the downgrade of mirrors-prettier to v2.7.1 ?

cclauss commented 10 months ago

Why the downgrade of mirrors-prettier to v2.7.1 ?

https://github.com/bloomberg/memray/actions/runs/6984049688/job/19006277652 which made no sense to me because Node.js v16 was indeed installed.

godlygeek commented 10 months ago

Ah, "prettier requires at least version 14 of Node, please upgrade". Hm. But we're installing Node 16...

Run actions/setup-node@v4
  with:
    node-version: 16
    always-auth: false
    check-latest: false
    token: ***
Found in cache @ /opt/hostedtoolcache/node/16.[2](https://github.com/bloomberg/memray/actions/runs/6984188977/job/19006591456?pr=501#step:3:2)0.2/x6[4](https://github.com/bloomberg/memray/actions/runs/6984188977/job/19006591456?pr=501#step:3:4)
Environment details
  node: v1[6](https://github.com/bloomberg/memray/actions/runs/6984188977/job/19006591456?pr=501#step:3:6).20.2
  npm: [8](https://github.com/bloomberg/memray/actions/runs/6984188977/job/19006591456?pr=501#step:3:9).1[9](https://github.com/bloomberg/memray/actions/runs/6984188977/job/19006591456?pr=501#step:3:10).4
  yarn: 1.22.21

Hm. I guess pre-commit isn't picking up the version that we installed...

cclauss commented 10 months ago

I tend to use https://pre-commit.ci instead of GHA to run pre-commit.

godlygeek commented 10 months ago

I think I've got it. nodeenv prefers a binary named nodejs over one named node:

https://github.com/ekalinin/nodeenv/blob/eaa9de97e561ab4f99458c94633e92547e72d5f1/nodeenv.py#L931-L938

And actions/setup-node isn't installing a nodejs, but there is one in /usr/bin on the runner, so that's getting chosen over the node we've just installed.

It seems we can hack around it by symlinking the node we install to nodejs, which, yuck, but oh well it seems to work...

godlygeek commented 10 months ago

I've entered https://github.com/actions/setup-node/issues/905 for that since that doesn't seem like expected behavior (though I'm not a JS person so it's entirely possible this is a nodeenv problem and not an actions/setup-node problem... we'll see)