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

Makefile variables need quotes to properly handle system paths with spaces (Windows/WSL2 + Docker) #317

Closed
aidedinertial opened this issue Jan 8, 2024 · 1 comment · Fixed by #335

Comments

@aidedinertial
Copy link

Description of Issue

Building firmware on the V3.0 branch at commit 1728a66 Prefer tr to ${char^^}, which does not work on older bash versions (#303).

Building on Windows/WSL2 with Docker:

$make

Produces the error:

/mnt/c/Program Files/Docker/Docker/resources/bin/docker build --tag zmk --file Dockerfile .
make: /mnt/c/Program: No such file or directory
make: *** [Makefile:17: all] Error 127

The issue is that the docker path has spaces:

$command -v docker

Returns the output

/mnt/c/Program Files/Docker/Docker/resources/bin/docker

This only seems to happen when Docker is in "Resource Saver Mode". When the docker engine is running, the command returns

$command -v docker
/usr/bin/docker

Suggested Fix

Recommended to wrap shell variables in quotation marks.

diff --git a/Makefile b/Makefile
index e5231eb..4b31591 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
-DOCKER := $(shell { command -v podman || command -v docker; })
-TIMESTAMP := $(shell date -u +"%Y%m%d%H%M")
-COMMIT := $(shell git rev-parse --short HEAD 2>/dev/null)
-detected_OS := $(shell uname)  # Classify UNIX OS
+DOCKER := "$(shell { command -v podman || command -v docker; })"
+TIMESTAMP := "$(shell date -u +"%Y%m%d%H%M")"
+COMMIT := "$(shell git rev-parse --short HEAD 2>/dev/null)"
+detected_OS := "$(shell uname)" # Classify UNIX OS
 ifeq ($(strip $(detected_OS)),Darwin) #We only care if it's OS X
@ReFil
Copy link
Collaborator

ReFil commented Jan 11, 2024

Hi, thank you for reporting this, i'll get right on fixing this

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 a pull request may close this issue.

2 participants