build(make): ensure SHELL includes PATH in its environment

[henrynash]

overview

make on macOS does not seem to honor updates to PATH within the Makefile for shell invocation. This can be resolved be setting the PATH env variable as part of the SHELL command. This change is benign for GNU make.

fix #4808

changelog

All Makefiles have been updated to modify the existing line that set the SHELL env variable to include the PATH.

review requests

I have tested this using standard make on macOS (10.15) and GNU make on Ubuntu linux. Some tests on other configurations would provide confidence that this works across the supported environments.

[codecov]

Codecov Report

Merging #4809 into edge will increase coverage by 9.67%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge    #4809      +/-   ##
==========================================
+ Coverage   57.91%   67.59%   +9.67%     
==========================================
  Files         956     1053      +97     
  Lines       26235    35451    +9216     
==========================================
+ Hits        15194    23963    +8769     
- Misses      11041    11488     +447

Impacted Files	Coverage Δ
opentrons/hardware_control/modules/types.py	87.5% <0%> (-12.5%)	⬇️
...p-generation/getNextRobotStateAndWarnings/index.js	62.5% <0%> (-6.25%)	⬇️
shared-data/js/getLabware.js	30% <0%> (-3.34%)	⬇️
protocol-designer/src/ui/modules/selectors.js	38.7% <0%> (-2.96%)	⬇️
protocol-designer/src/ui/steps/actions/actions.js	22.5% <0%> (-2.5%)	⬇️
...plist/formLevel/stepFormToArgs/magnetFormToArgs.js	14.28% <0%> (-2.39%)	⬇️
...tocol-designer/src/file-data/selectors/commands.js	51.47% <0%> (-1.56%)	⬇️
...ner/src/steplist/formLevel/stepFormToArgs/index.js	9.09% <0%> (-0.91%)	⬇️
opentrons/main.py	34.61% <0%> (-0.63%)	⬇️
opentrons/hardware_control/modules/tempdeck.py	80.31% <0%> (-0.61%)	⬇️
... and 146 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c93d83a...2956c76. Read the comment docs.

[mcous]

Thanks for this! We're gonna smoke test this on Windows make (pretty sure SHELL is ignored in Windows IIRC) and then merge

[mcous]

Update: my initial Windows smoke test (Win 10, PowerShell) resulted in failure:

C:\Users\mc\opentrons [henrynash/fix-makefiles]> make install
"C:\ProgramData\chocolatey\lib\make\tools\bin\make.exe": Interrupt/Exception caught (code = 0xc0000005, addr = 0x633b646d) 

Going to run this to ground tomorrow and see if I can offer any actionable feedback

[mcous]

@henrynash I don't have time to run this to ground further unfortunately, but some cursory doc reading + smoke testing has lead me to the MAKESHELL variable.

Seeing as SHELL apparently isn't an option here because it breaks on Windows, is there any chance you could run MAKESHELL as a solution to ground?

[henrynash]

No problem - I’ll investigate further. As an aside, what was your Windows 10 dev setup up? We’re you using the windows Linux subsystem?

[mcous]

@henrynash our Windows setup is PowerShell (I guess cmd.exe might work?) with the various GNU tools (make, curl, etc.) installed via chocolatey or scoop

I think we've got some internal Windows users using WSL but for now it's not our recommended Windows setup

[henrynash]

@mcous Thanks. Have got a few ideas that seem to work in isolation, just trying to test them out across all platforms. For Windows 10 Powershell - what is the typical environment? I assume people do install bash? Using scoop (even if it is from someones random bucket)? What about the version of make that is installed? I am currently getting a 193 error on the standard makefiles (without any changes) with make 3.8.1 - but they seem to work fine with make 4.2.1. Just trying to work out if I have not set things up properly.

[mcous]

@henrynash I'm soory, I didn't mean to send you down a Windows testing rabbit hole (we can test Windows on our end). I was mostly curious if MAKESHELL would behave as expected in *nix.

In the absence of a full Windows dev setup guide (it's on the todo list), my personal Windows setup is:

1. Install chocolatey
2. Install git, make, curl, Python v3.6, Node v12 via chocolatey
3. make install

(I've tested out the same general process on scoop and it seems to work but I don't have the setup steps in my memory)

On my machine, it's make 3.81, but it looks like Chocolatey has 4.2.1. I have no problem recommending / requiring latest make on Windows. Also our goal with Windows setup is to avoid any bash installation if at all possible

[henrynash]

@mcous No problem. Managed to do some basic Windows testing. Unfortunately .ONESHELL did not seem to do the trick under macOS. What does seem to work (at least from my initial testing) is setting the SHELL line to:

SHELL := /bin/bash -c

I think this prevents make on macOS from optimising away the PATH setting, and appears to not upset make on Windows. Will test out Linux later today. If it all hangs together, I'll post an updated PR.

(As an aside, looks like the basic install of make with both chocolatey and scoop is broken right now on Windows, since the 4.2.1 sourceforge zip is no longer available...since it has moved on to 4.3, and scoop/chocolatey have not yet caught up)

[mcous]

Very strange, but on my macOS machine, SHELL := /bin/bash doesn't work, but SHELL := bash does

[henrynash]

@mcous Hmm, odd. So I have updated the PR to use the "-c" flag in the SHELL command. For me this seems to work on macOS, Linux and Windows 10. But concerned that there seems to be an issue for you ?!

[mcous]

Sorry for the confusion @henrynash; what I meant by...

Very strange, but on my macOS machine, SHELL := /bin/bash doesn't work, but SHELL := bash does

...was that setting SHELL := bash on my mac without using -c works with the correct PATH.

# this works for me on macOS with built-in `make`
PATH := # ...
SHELL := bash

# ...

I'm also a little concerned about tacking -c to SHELL because, according to the GNU make docs:

The argument(s) passed to the shell are taken from the variable .SHELLFLAGS. The default value of .SHELLFLAGS is -c normally, or -ec in POSIX-conforming mode.

Setting SHELL to -c seems to me to be making the shell call /usr/bin/bash -c -c "...", which worries me (even though it appears to work)

[henrynash]

@mcous Ah, I get it, sorry! I kind of share your concern over "-c"....it is kind of wrong (although works!). I confirm that

SHELL := bash

also works on my machine. Even more weirdly, if you add a trailing space to /bin/bash, i.e.:

SHELL := /bin/bash

...it also works!! (But I like this less since it isn't obvious reading the file the difference between that and how it used to be)

I'll run some tests on Linux & windows on SHELL := bash to see how that pans out across them.

[henrynash]

Using bash instead of /bin/bash seems to work across all the platforms from my initial testing, so have updated the PR accordingly. Thanks to @mcous for this idea.

[mcous]

This is working on my macOS machine and my Windows machine. Could someone from @Opentrons/py give this a once over, too?