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

don't use system for ruby/node if it is a shim exe #1668

Merged
merged 1 commit into from Oct 29, 2020
Merged

Conversation

asottile
Copy link
Member

Resolves #1658

@avdv
Copy link

avdv commented Jun 10, 2021

Hi.

This is a really disruptive change IMO, which - from the discription - should only have an impact for ruby and node shims.

But basically this change disallows any binaries installed inside the home directory. Is this intentional?

Asking because pre-commit failed installing a hook, since it could not find a binary which is available in my path, at ~/.nix-profile/bin and symlinked to the real binary somewhere else. Lost an hour to find the reason for this...

@asottile
Copy link
Member Author

asottile commented Jun 10, 2021

This is a really disruptive change IMO, which - from the discription - should only have an impact for ruby and node shims.

this is correct, it only affects ruby and node as the code here is only used for ruby and node

But basically this change disallows any binaries installed inside the home directory.

if you read the change, it does not do that, it must contain both /shims/ and be in the home directory

it also doesn't ban them -- it just turns off the automatic language_version: system when a suitable binary is detected

Lost an hour to find the reason for this...

I suspect you have an XY problem -- instead of prescribing a solution please describe your exact reproducible problem and then I can help you

@avdv
Copy link

avdv commented Jun 10, 2021

if you read the change, it does not do that, it must contain both /shims/ and be in the home directory

I think that's wrong. The exe_exists function only returns true if the path does not contain shim and the path is not inside the home dir.

Should this not rather be something like return not (SHIMS_RE.search(found) and common != homedir) -- which is basically what you said above? nonsense, ignore... wait a second.

return not (SHIMS_RE.search(found) and common == homedir)

This should do it. What you said above... right?!

@asottile
Copy link
Member Author

no it's meant to ban both homedir and shims executables

please describe your actual problem behaviour instead of solution hunting, I suspect you're lost and trying to find something which isn't causing your problem and wasting all of our time

@avdv
Copy link

avdv commented Jun 10, 2021

no it's meant to ban both homedir and shims executables

But why? The problem started with something specific to rbenv shims. Now, all binaries inside the home dir (for node and ruby) are banned. Why?

please describe your actual problem behaviour instead of solution hunting, I suspect you're lost and trying to find something ? which isn't causing your problem and wasting all of our time

Using downloaded binaries does not usually work on my system. So, having nodeenv fetch some arbirtary binary from the net (II suspect it does that) will fail. I was using pre-commit 2.7 before it everything worked smoothly. Now pre-commit failed because of this disruptive change.

@asottile
Copy link
Member Author

the reason is home-directory installed binaries are likely to target shifting sands, breaking environments after the fact -- the specific issue was about rbenv which makes it very easy to do this but the problem persists for other managers as well

you can easily restore the previous default with:

default_language_version:
    node: system

@asottile
Copy link
Member Author

also, have some empathy "this disruptive change" is not appropriate language. you're getting free software and you're not sponsoring so I don't have to take this attitude from you

@pre-commit pre-commit locked as resolved and limited conversation to collaborators Jun 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

rbenv FileNotFoundError when installing markdownlint hook
2 participants