Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid type redefine of MPI_Comm and MPI_Info #2270

Merged
merged 2 commits into from
Apr 5, 2022
Merged

Conversation

wkliao
Copy link
Contributor

@wkliao wkliao commented Apr 2, 2022

MPI_Comm and MPI_Info should only be used when parallel I/O is enabled.
Compiling file libdispatch/dparallel.c is skipped when parallel I/O is disabled.

@edwardhartnett
Copy link
Contributor

Thanks, this is a good solution!

Copy link
Contributor

@edwardhartnett edwardhartnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever and simple! ;-)

@mathstuf
Copy link
Contributor

mathstuf commented Apr 4, 2022

Should netcdf_par.h be skipped from the install as well?

@wkliao
Copy link
Contributor Author

wkliao commented Apr 4, 2022

netcdf_par.h is installed only if parallel I/O is enabled.

if BUILD_PARALLEL
include_HEADERS += netcdf_par.h
endif

IF(ENABLE_PNETCDF OR ENABLE_PARALLEL4)
INSTALL(FILES ${netCDF_SOURCE_DIR}/include/netcdf_par.h
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
COMPONENT headers)
ENDIF()

@WardF WardF self-assigned this Apr 5, 2022
@WardF WardF added this to the 4.9.0 milestone Apr 5, 2022
Copy link
Member

@WardF WardF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic, thank you!

@edwardhartnett
Copy link
Contributor

Maybe we don't want to only build dparallel.c when parallel is enabled.

See Unidata/netcdf-fortran#344

@edwardhartnett
Copy link
Contributor

I have submitted a PR to undo these changes (#2300).

If these kind of changes are needed, they must be done in a way that does not break netcdf-fortran.

@mathstuf
Copy link
Contributor

Does this mean that #2187 is the way forward then?

@edwardhartnett
Copy link
Contributor

I really don't know. ;-)

However, whatever is done must be done in such a way that netcdf-fortran does not break. That could mean we restore these changes and then change netcdf-fortran so that it too does not include or use the parallel functions except for parallel builds.

Or perhaps some other solution, like #2187 is best.

But whatever it is, it needs to not break netcdf-fortran.

I am wondering if netcdf-c shouldn't build and test netcdf-fortran as part of the netcdf-c CI run. That is, treat netcdf-fortran as an extra set of tests for netcdf-c. That way, we would not make the mistake of making changes to netcdf-c which are fine by themselves, but break the fortran build.

@edwardhartnett
Copy link
Contributor

See #2301

@wkliao
Copy link
Contributor Author

wkliao commented Apr 25, 2022

I suggest to fix netcdf-fortran.
If netcdf-fortran detects the parallel feature is disabled in netcdf-c,
it should skip defining/compiling those parallel APIs.
Luckily, there are only 3 parallel APIs:

  • nf_create_par
  • nf_open_par
  • nf_var_par_accese

@edwardhartnett
Copy link
Contributor

@wkliao I don't object to that solution, but I suspect it will have to wait until after the next netcdf-c/netcdf-fortran release, unless it is done immediately...

@wkliao
Copy link
Contributor Author

wkliao commented Apr 27, 2022

It turns out the fix is simple. See #352

@WardF
Copy link
Member

WardF commented May 2, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants