tcgoetz / GarminDB

Download and parse data from Garmin Connect or a Garmin watch, FitBit CSV, and MS Health CSV files into and analyze data in Sqlite serverless databases with Jupyter notebooks.
GNU General Public License v2.0
1.18k stars 142 forks source link

`bash` path doesn't work on macOS #170

Closed saiwing-yeung closed 1 year ago

saiwing-yeung commented 1 year ago

Describe the bug Rebuilding the db results in a "Command not found" error.

To Reproduce Steps to reproduce the behavior:

make rebuild_dbs
fitbit.py --rebuild_db
make: /usr/local/bin/bash: Command not found
make: /usr/local/bin/bash: Command not found
make: /usr/local/bin/bash: No such file or directory
make: *** [rebuild_fitbit] Error 1

Expected behavior Database get rebuilt.

Additional context Modifying defines.mk like this fixes it on macOS, but it'd probably break other platforms.

diff --git a/defines.mk b/defines.mk
index 5102004..acc8b2c 100644
--- a/defines.mk
+++ b/defines.mk
@@ -10,13 +10,13 @@ PLATFORM=$(shell uname)

 ifeq ($(PLATFORM), Linux)

-SHELL=/usr/bin/bash
+SHELL=/bin/bash
 TIME ?= $(shell which time)
 YESTERDAY = $(shell date --date yesterday +"%m/%d/%Y")

 else ifeq ($(PLATFORM), Darwin) # MacOS

-SHELL=/usr/local/bin/bash
+SHELL=/bin/bash
 TIME ?= time
 YESTERDAY = $(shell date -v-1d +"%m/%d/%Y")
tcgoetz commented 1 year ago

If you're on MacOS uname should return Darwin and you should be in the

else ifeq ($(PLATFORM), Darwin) # MacOS

section. There should be no need to change anything in the Linux section. For me

orion:GarminDB_develop tgoetz$ which bash
/usr/local/bin/bash

What MacOs are you on? I'm on 13.1.

I think the better solution is to change this line:

--- a/defines.mk
+++ b/defines.mk
@@ -16,7 +16,7 @@ YESTERDAY = $(shell date --date yesterday +"%m/%d/%Y")

 else ifeq ($(PLATFORM), Darwin) # MacOS

-SHELL=/usr/local/bin/bash
+SHELL?=/usr/local/bin/bash
 TIME ?= time
 YESTERDAY = $(shell date -v-1d +"%m/%d/%Y")

and then you can put a value that matches your system in my-defines.mk.

saiwing-yeung commented 1 year ago

Interesting... I am on 12.6.2 (21G320). For me, almost everything on /usr/local/bin/ is from HomeBrew. From this, I think all stock shells should be on /bin.