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

DA closbuf fix #10

Merged
merged 6 commits into from
Jul 16, 2020
Merged

Conversation

mark-a-potts
Copy link
Contributor

The added guard tests to see if arrays have been allocated (which is done during openbf) before continuing with closbf. It is only used in the DYNAMIC_ALLOCATION builds. Addresses #9

src/list_of_files.cmake Outdated Show resolved Hide resolved
Copy link
Collaborator

@jbathegit jbathegit left a comment

Choose a reason for hiding this comment

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

I'll restate for the record here that an application code has no reason to ever be calling CLOSBF prior to OPENBF, and therefore (at least IMO) this is a shortcoming in the overall design of the GSI code, and something that in an ideal world should really be corrected there.

That said, I'm willing to add this trap into CLOSBF within the BUFRLIB baseline; however, I would ask that it be done using the library subroutine ERRWRT instead of a direct write statement to FT06. Subroutine ERRWRT is a general-purpose message logger used throughout the BUFRLIB and which allows a user to easily re-direct any library messages wherever they choose. The default version of ERRWRT that's distributed with the library writes to FT06, but users can, if they want, also supply their own version of ERRWRT within their application code to override the default version and thereby redirect error messages to wherever they want (e.g. a separate job log). So I would instead suggest the following:

@@ -55,9 +57,22 @@ C$$$

   INCLUDE 'bufrlib.prm'
  •  CHARACTER*128 ERRSTR
    

C-----------------------------------------------------------------------
C-----------------------------------------------------------------------

+#ifdef DYNAMIC_ALLOCATION

  •  IF ( .NOT. ALLOCATED(NULL) ) THEN
    
  •    CALL ERRWRT('++++++++++++++++++++WARNING++++++++++++++++++++++')
    
  •    ERRSTR = 'BUFRLIB: CLOSBF WAS CALLED WITHOUT HAVING ' //
    
  • .           'PREVIOUSLY CALLED OPENBF'
    
  •    CALL ERRWRT(ERRSTR)
    
  •    CALL ERRWRT('++++++++++++++++++++WARNING++++++++++++++++++++++')
    
  •    RETURN
    
  •  ENDIF
    

+#endif
+
CALL STATUS(LUNIT,LUN,IL,IM)

Thanks, and as a separate question re: list_of_files.cmake, I'm curious whether there's any way you could just have the build process automatically generate the list of source files on the fly (and in the correct order in which they need to be compiled, i.e. modvF, modaF, all other *F, all other *f as is done in the original makebufrlib.sh), rather than having to explicitly list every source file in list_of_files.cmake? I'm obviously not an expert on CMake or on all of the steps involved in this new build process, but I just wanted to put the question out there ;-)

@aerorahul
Copy link
Contributor

aerorahul commented Jul 16, 2020

I'll restate for the record here that an application code has no reason to ever be calling CLOSBF prior to OPENBF, and therefore (at least IMO) this is a shortcoming in the overall design of the GSI code, and something that in an ideal world should really be corrected there.

That said, I'm willing to add this trap into CLOSBF within the BUFRLIB baseline; however, I would ask that it be done using the library subroutine ERRWRT instead of a direct write statement to FT06. Subroutine ERRWRT is a general-purpose message logger used throughout the BUFRLIB and which allows a user to easily re-direct any library messages wherever they choose. The default version of ERRWRT that's distributed with the library writes to FT06, but users can, if they want, also supply their own version of ERRWRT within their application code to override the default version and thereby redirect error messages to wherever they want (e.g. a separate job log). So I would instead suggest the following:

@@ -55,9 +57,22 @@ C$$$

   INCLUDE 'bufrlib.prm'
  •  CHARACTER*128 ERRSTR
    

C-----------------------------------------------------------------------
C-----------------------------------------------------------------------

+#ifdef DYNAMIC_ALLOCATION

  •  IF ( .NOT. ALLOCATED(NULL) ) THEN
    
  •    CALL ERRWRT('++++++++++++++++++++WARNING++++++++++++++++++++++')
    
  •    ERRSTR = 'BUFRLIB: CLOSBF WAS CALLED WITHOUT HAVING ' //
    
  • .           'PREVIOUSLY CALLED OPENBF'
    
  •    CALL ERRWRT(ERRSTR)
    
  •    CALL ERRWRT('++++++++++++++++++++WARNING++++++++++++++++++++++')
    
  •    RETURN
    
  •  ENDIF
    

+#endif
+
CALL STATUS(LUNIT,LUN,IL,IM)

Thanks, and as a separate question re: list_of_files.cmake, I'm curious whether there's any way you could just have the build process automatically generate the list of source files on the fly (and in the correct order in which they need to be compiled, i.e. modv_F, moda_F, all other *F, all other *f as is done in the original makebufrlib.sh), rather than having to explicitly list every source file in list_of_files.cmake? I'm obviously not an expert on CMake or on all of the steps involved in this new build process, but I just wanted to put the question out there ;-)

@jbathegit I'll respond to the second question.

CMake has a GLOB feature, but globbing is not a good practice when it comes to CMake. Here is a snippet from a book on best practices:

CMake is a build system generator, not a build system. It evaluates the GLOB expression to a list of files when generating the build system. The build system then operates on this list of files. Therefore, the build system cannot detect that something changed in the file system.

CMake cannot just forward the GLOB expression to the build system, so that the expression is evaluated when building. CMake wants to be the common denominator of the supported build systems. Not all build systems support this, so CMake cannot support it neither.

@jbathegit
Copy link
Collaborator

Thanks for your reply @aerorahul and for clarifying that for me!

@mark-a-potts , to answer your earlier question from another thread, I would suggest tagging this new release as 11.3.2, because at some point in the future I'll need to provide an official 11.4.0 release to NCO, and it will include not only this but some other code updates as well. That may not happen until the WCOSS2 moratorium, but I guess we'll see. But in the meantime if you guys use 11.3.2 and then just increment any additional future changes in the third (z) number (e.g. 11.3.3, 11.3.4, etc.), then it should all make sense and merge together fine whenever 11.4.0 gets released.

Please let me know if you need anything else from me or if you have any further questions. And thanks to both of you for your help on this!

@aerorahul
Copy link
Contributor

Thanks for your reply @aerorahul and for clarifying that for me!

@mark-a-potts , to answer your earlier question from another thread, I would suggest tagging this new release as 11.3.2, because at some point in the future I'll need to provide an official 11.4.0 release to NCO, and it will include not only this but some other code updates as well. That may not happen until the WCOSS2 moratorium, but I guess we'll see. But in the meantime if you guys use 11.3.2 and then just increment any additional future changes in the third (z) number (e.g. 11.3.3, 11.3.4, etc.), then it should all make sense and merge together fine whenever 11.4.0 gets released.

Please let me know if you need anything else from me or if you have any further questions. And thanks to both of you for your help on this!

@jbathegit
What are the prospects of unifying the build under CMake? How long do we need to carry forward the GNUMake based build?

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

Changes work for me.
Waiting for @jbathegit to approve.

@mark-a-potts
Copy link
Contributor Author

Thanks for being flexible on the change @jbathegit

I agree that the GSI (and any other software) calling closbf should be calling openbf first, but I also think it is good coding practice to build in guards and other checks to account for the possibility that the calling routine may be incorrect and then either handle the call with a warning or fail gracefully. Fortunately, I think this solution accomplishes the former.

I will go ahead and tag this update as version 11.3.2

@mark-a-potts mark-a-potts merged commit c39f6e5 into NOAA-EMC:develop Jul 16, 2020
@jbathegit jbathegit self-assigned this Nov 17, 2020
This pull request was closed.
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.

3 participants