LedgerHQ / nanos-secure-sdk

Secure (ST31) SDK for Ledger Nano S
Apache License 2.0
98 stars 53 forks source link

`Makefile` biz logic not compatible with OSX due to `find` tool differences #36

Closed simonhf closed 1 year ago

simonhf commented 3 years ago

When trying to build an app for the Ledger Nano S on OSX I found a build / Makefile issue and managed to track it down to the way the command line tool find works differently on OSX than on Linux.

On OSX this Makefile line [1] assigning VPATH ends up with duplicated folders which are spelt slightly differently. This causes make to run differently and not create the .d files and ultimately the make fails because of this.

The problem is introduced on this Makefile line [2] which relies on find.

Example of the problem manifesting itself with simple command line find examples:

On Linux then an example find looks like this, which is also what is expected:

$ echo find on Linux:
$ find ./src/ ./src/common/ -name '*.[csS]' | sort
./src/account.c
./src/addr.c
./src/apdu_handler.c
./src/common/actions.c
./src/common/actions.c
./src/common/app_main.c
./src/common/app_main.c
./src/common/main.c
./src/common/main.c
./src/common/tx.c
./src/common/tx.c
./src/crypto.c
./src/json/json_parser.c
./src/parser.c
./src/parser_impl.c
./src/rlp.c

On OSX then an example find looks like this, which is unexpected, and also introduces the issue that identical paths are spelt differently, e.g. : ./src//common/ and ./src/common// which ultimately causes make to fail (see above).

$ echo find on OSX:
$ find ./src/ ./src/common/ -name '*.[csS]' | sort
./src//account.c
./src//addr.c
./src//apdu_handler.c
./src//common/actions.c
./src//common/app_main.c
./src//common/main.c
./src//common/tx.c
./src//crypto.c
./src//json/json_parser.c
./src//parser.c
./src//parser_impl.c
./src//rlp.c
./src/common//actions.c
./src/common//app_main.c
./src/common//main.c
./src/common//tx.c

Why does this happen with find on OSX? According to this [3] it's just how things are with find on OSX :-(

However, a fix / workaround is to strip out any // e.g. as follows:

$ echo find on OSX with fix:
$ find ./src/ ./src/common/ -name '*.[csS]' | perl -lane 's~//~/~gs; print;' | sort
./src/account.c
./src/addr.c
./src/apdu_handler.c
./src/common/actions.c
./src/common/actions.c
./src/common/app_main.c
./src/common/app_main.c
./src/common/main.c
./src/common/main.c
./src/common/tx.c
./src/common/tx.c
./src/crypto.c
./src/json/json_parser.c
./src/parser.c
./src/parser_impl.c
./src/rlp.c

The fix in Makefile.rules file which worked for me is:

$ git diff
diff --git a/Makefile.rules b/Makefile.rules
...
-SOURCE_PATH   += $(BOLOS_SDK)/src $(foreach libdir, $(SDK_SOURCE_PATH), $(dir $(shell find $(BOLOS_SDK)/$(libdir) -name '*.[csS]'))) $(dir $(shell find $(APP_SOURCE_PATH) -name '*.[csS]'))
-SOURCE_FILES  += $(shell find $(SOURCE_PATH) -name '*.[csS]') $(GLYPH_DESTC) $(APP_SOURCE_FILES)
-INCLUDES_PATH += $(dir $(foreach libdir, $(SDK_SOURCE_PATH), $(dir $(shell find $(BOLOS_SDK)/$(libdir) -name '*.h')))) include $(BOLOS_SDK)/include $(BOLOS_SDK)/include/arm $(dir $(shell find $(APP_SOURCE_PATH) -name '*.h'))
+SOURCE_PATH   += $(sort $(BOLOS_SDK)/src $(foreach libdir, $(SDK_SOURCE_PATH), $(dir $(shell find $(BOLOS_SDK)/$(libdir) -name '*.[csS]'))) $(dir $(shell find $(APP_SOURCE_PATH) -name '*.[csS]')))
+SOURCE_FILES  += $(shell find $(SOURCE_PATH) -name '*.[csS]' | perl -lane 's~//~/~gs; print;') $(GLYPH_DESTC) $(APP_SOURCE_FILES)
+INCLUDES_PATH += $(sort $(dir $(foreach libdir, $(SDK_SOURCE_PATH), $(dir $(shell find $(BOLOS_SDK)/$(libdir) -name '*.h')))) include $(BOLOS_SDK)/include $(BOLOS_SDK)/include/arm $(dir $(shell find $(APP_SOURCE_PATH) -name '*.h')))

-VPATH += $(dir $(SOURCE_FILES))
+VPATH += $(sort $(dir $(SOURCE_FILES)))

Note: I also sprinkled added sort around too in order to remove unnecessary duplicates [4] which make debugging harder. Unfortunately sort is unable to remove "duplicates" like this: ./src//common/ and ./src/common//.

If you'd like then I can put together a PR if necessary.

Looking forward to comments and feedback :-)

[1] https://github.com/LedgerHQ/nanos-secure-sdk/blob/master/Makefile.rules#L34 [2] https://github.com/LedgerHQ/nanos-secure-sdk/blob/master/Makefile.rules#L31 [3] https://discussions.apple.com/thread/3565773 [4] https://stackoverflow.com/questions/16144115/makefile-remove-duplicate-words-without-sorting

simonhf commented 3 years ago

Note: Some of the $(sort ...) additions have already been made to the Nano X SDK Makefile.rules [1] but not the Nano S SDK equivalent [2].

[1] https://github.com/LedgerHQ/nanox-secure-sdk/blob/master/Makefile.rules [2] https://github.com/LedgerHQ/nanos-secure-sdk/blob/master/Makefile.rules

fbeutin-ledger commented 1 year ago

Hello @simonhf , sorry for the very late answer, I think that this issue is not relevant anymore with the new app builder docker that is OXS compatible. I'll close the issue, feel free to reach out if you are not satisfied with the answer