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

Update GTest #1738

Merged
merged 9 commits into from Apr 21, 2024
Merged

Update GTest #1738

merged 9 commits into from Apr 21, 2024

Conversation

bennibbelink
Copy link
Contributor

@bennibbelink bennibbelink commented Apr 20, 2024

According to CEP3 we should update GTest prior to release. The linked instructions for fusing GTest just points to the googletest GitHub repo, and I wasn't able to find instructions for fusing GTest. However, I followed instructions from GoogleTest (and review different installation options in the googletest README) and it appears that the method with the least limitation (and easiest maintenance) is to fetch the GTest source code within the CMake build.

This PR makes the following changes:

  • Modifies the CMake build to fetch the GTest source automatically (and build the gtest library as we did before)
  • Delete the tests/GoogleTest directory as it is no longer necessary to maintain our own GTest source
  • GTEST_HAS_PARAM_TEST is no longer defined because gtest always supports parameterized test fixtures. A handful of tests had checks for this, those checks have been removed.

Copy link

github-actions bot commented Apr 20, 2024

Downstream Build Status Report - 7e009dd - 2024-04-20 20:30:54 +0000

Build FROM cyclus_20.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_20.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Success

@coveralls
Copy link
Collaborator

coveralls commented Apr 20, 2024

Pull Request Test Coverage Report for Build 8767585032

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+8.1%) to 40.726%

Files with Coverage Reduction New Missed Lines %
tests/toolkit/infile_converters_tests.cc 1 96.88%
Totals Coverage Status
Change from base Build 8696889163: 8.1%
Covered Lines: 53137
Relevant Lines: 130473

💛 - Coveralls

@bennibbelink
Copy link
Contributor Author

Currently this PR fetches googletest v1.14.0 (the latest release)

@bennibbelink
Copy link
Contributor Author

The coverage check was failing due a decrease in coverage. I took a closer look at the coveralls report and there were odd coverage changes. According to this stackoverflow post running lcov with branch coverage can lead to many branches where scope exits are possible (like when an exception could be thrown). I think I removed to proper lcov flags for branch coverage - we may want to consider moving forward without branch coverage.

@bennibbelink
Copy link
Contributor Author

bennibbelink commented Apr 20, 2024

This is ready for review, but note that Downstream CI is not passing currently. See cyclus/cycamore#598 (comment) for the sequence of events that needs to happen.

@bennibbelink bennibbelink marked this pull request as ready for review April 20, 2024 20:57
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks @bennibbelink - definitely work a discussion on the idea of fetching Gtest with every build.

"${CMAKE_CURRENT_SOURCE_DIR}/GoogleTest/gtest/gtest-all.cc"
)
include(FetchContent)
FetchContent_Declare(
Copy link
Member

Choose a reason for hiding this comment

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

Does CMake do any caching/checking for existing installs/fetches? Or does this mean that every time CMake is invoked, the ZIP file is fetched? Seems OK for an occasional install, but annoying/a lot for developers, and even more for CI.

Obviously the existence of FetchContent means this is some kind of best (common?) practice. Should we be worried about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will cache the source files and build files in separate subdirectories of our build directory (_deps/googletest-src and _deps/googletest-build, respectively). If you re-run CMake it accesses this cache. My understanding is that ZIP will only be fetched if you run a clean build (deleting the fetched cached source/build). It automatically hides a lot of the logging so the build output isn't swamped, and in my experience building locally I have not noticed any slowdown from having to fetch the ZIP.

I don't think we should be worried about it. This is the method GoogleTest suggests in their CMake Quickstart, and CMake even uses GoogleTest as the example in their FetchContent docs and Using Dependencies Guide so I am pretty confident that it is (and will continue to be) well supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not noticed any slowdown from having to fetch the ZIP

Expanding on this a little. The FetchContent docs say:.

"Whereas ExternalProject_Add() downloads at build time, the FetchContent module makes content available immediately, allowing the configure step to use the content in commands like add_subdirectory(), include() or file() operations."

"The FetchContent_MakeAvailable() command ensures the named dependencies have been populated, either by an earlier call or by populating them itself"

I interpret that to mean it fetches content in the background while the CMake build continues to run (until you call FetchContent_MakeAvailable()). I am not sure how optimized this is or whether we take full advantage of this functionality in the build, but I don't see the build hanging anywhere while waiting to fetch the ZIP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just built locally with FetchContent in verbose mode and can confirm that the population happens very fast - the ZIP is only 1MB. If we were fetching a larger project I might suggest re-ordering our CMake so that we could take advantage of background fetching but I don't think that is necessary here.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks so much for the thorough information @bennibbelink

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks for another step forward @bennibbelink

@gonuke gonuke merged commit 26f5438 into cyclus:main Apr 21, 2024
10 checks passed
@bennibbelink bennibbelink deleted the gtest branch April 21, 2024 17:38
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.

None yet

3 participants