make: bail out if there are spaces in the path #5221
Conversation
|
Is checking them all at once robust? |
| @@ -42,6 +42,10 @@ BINDIRBASE := $(abspath $(BINDIRBASE)) | |||
| BINDIR ?= $(BINDIRBASE)/$(BOARD) | |||
| BINDIR := $(abspath $(BINDIR))/ | |||
|
|
|||
| ifneq (9, $(words ${RIOTBASE} ${CCACHE_BASEDIR} ${RIOTCPU} ${RIOTBOARD} ${RIOTPROJECT} ${RIOTPKG} ${APPDIR} ${BINDIRBASE} ${BINDIR})) | |||
There was a problem hiding this comment.
please add a substitution for each variable in case it is undefined or empty:
ifneq (9, $(words ${RIOTBASE}:-default ${CCACHE_BASEDIR}:-default ${RIOTCPU}:-default ${RIOTBOARD}:-default ${RIOTPROJECT}:-default ${RIOTPKG}:-default ${APPDIR}:-default ${BINDIRBASE}:-default ${BINDIR}:-default))
There was a problem hiding this comment.
My logic was borken. Now I apply each variable individually to $(words) and test if any outcome is unequal 1.
|
@gebart, no my check was not rebust at all. Fixed. Also I noticed a non-absolute BINDIR leads to messed up build. It is ensured now, too, that variables are absolute, and end with a slash if they need to. |
|
If the paths have to end with a slash or no slash then it is a bug in the build system. Where does it fail if the path contains an extra slash in the middle? Normally /bin/ls and /bin//ls point to the same file, same as /bin/./ls as well. Additionally, abspath will filter away all extra slashes and relative directories. You could do an override MYDIR:=$(abspath ${MYDIR}) to always get a consistent state with regards to slashes |
|
If you use |
|
Seems like |
|
hmm, the former implementation was much less intrusive (: |
|
Now it works. I think it's not even ugly. :) |
|
|
||
| GITCACHE:=$(RIOTBASE)/dist/tools/git/git-cache | ||
| # Path to the current directory relative to the git root | ||
| BUILDRELPATH ?= $(shell git rev-parse --show-prefix) |
There was a problem hiding this comment.
this makes it necessary to have a .git directory in our root, and to have git installed.
Why not just ${PWD#$RIOTBASE} ?
There was a problem hiding this comment.
I only moved the line.
|
Please review again. |
Kijewski
added
the
CI: needs squashing
|
doesn't seem to break anything for me. ACK please squash |
Kijewski
removed
the
CI: needs squashing
kaspar030
added
the
CI: ready for build
|
Squashed |
kaspar030
added
CI: ready for build
| ifneq (, $(filter-out 1, $(foreach v,${__DIRECTORY_VARIABLES},$(words ${${v}})))) | ||
| $(info Aborting compilation for your safety.) | ||
| $(info Related variables = ${__DIRECTORY_VARIABLES}) | ||
| $(error Make sure no path override is empty or contains spaces!) |
There was a problem hiding this comment.
Wouldn't this fail on many Windows systems?
There was a problem hiding this comment.
If there are spaces in the path, yes, that's the point. :) Otherwise Windows' directory layout got smarter, e.g. $HOME is C:\Users$ID, Eigene Daten is actually $HOME/Documents. There should be no spaces in your common folders unless you put one in there.
There was a problem hiding this comment.
@Kijewski I am just wondering: would it be "safer" if this check is done after making all paths absolute below?
There was a problem hiding this comment.
Yes, that would catch the unlikely case that (RIOTBASE != PWD) && !contains_space(RIOTBASE) && contains_space(PWD). Will change this evening.
There was a problem hiding this comment.
Will change this evening.
@Kijewski it's now or never (for this release at least) (;
If we use `rm -r(f)` then we should make sure that we don't delete random directory trees because there was a whitespace in the path.
|
Addressed @cgundogan's comment, squashed directly. |
|
nice! |
cgundogan
added
the
Process: needs backport
|
ACK and GO |
cgundogan
removed
the
Process: needs backport
make: bail out if there are spaces in the path (backport of #5221)
If we use
rm -r(f)then we should make sure that we don't delete random directory trees because there was a whitespace in the path.Related story: “In A UEFI World, "rm -rf /" Can Brick Your System”.