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

pre-commit autoupdate #501

Merged
merged 3 commits into from
Nov 24, 2023
Merged

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Nov 24, 2023

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
Copy link

codecov-commenter commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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 Coverage Δ
cpp 85.72% <ø> (ø)
python_and_cython 95.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Changes produced by `pre-commit autoupdate`.

Signed-off-by: Christian Clauss <[email protected]>
@godlygeek godlygeek force-pushed the pre-commit-autoupdate branch from 6e04372 to f4204bc Compare November 24, 2023 19:13
@godlygeek
Copy link
Contributor

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

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

@cclauss
Copy link
Contributor Author

cclauss commented Nov 24, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

cclauss commented Nov 24, 2023

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

@godlygeek godlygeek force-pushed the pre-commit-autoupdate branch 2 times, most recently from 59d2d00 to bacdd51 Compare November 24, 2023 20:27
It seems that it wants to pick up a binary called `nodejs`, while
`actions/setup-node` is only installing one called `node`.

Let's hack around this with a symlink.

Signed-off-by: Matt Wozniski <[email protected]>
@godlygeek godlygeek force-pushed the pre-commit-autoupdate branch from bacdd51 to e9bc3d7 Compare November 24, 2023 20:55
@godlygeek
Copy link
Contributor

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
Copy link
Contributor

I've entered actions/setup-node#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)

Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, my terrible hack works well enough, so LGTM. Thanks @cclauss!

@godlygeek godlygeek merged commit 1cce4cb into bloomberg:main Nov 24, 2023
34 checks passed
@cclauss cclauss deleted the pre-commit-autoupdate branch November 24, 2023 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants