I discovered this problem (in the process of several hours of debugging) when trying out scorpio following #592

I could obviously make /bin/sh point to bash on my own system but if anyone else tries to build MPAS on Ubuntu systems, they will run into this issue and, like me earlier today, won't know why.

I'd strongly recommend we be clear about the shell used-- even mac may leave bash. There are other modern alternatives and some folks run tcsh too. Being really clear about the shell used is a good thing.

There are other modern alternatives and some folks run tcsh too.

Just to clarify, make uses /bin/sh by default, so this isn't about what shell a user is running, just about what the system links /bin/sh to. On grizzly, it's bash but that's not a guarantee on other systems.

@mgduda, @mark-petersen and @matthewhoffman, if you're okay with this change, could you do a quick build and test with this Makefile when you have a chance? It should be easy to test.

If dash is POSIX compliant, then the failure in the pio_test target would suggest that there is something in the commands for that target that are not POSIX-compliant shell commands. I'd propose we fix the shell commands, rather than force the use of /bin/bash.

@xylar I don't have access to a system running Ubuntu. Could you provide the failure message, if any, that you're seeing? I'll also check whether I have access to any systems with dash.

@mgduda, it basically isn't getting the exit code from the failed compile command. It thinks PIO1 succeeded even when it fails.

$ make gfortran CORE=ocean USE_PIO2=true
Checking for a usable PIO library...
=> PIO 1 detected
x86_64-conda_cos6-linux-gnu-gfortran.bin: error: pio1.f90: No such file or directory
x86_64-conda_cos6-linux-gnu-gfortran.bin: error: pio2.f90: No such file or directory

"PIO1 detected" is definitely incorrect and if I actually run the command (in bash), it produces various error messages and exits 1

If I switch to bash, I get:

Checking for a usable PIO library...
=> PIO 2 detected

and everything builds as expected.

If you want to go the route of fixing the test_pio, I think you'll need to figure out a way to test that yourself. I don't have the expertise needed (I do as little shell scripting as I can get away with) and I feel I've invested as much time into trying to debug it as I have for now. I much prefer to switch to bash and be done.

It turned out to take just a few minutes to install dash. I think the issue is that dash is adding \ characters in strings; for example:

$ echo "FC = mpif90" 
FC \= mpif90

This creates problems when we try to pass variables to make, e.g. here in the top-level Makefile. The result is that FC is never correctly set, breaking the compile commands in the pio_test target.

@mgduda, that sounds like progress. Thanks for investigating! Does that still suggest that the easiest solution is bash or are you determined to make things work with dash somehow?

@xylar I'm still experimenting a bit. Sometimes, this can lead to the realization that we're doing something fundamentally "odd", and that a different approach would solve the problem and put our code more in line with best practices. For example, maybe invoking make recursively like

gfortran:                                                                                                                            
        ( $(MAKE) all \                                                                                                              
        "FC_PARALLEL = mpif90" \                                                                                                     
        "CC_PARALLEL = mpicc" \                                                                                                      
        "CXX_PARALLEL = mpicxx" \                                                                                                    
        "FC_SERIAL = gfortran" \                                                                                                     
        "CC_SERIAL = gcc" \                                                                                                          
        "CXX_SERIAL = g++" \                                                                                                         
        "FFLAGS_PROMOTION = -fdefault-real-8 -fdefault-double-8" \                                                                   
        "FFLAGS_OPT = -O3 -m64 -ffree-line-length-none -fconvert=big-endian -ffree-form" \                                           
        "CFLAGS_OPT = -O3 -m64" \                                                                                                    
        "CXXFLAGS_OPT = -O3 -m64" \                                                                                                  
        "LDFLAGS_OPT = -O3 -m64" \                                                                                                   
        "FFLAGS_DEBUG = -g -m64 -ffree-line-length-none -fconvert=big-endian -ffree-form -fbounds-check -fbacktrace -ffpe-trap=invalid,zero,overflow" \
        "CFLAGS_DEBUG = -g -m64" \                                                                                                   
        "CXXFLAGS_DEBUG = -O3 -m64" \                                                                                                
        "LDFLAGS_DEBUG = -g -m64" \                                                                                                  
        "FFLAGS_OMP = -fopenmp" \                                                                                                    
        "CFLAGS_OMP = -fopenmp" \                                                                                                    
        "CORE = $(CORE)" \                                                                                                           
        "DEBUG = $(DEBUG)" \                                                                                                         
        "USE_PAPI = $(USE_PAPI)" \                                                                                                   
        "OPENMP = $(OPENMP)" \                                                                                                       
        "CPPFLAGS = $(MODEL_FORMULATION) -D_MPI" ) 

isn't a good paradigm. On the other hand, it could be that what we're doing is reasonable, and it's simply dash that is problematic.

Simply setting SHELL = /bin/bash in the top-level Makefile certainly would be easy, but I'm not yet convinced it's the best option. What happens if a Linux distribution decides to not provide bash, but to only provide zsh (if I recall correctly, Ubuntu doesn't provide tcsh by default, so there is precedent for distributions not offering widely used shells); or, what if bash is only provided as /usr/bin/bash? Essentially, just setting SHELL = /bin/bash seems like a workaround for the shortcoming of a distribution.

For what it's worth, I think the following should also be sufficient:

make gfortran SHELL=/bin/bash CORE=ocean <other options>

Here's a C program to help illustrate how dash (in my opinion) is fundamentally broken when it comes to handling of quoted strings:

#include <stdio.h>

int main(int argc, char **argv)
{
	int i;

	for (i=1; i<argc; i++) {
		printf("ARG: %s\n", argv[i]);
	}

	return 0;
}

With, e.g., bash, it's possible to provide a string as a single command-line argument:

bash-3.2$ ./a.out "FC=mpif90 -O3"
ARG: FC=mpif90 -O3

With dash, this is what we get:

$ ./a.out "FC=mpif90 -O3"
ARG: FC\=mpif90 \-O3

Here's an example Makefile that, when using dash as the shell, illustrates the problem that dash causes for us in MPAS:

all:
	($(MAKE) src FCFLAGS="-O3 -fdefault-real-8")

src:
	@echo $(FCFLAGS)

For what it's worth, I think the following should also be sufficient:
make gfortran SHELL=/bin/bash CORE=ocean <other options>

Oh, I'm fine with that solution. It didn't occur to me that the solution would be that simple.

Hmm, so that raises a question for me. Typically, I define export CORE=ocean rather than passing CORE=ocean to the make command. But $SHELL is always already defined and obviously make isn't picking it up. Why is that? A special case?

@xylar My uninformed guess would be that SHELL is a special case.

In the interest of placing all ideas on the table for consideration, another option might be to add

ifneq "${MPAS_SHELL}" ""
        SHELL = ${MPAS_SHELL}
endif

at the top of the Makefile. Setting the environment variable MPAS_SHELL would then override the default shell used by make.

@mgduda, I like that suggestion very much. I updated the PR with that change.

Tested with Ubuntu, export MPAS_SHELL=/bin/bash and the SCORPIO variant of PIO2, and everything compiles as expected.

@MiCurry, @matthewhoffman and @mark-petersen, could you try this out and review when you get a chance? The priority is to make sure nothing breaks. Secondarily, you could set MPAS_SHELL if you like.

@xylar I'll give it a test.

I'd strongly recommend we be clear about the shell used-- even mac may leave bash. There are other modern alternatives and some folks run tcsh too. Being really clear about the shell used is a good thing.

In fact, Mac OS has already switched its default shell from bash to zsh with the release of Catalina. As a zsh user, I'm not complaining! A lot of people here at

@MiCurry, thank you for looking into that. I hadn't run into any problems deeper in but that may simply be because there aren't currently shell calls in deeper Makefiles in the ocean core.

@mgduda, are you okay with passing the $SHELL variable to deeper make files as @MiCurry suggests? I will go ahead and make the change, but we can remove the commit later if you aren't happy with it.

@matthewhoffman I think you can approve based on my testing, unless you want to test a different system.

@mgduda any further comments?

@xylar @MiCurry Rather than having to add "SHELL = $(SHELL)" in the dozens of places where we invoke sub-make processes, an alternative might just be to add

ifneq "${MPAS_SHELL}" ""                                                                                                             
        SHELL = ${MPAS_SHELL}                                                                                                        
endif

once at the top of each Makefile.

I'm also fine with just modifying the top-level Makefile for now, since that's the only place where we apparently use shell commands that fail with dash. On an as-needed basis, we can update other Makefiles in future.

@mgduda, I think we would need to pass $MPAS_SHELL in all of the same calls instead of $SHELL, so it would make things worse rather than better. At least, we're passing a lot of other env. variables that way that give me that sense.

So for now I'm just removing my last commit and we'll only modify the base Makefile unless and until we have future problems. Hopefully, we'll move to cmake soonish and I'll never notice again...

@mgduda and @matthewhoffman, if you can review this soon, I think @mark-petersen would like to merge develop to CORE/develop for at least ocean and maybe others by early next week.

@xylar I'm fine with just making changes in the top-level Makefile in this PR.

To clarify for future reference, we would not need to pass MPAS_SHELL in the same way as we proposed to pass SHELL in every sub-make invocation. The logic

ifneq "${MPAS_SHELL}" ""                                                                                                             
        SHELL = ${MPAS_SHELL}                                                                                                        
endif

sets the SHELL make variable based on the MPAS_SHELL environment variable, so the user's environment is queried on every invocation of a sub-make process. To convince yourself that it's not necessary to pass MPAS_SHELL with the above logic, you can create a test set-up with a top-level Makefile that contains:

ifneq "${MPAS_SHELL}" ""
        SHELL = ${MPAS_SHELL}
endif

all:
	@echo "In top-level directory: " $(SHELL)
	$(MAKE) -C src

Then, in a src sub-directory, create a Makefile that contains:

ifneq "${MPAS_SHELL}" ""
	SHELL = ${MPAS_SHELL}
endif

all:
	@echo "In src directory: " $(SHELL)

@mgduda, thanks for the explanation. It does sound like altering all the Makefiles would be plausible, then. Nevertheless, let's let it be for expediency right now.

I will update the description/commit message in just a moment as you requested. Thanks for those suggestions, they certainly will make things clearer.