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

Adapt extension of R executable depending on os #2605

Merged
merged 1 commit into from Dec 12, 2022

Conversation

lorenzwalthert
Copy link
Contributor

@lorenzwalthert lorenzwalthert commented Nov 19, 2022

Closes #2599 by adding ensuring .exe suffix to the Rscript executable on Windows. But I don't understand if that change breaks current uses or if some versions of Windows / shells just add `.exe. when it's missing while others don't.

Also, I am not sure if it also closes lorenzwalthert/precommit#441 or if the added ~ in the Rscript path is of any harm or not and ctypes.windll.kernel32.GetLongPathNameW from the standard library is needed to use get full path.

@lorenzwalthert lorenzwalthert changed the title Adapt extension depending on os Adapt extension if R executable depending on os Nov 20, 2022
@lorenzwalthert lorenzwalthert changed the title Adapt extension if R executable depending on os Adapt extension of R executable depending on os Nov 20, 2022
pre_commit/languages/r.py Outdated Show resolved Hide resolved
@lorenzwalthert
Copy link
Contributor Author

lorenzwalthert commented Nov 25, 2022

Since the string "Rscript" is actually used in many places in languages/r.py, I turned it into a variable. Should I import it in the tests as well instead of re-defining it inline? Let me know if these changes are already too convoluted for you.

@asottile
Copy link
Member

I don't think the constant improves things. this should just be a 2 line change (import + changing the call)

@lorenzwalthert
Copy link
Contributor Author

ok, will revert later.

@lorenzwalthert
Copy link
Contributor Author

Seems like the swift installation (unrelated to this PR) is broken...

@asottile
Copy link
Member

asottile commented Dec 6, 2022

Seems like the swift installation (unrelated to this PR) is broken...

yeah there's like ~4 things that have slipped -- I wouldn't worry about it -- I'll be fixing those separately

@lorenzwalthert
Copy link
Contributor Author

Ok. I think my part is done, I'll rebase once main is passing.

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile merged commit 3aa6206 into pre-commit:main Dec 12, 2022
@lorenzwalthert lorenzwalthert deleted the r/fix-exe branch December 12, 2022 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Rscript not found in Windows Rscript not found
2 participants