Skip to content

Adapt extension of R executable depending on os#2605

Merged
asottile merged 1 commit intopre-commit:mainfrom
lorenzwalthert:r/fix-exe
Dec 12, 2022
Merged

Adapt extension of R executable depending on os#2605
asottile merged 1 commit intopre-commit:mainfrom
lorenzwalthert:r/fix-exe

Conversation

@lorenzwalthert
Copy link
Copy Markdown
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
@lorenzwalthert
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@lorenzwalthert
Copy link
Copy Markdown
Contributor Author

ok, will revert later.

@lorenzwalthert
Copy link
Copy Markdown
Contributor Author

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

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

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

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