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

Fix handling of whitespace in Makevars filename #160

Merged
merged 6 commits into from
Mar 31, 2021

Conversation

klmr
Copy link
Contributor

@klmr klmr commented Mar 30, 2021

This PR adds escaping of spaces in path names that are written to the Makevars file used by ‘cpp11’ for compilation.

I came across this issue because I’m keeping my R package library on macOS in the standard config location, which is ~/Library/Application Support (note the space). Since ‘cpp11’ adds its own installation directory to the include path, this consequently causes an error when invoking e.g. cpp_source(), which looks as follows:

clang: error: no such file or directory: 'Support/R/4.0/library/cpp11/include'
make: *** [/private/var/folders/dr/rp46tjqs5qzfj_ghjpxqrttr0000gn/T/RtmpMtBJTo/file13e521450b665/src/code_13e523ef6ab4e.o] Error 1
Error: Compilation failed.

@jimhester
Copy link
Member

jimhester commented Mar 30, 2021

I think I would prefer to handle this by unconditionally surrounding the paths in single quotes at

out[[i]] <- paste0("-I", path)

Also can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

@klmr
Copy link
Contributor Author

klmr commented Mar 30, 2021

How’s this?

@jimhester
Copy link
Member

Looks good except that windows apparently can't handle the single quotes 🙈

We could try double quotes, though then I worry POSIX systems will interpolate variables in them. I guess we might have to live with the backslashes...

@klmr
Copy link
Contributor Author

klmr commented Mar 30, 2021

We could try double quotes, though then I worry POSIX systems will interpolate variables in them. I guess we might have to live with the backslashes...

What about shQuote? It should handle this.

@jimhester
Copy link
Member

Sure, shQuote seems like a good idea

@klmr
Copy link
Contributor Author

klmr commented Mar 31, 2021

Hmm. The check failure on Windows (release) seems unrelated (?). But it still fails on Windows (3.6). :-(

@klmr
Copy link
Contributor Author

klmr commented Mar 31, 2021

I’m not entirely sure what the issue is but it seems that Make on Windows doesn’t like double quotes. So, should I revert back to the first version (escape whitespace with backslashes)?

(If that’s alright I’ll push a new commit rather than amending and force-pushing — when squashing the PR the effect would be the same.)

@jimhester
Copy link
Member

Yeah, we can revert back to using backslashes and I will squash this before merging.

@klmr
Copy link
Contributor Author

klmr commented Mar 31, 2021

Alright, back to escaping whitespace. And I noticed that my original version actually didn’t escape anything (oops!), since \ inside a replacement string needs to be escaped twice. My original (manual) testing failed to catch this since the dev version of ‘cpp1’ was installed in a path without spaces (I changed that afterwards to test properly).

… but I’m wondering if this should be caught by a unit test. Unfortunately the only way to test this that I can think of is to create a copy of the whole repo during testing (and put it into a path with spaces), which feels like overkill. … Maybe with vendoring?

In f6f8772 we accidently were calling shQuote twice on windows, which is
why it failed.
@jimhester jimhester merged commit 0fa6774 into r-lib:master Mar 31, 2021
@jimhester
Copy link
Member

Looking at this again I think the problem with f6f8772 is you were accidentily calling shQuote twice on windows, once in the conditional and once outside it, which is why it failed. I went back to that change and added some tests, so I will merge this, thanks again for opening the PR!

@jimhester
Copy link
Member

Also at least on my macOS system ~/Library/ApplicationSupport without a space is a symlink to ~/Library/Application Support. I don't think I added this symlink myself, so you might save yourself grief by using that as your library path rather than the one with a space.

@klmr
Copy link
Contributor Author

klmr commented Mar 31, 2021

you were accidentily calling shQuote twice on windows

Oh wow, I completely missed that. Thanks for spotting that, and for adding the tests.

Also at least on my macOS system ~/Library/ApplicationSupport without a space is a symlink to ~/Library/Application Support.

Yes, that symlink is always there and I fall back to it when I need to — but by default I use the canonical path intentionally to flush out just this kind of issue during development. There are actually very few issues: In my experience, Make is the only modern, widely used tool that doesn’t support spaces.

@klmr klmr deleted the fix-makevars branch March 31, 2021 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants