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

Critical flaw in Macro generation: does not preserve order #3446

Closed
jgfouca opened this issue Mar 12, 2020 · 1 comment · Fixed by #4088
Closed

Critical flaw in Macro generation: does not preserve order #3446

jgfouca opened this issue Mar 12, 2020 · 1 comment · Fixed by #4088

Comments

@jgfouca
Copy link
Contributor

jgfouca commented Mar 12, 2020

Some background: I was debugging a problem on summit and I discovered that I needed to have the pnetcdf link come earlier in the link line, so I changed the config_compilers.xml like this:

FROM:

   <SLIBS>
    <append>-lxlopt -lxl -lxlsmp -L$ENV{NETCDF_C_PATH}/lib -lnetcdf -L$ENV{NETCDF_FORTRAN_PATH}/lib -lnetcdff -L$ENV{ESSL_PATH}/lib64 -lessl -L$ENV{NETLIB_LAPACK_PATH}/lib -llapack</append>
     <append MPILIB="!mpi-serial"> -L$ENV{PNETCDF_PATH}/lib -lpnetcdf -L$ENV{HDF5_PATH}/lib -lhdf5_hl -lhdf5 </append>
   </SLIBS>

TO:

   <SLIBS>
     <append MPILIB="!mpi-serial"> -L$ENV{PNETCDF_PATH}/lib -lpnetcdf -L$ENV{HDF5_PATH}/lib -lhdf5_hl -lhdf5 </append>
    <append>-lxlopt -lxl -lxlsmp -L$ENV{NETCDF_C_PATH}/lib -lnetcdf -L$ENV{NETCDF_FORTRAN_PATH}/lib -lnetcdff -L$ENV{ESSL_PATH}/lib64 -lessl -L$ENV{NETLIB_LAPACK_PATH}/lib -llapack</append>
   </SLIBS>

In other words, just moved the pnetcdf SLIBS append earlier. To my surprise, this had no impact on the link flag order generated in Macros.cmake. I checked out BuildTools/macroconditiontree.py and discovered that it does not appear to be order-preserving. Things are being sorted, stored in unordered dicts, and written without regard to the original order.

Since compilation and link flags can be order-sensitive, this strikes me as a fatal flaw... I think the entire BuildTools library may need to be rewritten.

@jgfouca
Copy link
Contributor Author

jgfouca commented Mar 12, 2020

Just FYI, i was able to hack my way around this particular problem by doing this:

   <SLIBS>
     <append MPILIB="!mpi-serial"> -L$ENV{PNETCDF_PATH}/lib -lpnetcdf -L$ENV{HDF5_PATH}/lib -lhdf5_hl -lhdf5 -lxlopt -lxl -lxlsmp -L$ENV{NETCDF_C_PATH}/lib -lnetcdf -L$ENV{NETCDF_FORTRAN_PATH}/lib -lnetcdff -L$ENV{ESSL_PATH}/lib64 -lessl -L$ENV{NETLIB_LAPACK_PATH}/lib -llapack</append>
    <append MPILIB="mpi-serial">-lxlopt -lxl -lxlsmp -L$ENV{NETCDF_C_PATH}/lib -lnetcdf -L$ENV{NETCDF_FORTRAN_PATH}/lib -lnetcdff -L$ENV{ESSL_PATH}/lib64 -lessl -L$ENV{NETLIB_LAPACK_PATH}/lib -llapack</append>
   </SLIBS>

@rljacob rljacob removed the CRITICAL label Mar 18, 2020
jgfouca added a commit that referenced this issue Sep 10, 2021
Change E3SM to use cmake macro file system

This is a big change for E3SM but should have no impact on other models and did not require big changes to CIME code.

This PR lays the groundwork for E3SM leaving the config_compilers.xml/BuildTools.configure behind for good, replacing that system with a Cmake-based cache file system that can, if needed, be used to generate a Makefile macro when needed (for some sharedlibs).

I will lay out detail documentation for this change when I create the corresponding PR in E3SM that activates these new macros.

Test suite: scripts_regression_tests (both with and without new macros in the host repo)
Test baseline:
Test namelist changes:
Test status: [bit for bit, roundoff, climate changing]

Fixes #3287
Fixes #3446
Fixes #3341
Fixes #2965

User interface changes?:

Update gh-pages html (Y/N)?:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants