Change delimiter in git submodule cut to double quote

[nitz]

The single quote causes issues with the string parsing on shells that expect double quotes.

In particular, I'm building on windows, and my cut/paste commands are standard win32 binaries. The shell invocation from make causes the quote to make cut very grumpy, and it while building is still possible, the version defines don't make it to the CC commands nicely.

I think this qualifies as my requisite first world problem of the week. 😂

[hathach]

look good to me, I tried it on my linux pc, doesn't make any differences, though I wonder what is your submodule output on windows with $(error $(GIT_SUBMODULE_VERSIONS))

[nitz]

I'm glad it works on your Linux machine too. I tested in in my WSL2 Ubuntu install, but I was worried it might make a difference in a full *nix. I googled around a bit because I couldn't remember ever having issues I'd solved with single vs double quotes (except in PHP...)!

So, here's the output of the error expansion: It tries to treat the single quote as the character, haha:

❯ make BOARD=mdk_nrf52840_dongle
cut: ': No such file or directory
Makefile:30: *** .  Stop.

I can't seem to track down exactly where exactly along the lines the issue comes from. My build environment is definitely non-standard: On Windows, I'm using powershell 7 (or command prompt occasionally) via windows terminal, with make 4.3, I have a whole slew of gnu tools compiled for windows on my path (via UnxUtils) because that was easier than trying to unlearn my muscle memory and lighter than a full cygwin/msys2 setup. I got into problems first because it actually has a sh.exe and zsh.exe binary that $(shell) was trying to use first. Makes sense for make, but not for me -- so I got those off my path then just had the issue with cut there.

Best I can tell it's somewhere in how make on windows launches the shell commands. Windows/cut doesn't seem to be upset handling the single quotes from the terminal, but I'm guessing it's probably some issue with ShellExecute/CreateProcess or whatever make on windows is using to actually spawn the shell commands.

Edited to move other suggestion to next comment.

[nitz]

While I'm in here, I'm curious though. Maybe we could change the shell variable expansion to instead use the shell assignment make added in 4.0, the != operator? That way those shell evaluations just get executed once instead of for each CC, which at least on my machine increases a clean build speed considerably (the majority of the time I spend waiting in build is in git submodule status, the actual compilation is considerable faster):

Something like this?

+++ "b/Makefile"
@@ -22,8 +22,8 @@ MBR_HEX            = lib/softdevice/mbr/hex/mbr_nrf52_2.4.1_mbr.hex
 # linker by MCU eg. nrf52840.ld
 LD_FILE      = linker/$(MCU_SUB_VARIANT).ld

-GIT_VERSION = $(shell git describe --dirty --always --tags)
-GIT_SUBMODULE_VERSIONS = $(shell git submodule status | cut -d' ' -f3,4 | paste -s -d" " -)
+GIT_VERSION != git describe --dirty --always --tags
+GIT_SUBMODULE_VERSIONS != git submodule status | cut -d" " -f3,4 | paste -s -d" " -

 # compiled file name
 OUT_FILE = $(BOARD)_bootloader-$(GIT_VERSION)

Not that it takes too terribly long to compile from fresh, (~40 seconds with my particular fork) but it reduces that time to ~14 seconds without calling git twice for each CC. In fact, running make with -n to actually skip the compiles, it takes ~27 seconds with the current way, and ~700 ms with the !=.

I could open that in a separate PR if you think it makes sense!

[hathach]

While I'm in here, I'm curious though. Maybe we could change the shell variable expansion to instead use the shell assignment make added in 4.0, the != operator? That way those shell evaluations just get executed once instead of for each CC, which at least on my machine increases a clean build speed considerably (the majority of the time I spend waiting in build is in git submodule status, the actual compilation is considerable faster):

Yeah sure, it should really be := , but != is better, you don't have to open an separated PR for this, just pushed the update.

[nitz]

Done and done. Stuck my nose into another PR's CI build just to compare times. Looks like an improvement for sure: 🙂

[nitz]

Thanks for letting me help out! (Even if it started a bit selfishly!)

[hathach]

Done and done. Stuck my nose into another PR's CI build just to compare times. Looks like an improvement for sure:

Indeed, it shorten at least 2x time to build. I always wondered why it takes too long to build bootloader with this size as well. Thanks a lot.

[hathach]

Thanks for letting me help out! (Even if it started a bit selfishly!)

We are the one to say thank. As a old wise man said "Opensource PR is all about selfish need to fix personal problem", but it happens to fix :100: other people problem as well