search

wrf-model / WRF

The official repository for the Weather Research and Forecasting (WRF) model
Other
1.18k stars 658 forks source link

Make sure that USENETCDFPAR is not undefined #1988

Closed MicroTed closed 5 months ago

MicroTed commented 5 months ago

Some versions of make appear to fail on the USENETCDFPAR logic in the top level Makefile if the variable is undefined. Making sure it is at least set to 0 by the configure script fixes the issue.

TYPE: bug fix

KEYWORDS: make

SOURCE: Ted Mansell (NOAA/NSSL)

DESCRIPTION OF CHANGES: Problem: Logic failure in top level Makefile with some versions of make (don't remember which) if USENETCDFPAR is undefined

Solution: Making sure that at USENETCDFPAR is set to either 0 or 1 in the configure script fixes the issue.

LIST OF MODIFIED FILES: configure

TESTS: it passed regression tests.

islas commented 5 months ago

While this is technically no longer an issue since merging of #1743, I think setting the precedent to fixing the issue upstream and simply having the env var defined is good practice - especially when WRF is the one setting the variable to begin with.

islas commented 5 months ago

@MicroTed Can you reword the title the PR to be more descriptive of the issue?

For the description - I think it would be better to talk about the underlying issue which isn't so much about versions of make as it is about the nc4_test target previously using logic that assumed USENETCDFPAR had been set, leading to malformed syntax of if [ -eq 0 ]; then... in the recipe expansion.

MicroTed commented 5 months ago

@islas Ah, thank you, this was something I had encountered a while ago and didn't realize that the Makefile logic had been fixed. If this is therefore moot, it can just be closed.

islas commented 5 months ago

I do believe there is still merit in making sure the environment variables we set in configure are well defined, even if subsequent logic has additional error handling

MicroTed commented 5 months ago

OK, sounds good. And I think the original issue was actually my fault, when the parallel netcdf4 option was added 😄

weiwangncar commented 5 months ago

The regression test results:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None