Build: Handle special characters in path correctly #3583
Conversation
Escape everything by enclosing the path in single quotes. Now the only character left that needs to be escaped is single quote. Interestingly, the proper way to escape it seems to be '\''
klimeryk
added
[Type] Bug
[Status] Needs Review
| THIS_DIR:=$(shell cd $(dir $(THIS_MAKEFILE_PATH));pwd) | ||
| SINGLE_QUOTE := ' | ||
| # We need to escape single quote as '\'' | ||
| THIS_DIR := '$(subst $(SINGLE_QUOTE),$(SINGLE_QUOTE)\$(SINGLE_QUOTE)$(SINGLE_QUOTE),$(CURDIR))' |
There was a problem hiding this comment.
CURDIR is a variable set by make pointing to the current working directory.
I don't see much sense in jumping through hoops to get the CWD, when we don't actually support invoking make from any other directory than the root of the project (where the Makefile resides).
There was a problem hiding this comment.
There are many makefiles in the project that allows you to run local tests.
There was a problem hiding this comment.
True, but those Makefiles are independent of this one. So running make test directly from the directory with the tests obviously works as it should. As for make test from the root - that actually calls make -C /path/to/test - so, again, only the local Makefile will be used.
Just to confirm, I've tried both make test from the root directory to run all tests and make test in a subdirectory where some tests lie - both cases are working. But thanks for reminding me of this use case - it's good to double check stuff like that :)
There was a problem hiding this comment.
As a side note - all those individual Makefiles could be replaced in 99% of cases by a single, common Makefile 😁 One of those days, I'll find some time to create a PR for that.
There was a problem hiding this comment.
As a side note - all those individual Makefiles could be replaced in 99% of cases by a single, common Makefile One of those days, I'll find some time to create a PR for that.
I would hope to get rid of all the Makefiles we use for testing
There was a problem hiding this comment.
Yeah, one wonderful day we'll get #2108 resolved 😉
|
Tested on OSX with a path like: HotReload doesn't work still. When a jsx file is edited, the console doesn't show up the "INVALID" message that highlights the correct watch triggering the reload process. |
folletto
added
[Status] Needs Author Reply
and removed
[Status] Needs Review
|
Ah, damn hot reloading... I've investigated this and it seems like it's a bug somewhere higher up, not in Calypso. I've tried |
|
I see, shame. :( Thanks for having looked into this. :) |
klimeryk
added
[Status] Needs Review
|
looks good, 🚢 |
blowery
added
[Status] Ready to Merge
and removed
[Status] Needs Review
|
Since this is a few weeks old, you might want to rebase it first and verify it still passes tests on a current master. |
…s-in-path Build: Handle special characters in path correctly
This PR fixes any outstanding and possible problems with special characters in the path to the directory where Calypso has been cloned into.
Basically, we enclose the path in single quotes - this way the shell won't treat the characters in it in any special way. Now the only character left that needs to be escaped is the single quote:
'. Interestingly, the proper way to escape it seems to be'\''Tested on:
Fixes #100 and #1345