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.

Sign up for GitHub

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

Jump to bottom

Automate building external libraries #694

Merged
merged 32 commits into from
Jan 25, 2024
Merged

Automate building external libraries #694

merged 32 commits into from
Jan 25, 2024

Conversation

program--
Copy link
Contributor

@program-- program-- commented Dec 28, 2023 •
edited by PhilMiller
Loading

This PR provides automation around building external libraries based on options selected.

Additions

  • Adds options
    • NGEN_WITH_EXTERN_SLOTH
    • NGEN_WITH_EXTERN_TOPMODEL (Dependent on NGEN_WITH_BMI_C)
    • NGEN_WITH_EXTERN_CFE (Dependent on NGEN_WITH_BMI_C)
    • NGEN_WITH_EXTERN_PET (Dependent on NGEN_WITH_BMI_C)
    • NGEN_WITH_EXTERN_NOAH_OWP_MODULAR (Dependent on NGEN_WITH_BMI_FORTRAN)
    • NGEN_WITH_EXTERN_ALL: enables all models if the dependency is enabled, i.e. sets NGEN_WITH_EXTERN_CFE=ON if NGEN_WITH_BMI_C=ON.
  • Adds cmake/ExternalSubdirectory.cmake which provides the add_external_subdirectory function. This is a simple convenience function wrapping add_subdirectory that can auto-update git submodules (GIT_UPDATE), and ensures a given target is imported (IMPORTS).
  • Automates building test BMI libraries when a dependent option is enabled, i.e. NGEN_WITH_BMI_C means testbmicmodel will be built.
  • Adds a Fortran compiler configuration step in the ngen-build action.

Changes

  • Updates build summary output to include new options and C/CXX flags set.
  • Modifies ngen_add_test to allow DEPENDS arguments, which are dependent targets for that test. This differs from REQUIRES and LIBRARIES in that DEPENDS are simply dependent targets, and otherwise have no relation/linkage to the target.
  • Modifies CMake listfile for noah-owp-modular, iso_c_bmi, and testbmifortran to better handle building in conjunction with NGen, or independently.
  • Modifies the Docker container used for an example run of NGen.

Removals

  • Removes the deprecated option -DPACKAGE_TESTS since this conflicts with SLoTH when auto-building.
  • Removes FindBMI_FORTRAN_ISO_C module since this PR removes the need for it.

TODO

  • Resolve including git submodules in new ngen.dockerfile.
  • Resolve CFE BMI sanitizer error.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Target Environment support

  • Linux
  • macOS

@program-- program-- added the build Issues related to CMake and building ngen label Dec 28, 2023
@program-- program-- requested a review from PhilMiller December 28, 2023 15:56
@program-- program-- self-assigned this Dec 28, 2023
PhilMiller
PhilMiller reviewed Dec 28, 2023
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
PhilMiller
PhilMiller reviewed Dec 28, 2023
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
PhilMiller
PhilMiller reviewed Dec 28, 2023
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
PhilMiller
PhilMiller reviewed Dec 28, 2023
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
PhilMiller
PhilMiller reviewed Dec 28, 2023
View reviewed changes
test/CMakeLists.txt Outdated Show resolved Hide resolved
@program-- program-- marked this pull request as ready for review January 3, 2024 22:30
@PhilMiller
Copy link
Contributor

The to-do left here is to update the version of cfe we test against to incorporate NOAA-OWP/cfe#101 and see it pass the tests, right?

@PhilMiller PhilMiller mentioned this pull request Jan 9, 2024
Merged
11 tasks
@PhilMiller
Copy link
Contributor

#664 already has that CFE update in progress

@program--
Copy link
Contributor Author

The to-do left here is to update the version of cfe we test against to incorporate NOAA-OWP/cfe#101 and see it pass the tests, right?

That is correct.

@aaraney
Copy link
Member

aaraney commented Jan 11, 2024

#664 has been merged.

@program-- program-- force-pushed the jsm-automate-extern branch from d265dea to fe8836c Compare January 11, 2024 19:48
PhilMiller
PhilMiller reviewed Jan 11, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
PhilMiller
PhilMiller reviewed Jan 11, 2024
View reviewed changes
docker/ngen.dockerfile Outdated Show resolved Hide resolved
PhilMiller
PhilMiller previously approved these changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@PhilMiller PhilMiller left a comment

Choose a reason for hiding this comment

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

Anyone else want to have a look at this before it gets merged?

@PhilMiller
Copy link
Contributor

Let's mention this on the stand-up tomorrow, and merge if no one wants to hold it for their own review.

@program--
Copy link
Contributor Author

@PhilMiller Is there anything else needed for this? Otherwise, I think we're good to merge now.

program-- and others added 2 commits January 24, 2024 16:37
@program--
cmake: automate building extern libraries; see extended commit msg
cde8857 
- adds options:
  + NGEN_WITH_EXTERN_CXX_MODELS
  + NGEN_WITH_EXTERN_C_MODELS
  + NGEN_WITH_EXTERN_FORTRAN_MODELS
- modifies ngen_add_test to allow `DEPENDS` arguments,
  which are dependent targets for that test. This differs
  from `REQUIRES` and `LIBRARIES` in that `DEPENDS` are simply
  dependent targets, and otherwise have no relation/linkage to the target.
- adds cmake/ExternalSubdirectory.cmake which provides the
  `add_external_subdirectory` function. This is a simple convenience
  function wrapping `add_subdirectory`.
@PhilMiller @program--
Be more exact in disabling SLoTH testing
30b5c8c
program-- and others added 22 commits January 24, 2024 16:37
@program--
cmake: replace add_fortran_library so projects can be built independe…
4f3f06e 
…ntly; add netcdf find for noah-owp-modular
@program--
gh: rm building submodules for fortran BMI tests
1a2860b
@program--
cmake: fix target link, bmi_fortran_iso_c -> iso_c_bmi
beaa72c
@program--
cmake: change scope of compile options to PRIVATE to prevent non-port…
cf97f6a 
…ability
@program--
gh: attempt to specify fortran compiler for test_bmi_fortran job
90d218c
@program--
gh: add fortran compiler setup step for macOS runners when bmi_fortra…
fb94360 
…n is ON
@program--
cmake: add correct requirements/dependencies for test_bmi_multi
02ba321
@program--
gh: fix placement of fortran env step; cmake: add missing ffree-line-…
a148143 
…length-none option
@program--
gh: remove submod build steps for integration tests
cdca20c
@program--
gh: refactor docker workflow
19f6e5e
@program--
docker: revert pybind/gtest submodule updates; cmake: ensure sloth ha…
0279bfc 
…s default visbility instead of hidden
@program--
docker: include git files in build context
9d7b831
@program--
cmake: rm find bmi_fortran_iso_c module
6390adb
@program--
cmake: update submodules on configure
7bbb846
@program--
cmake: update extern bmi-cxx
f5e3860
@program--
cmake: add recursive flag to git submodules function
98320e7
@program--
cmake: finer options for building models; default visibility for impo…
94481f3 
…rted targets
@program--
cmake: revert visibility modification to just sloth
a538d33
@program--
cmake: add ngen_with_extern_all option
d211c61
@program-- @PhilMiller
cmake: rm fully-deprecated option; ensure all support
3827696 
Co-authored-by: Phil Miller <[email protected]>
@program--
gh: rm submodule build steps
341c03b
@program-- @PhilMiller
docker: update boost URL
3576b76 
Co-authored-by: Phil Miller <[email protected]>
PhilMiller
PhilMiller reviewed Jan 25, 2024
View reviewed changes
docker/ngen.dockerfile Outdated Show resolved Hide resolved
@program-- program-- force-pushed the jsm-automate-extern branch from 5e2cf79 to 0c378c0 Compare January 25, 2024 00:44
@PhilMiller PhilMiller merged commit 28dee37 into NOAA-OWP:master Jan 25, 2024
19 checks passed
@program-- program-- deleted the jsm-automate-extern branch February 12, 2024 23:27
@hellkite500 hellkite500 mentioned this pull request May 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZacharyWills ZacharyWills ZacharyWills left review comments

@PhilMiller PhilMiller PhilMiller approved these changes

@hellkite500 hellkite500 Awaiting requested review from hellkite500

Assignees

@program-- program--

Labels
build Issues related to CMake and building ngen
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@program-- @PhilMiller @aaraney @hellkite500 @ZacharyWills