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

Updates to VersionInfo module and its dependencies #1124

Merged
merged 5 commits into from
Jun 1, 2022

Conversation

bjonkman
Copy link
Contributor

@bjonkman bjonkman commented May 9, 2022

Feature or improvement description
The VersionInfo module was originally created to avoid having the version information at the lowest level of the build. This would avoid having to rebuild NWTC Library every time the git hash changed. (See #267 and #269.) However, in a later pull request, the NWTC Library added a dependency on VersionInfo (see #428), putting VersionInfo at the lowest level of the build again.

This pull request takes the subroutines from NWTC_IO.f90 that use version information and puts them into the VersionInfo module so that NWTC_Library no longer uses the VersionInfo module.

Related issue, if one exists
#267, #269, #428

Impacted areas of the software

  • Unit tests: NWTC Library unit test for CheckArgs is now in a VersionInfo unit test
  • Cmake build system: more modules depend on versioninfolib
  • FAST_subs: the GetVersion subroutine is now in VersionInfo so that other driver codes can call the routine if desired.
  • Driver codes that use CheckArgs or any subroutine to display version info have been updated.

Additional supporting information
I also renamed the (unused) modules/nwtc_library/test directory to modules/nwtc_library/old_test to avoid confusion with the modules/nwtc_library/tests directory.

Test results, if applicable
This should not change any test results, though the screen output for driver codes may change.

bjonkman added 3 commits May 9, 2022 10:59
…dule

-moved all code that was in NWTC_IO that called the version info module into the version info module. This allows us to rebuild the version info stuff without having to rebuild the NWTC library.
- Updated cmake dependencies on VersionInfo + NWTC_Library
- Also moved some of the more generic GetVersion functions from FAST into the version info module so we won't have to duplicate code with every driver that wants to use it.
Moved CheckArgs test from NWTC_IO into VersionInfo
I had a hard time remembering if the new test directory was the `test` or `tests` one, so I renamed `test` to `Old_test` to help me remember.
Copy link
Collaborator

@andrew-platt andrew-platt left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think we should merge this as part of the 3.2 milestone.

@andrew-platt andrew-platt requested a review from rafmudaf May 18, 2022 22:56
use nwtc_library_test_tools
use VersionInfo
! use nwtc_library_test_tools
! bjj: I haven't figured out how to get the unit tests to consistently find the file
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rafmudaf, any thoughts on getting cmake/ctest to find the nwtc_library_test_tools.F90 consistently?

@andrew-platt andrew-platt added this to the v3.2.0 milestone May 18, 2022
@andrew-platt andrew-platt removed the request for review from rafmudaf May 24, 2022 19:20
@rafmudaf
Copy link
Collaborator

Thanks for this pull request @bjonkman. I'm documenting this info primarily for our own future reference.

The issue in the previous implementation of the git-based version numbering via CMake was that it was done through a compiler directive. This meant that any change to the git-status also changed the compiler command (see v1.0.0) and every file then had to be recompiled. The VersionInfo module was added so that instead only one file gets a new compiler command when the version changes. This allows all other modules to link to VersionInfo without having to be recompiled.

I tested changing the version (i.e. the git status) and compiling nwtc_library_utest on dev so not including the changes in this pull request.

## STARTING 1 COMMIT PRIOR TO `dev`

(openfast) >>mbp@~/Development/openfast_forks/bonnie/build (dev)$ git co b0ac7ad5
Note: switching to 'b0ac7ad5'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at b0ac7ad5 Merge pull request #1050 from ptrbortolotti/stability
(openfast) >>mbp@~/Development/openfast_forks/bonnie/build ((b0ac7ad5...))$ cmake ..
-- Enabling Fortran 2008 features
-- Setting system file as: src/SysGnuLinux.f90
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/rmudafor/Development/openfast_forks/bonnie/build
(openfast) >>mbp@~/Development/openfast_forks/bonnie/build ((b0ac7ad5...))$ make nwtc_library_utest -j2
Scanning dependencies of target versioninfolib
[  0%] Creating directories for 'pfunit'
[  0%] Building Fortran object modules/version/CMakeFiles/versioninfolib.dir/src/VersionInfo.f90.o
[  0%] No download step for 'pfunit'
[  0%] No update step for 'pfunit'
[  7%] Linking Fortran static library libversioninfolib.a
[ 14%] No patch step for 'pfunit'
[ 14%] Built target versioninfolib
Scanning dependencies of target nwtclibs

... REMOVED THE COMPILE DETAILS But trust me in that this was the full compile for this target...

[100%] Linking Fortran executable nwtc_library_utest
[100%] Built target nwtc_library_utest


## Checkout `dev`

(openfast) >>mbp@~/Development/openfast_forks/bonnie/build ((b0ac7ad5...))$ git co dev
Previous HEAD position was b0ac7ad5 Merge pull request #1050 from ptrbortolotti/stability
Switched to branch 'dev'
Your branch is up to date with 'openfast/dev'.

(openfast) >>mbp@~/Development/openfast_forks/bonnie/build (dev)$ make nwtc_library_utest -j2
-- Enabling Fortran 2008 features
-- Setting system file as: src/SysGnuLinux.f90
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/rmudafor/Development/openfast_forks/bonnie/build
Scanning dependencies of target versioninfolib
[ 14%] Built target pfunit
[ 14%] Building Fortran object modules/version/CMakeFiles/versioninfolib.dir/src/VersionInfo.f90.o
[ 21%] Linking Fortran static library libversioninfolib.a
[ 21%] Built target versioninfolib
Scanning dependencies of target nwtclibs
[ 85%] Built target nwtclibs
Scanning dependencies of target nwtc_library_utest
[ 85%] Building Fortran object unit_tests/nwtc-library/CMakeFiles/nwtc_library_utest.dir/__/pfunit/include/driver.F90.o
[ 85%] Linking Fortran executable nwtc_library_utest
[100%] Built target nwtc_library_utest

At least in CMake, I think this is a desirable compile pipeline. Waay back in #42, we decided to remove the specific library versions in favor of a global OpenFAST version. In the current implementation, each compile target gets its version by linking to VersionInfo. With this pull request, the git-status (therefore, the version) can change and NWTC Library recompiled, but it is no longer associated with a version number. However, I don't think this matters in practice since in order to get a module library's version, it would have to link with VersionInfo at compile-time. It's a "does a tree fall in the woods" situation.

Continuing from above, the test below checks out this pull request's branch and compiles nwtc_library_utest. Here, VersionInfo is not recompiled even though the version has changed. But, again, we would never know that because there's no method in NWTC Library to print the version and if there were it would have to be linked to VersionInfo. The same goes for all libraries in OpenFAST.

## Checkout the current pull request branch

(openfast) >>mbp@~/Development/openfast_forks/bonnie/build (dev)$ git co f/VersionInfo 
Switched to branch 'f/VersionInfo'
Your branch is up to date with 'origin/f/VersionInfo'.
(openfast) >>mbp@~/Development/openfast_forks/bonnie/build (f/VersionInfo)$ make nwtc_library_utest -j2
-- Enabling Fortran 2008 features
-- Setting system file as: src/SysGnuLinux.f90
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/rmudafor/Development/openfast_forks/bonnie/build
Scanning dependencies of target nwtclibs
[ 16%] Built target pfunit
[ 16%] Building Fortran object modules/nwtc-library/CMakeFiles/nwtclibs.dir/src/NWTC_IO.f90.o
[ 16%] Building Fortran object modules/nwtc-library/CMakeFiles/nwtclibs.dir/src/NWTC_Num.f90.o
[ 25%] Building Fortran object modules/nwtc-library/CMakeFiles/nwtclibs.dir/src/NWTC_RandomNumber.f90.o
[ 25%] Building Fortran object modules/nwtc-library/CMakeFiles/nwtclibs.dir/src/ModMesh_Types.f90.o
[ 25%] Building Fortran object modules/nwtc-library/CMakeFiles/nwtclibs.dir/src/ModMesh.f90.o
[ 25%] Building Fortran object modules/nwtc-library/CMakeFiles/nwtclibs.dir/src/ModMesh_Mapping.f90.o
[ 25%] Building Fortran object modules/nwtc-library/CMakeFiles/nwtclibs.dir/src/NWTC_Library.f90.o
[ 25%] Building Fortran object modules/nwtc-library/CMakeFiles/nwtclibs.dir/src/NetLib/fftpack/NWTC_FFTPACK.f90.o
[ 25%] Linking Fortran static library libnwtclibs.a
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libnwtclibs.a(SingPrec.f90.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libnwtclibs.a(SingPrec.f90.o) has no symbols
[ 83%] Built target nwtclibs
Scanning dependencies of target nwtc_library_utest
[ 83%] Building Fortran object unit_tests/nwtc-library/CMakeFiles/nwtc_library_utest.dir/__/tests/nwtc-library/NWTC_Library_test_tools.F90.o
[ 83%] Building Fortran object unit_tests/nwtc-library/CMakeFiles/nwtc_library_utest.dir/__/tests/nwtc-library/test_NWTC_IO_FileInfo.F90.o
[ 91%] Building Fortran object unit_tests/nwtc-library/CMakeFiles/nwtc_library_utest.dir/__/pfunit/include/driver.F90.o
[ 91%] Building Fortran object unit_tests/nwtc-library/CMakeFiles/nwtc_library_utest.dir/__/tests/nwtc-library/test_NWTC_RandomNumber.F90.o
[100%] Linking Fortran executable nwtc_library_utest
[100%] Built target nwtc_library_utest

@rafmudaf rafmudaf merged commit f0e4140 into OpenFAST:dev Jun 1, 2022
@bjonkman bjonkman deleted the f/VersionInfo branch June 14, 2022 15:43
@rafmudaf rafmudaf mentioned this pull request Jul 8, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants