Correct reference to python in Makefile.common

[donnaaboise]

It seems that the environment variable CLAW_PYTHON (or PYTHON) is not consistently referenced from Makefile.common.

[rjleveque]

Does anyone remember why there is both a PYTHON and CLAW_PYTHON in this Makefile?

Elsewhere in the Makefile it is CLAW_PYTHON that is used, so is there a reason this new PR uses PYTHON?

It might be best to settle on one and remove the other. The advantage of CLAW_PYTHON is that if you set it in a general .bashrc file, for example, it will be clear that this environment variable is specific to Clawpack, unless it is standard practice to set a PYTHON variable that is used in other contexts as well?

[donnaaboise]

I am looking again at Makefile.common, and now wonder how this code is supposed to work :

PYTHON ?= python
CLAW_PYTHON ?= $(PYTHON)

If I set CLAW_PYTHON=python3 (following comments in Makefile.common) in an example Makefile, I still run into an error importing argparse (from check_src.py.

[donnaaboise]

@rjleveque I wondered too why both were being used. Probably makes more sense to use CLAW_PYTHON. I was more concerned about the logic. But then I am not sure what ?= means.

[rjleveque]

In general ?= means set it to the specified value only if it is not already set to something as an environment variable.

Setting PYTHON=python3 seems to work ok for me, not sure what's going wrong for you. Did you try make -d .exe so it prints debug info?

[donnaaboise]

Setting PYTHON=python3 seems to work. (but then maybe the comment should be changed ...)

I'll update the PR.

[donnaaboise]

I also set the MODULE_FLAG to -I for other compilers. The PGI compiler, for example, does not like -J.

pgfortran-Error-Unknown switch: -J/home/donnacalhoun/clawpack5.5/clawpack/classic/src
make: *** [/home/donnacalhoun/clawpack5.5/clawpack/classic/src/utility_module.mod] Error 1

[mandli]

I think this was a work around when someone may have had two different pythons around. I think it's more confusing than anything.

For MODULE_FLAGS I think it is a bit more common to use the gcc style flagging (or at least that it is more commonly supported). The best thing to do would be to create a specific PGI compiler if clause. I also don't think -I should be the default given that it has another use for all the compilers.

[donnaaboise]

I have always just used -I for modules, but maybe this issue is best left for another PR. I"ll update the PR and revert back to -J.

But is the CLAW_PYTHON okay? I did find it handy. I am running on a system where i don't have root privileges, and the admin set up Python 3.x as python3.

[mandli]

The -J also tells the compiler where to place modules. I have found it helps a lot when there are issues with module conflicts and versioning.

[donnaaboise]

Sounds good. I have very little experience with the PGI compilers, so maybe the person to ask about this is @xinshengqin who has re-worked the Makefile.common extensively for CudaClaw.

[mandli]

I might not be a bad idea to add a warning in that last clause though.

[rjleveque]

So should we replace the lines:

PYTHON ?= python
CLAW_PYTHON ?= $(PYTHON)

by simply

CLAW_PYTHON ?= python

since the PYTHON variable apparently isn't used elsewhere?

[donnaaboise]

Makes sense to me, especially since it probably isn't a good idea to set PYTHON as an environment variable.

[mandli]

I concur.

[donnaaboise]

I went ahead and added a branch for the PGI compiler, basically the two lines:

else ifeq ($(CLAW_FC), pgfortran)
    MODULE_FLAG = -module
    OMP_FLAG = -openmp

I haven't verified the OMP_FLAG, though ...

[mandli]

From https://www.pgroup.com/resources/docs/17.10/x86/pgi-ref-guide/index.htm it looks like -mp is the equivalent.

[donnaaboise]

Sounds right. @xinshengqin - can you confirm this?

[xinshengqin]

Sorry for the late response. I am in Hong Kong time zone. @donnaaboise Yes, the openmp flag for PGI compiler is -mp

[xinshengqin]

@donnaaboise

I just checked the changes. The current code probably won't read the pgfortran clause since the clause at line 98, ifeq ($(findstring gfortran,$(CLAW_FC)),gfortran) will be selected even if CLAW_FC is set to pgfortran. In my Makefile, I have this line replaced with ifeq ($(CLAW_FC),gfortran).

Also, at line 106, else ifeq ($(CLAW_FC), pgfortran), there is a space before pgfortran, which I think should be removed. Make treats the entire thing (including any space) after the comma as the string to be compared with.

Another (possibly better) alternative is to use ifeq ($(findstring pgfortran,$(CLAW_FC)),pgfortran) first to check pgi compiler, followed by else ifeq ($(findstring gfortran,$(CLAW_FC)),gfortran) and any other compiler. In this way, weird things won't happen when the user do things like CLAW_FC = pgfortran (some spaces before pgfortran) instead of CLAW_FC =pgfortran.

[donnaaboise]

Thanks, @xinshengqin. I fixed the first two issues you pointed out. I wasn't sure about your last suggestion, because it seems very specific to pgfortran. What is someone sets `CLAW_FC =gfortran' ? Would the same space problem show up?

[xinshengqin]

My point is ifeq ($(findstring pgfortran,$(CLAW_FC)),pgfortran) checks if CLAW_FC contains any sub-string that matches pgfortran. This can handle cases where user accidentally set CLAW_FC to ' pgfortran ' (arbitrary number of space before and after pgfortran). It strips out all spaces before and after. I think this idea should be used in general for many other cases where you need to check a user input.

[xinshengqin]

The original code uses ifeq ($(findstring gfortran,$(CLAW_FC)),gfortran) for the same reason (strip out space in the front and back of CLAW_FC) perhaps.

[rjleveque]

Looking at https://github.com/clawpack/geoclaw/pull/538, I realized that this PR was never merged.

@mandli and @donnaaboise: I think it's still fine to merge?