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

#349: use docker images from DARMA-tasking/workflows #377

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

#349: use docker images from DARMA-tasking/workflows #377

wants to merge 12 commits into from

Conversation

cz4rs
Copy link
Contributor

@cz4rs cz4rs commented Nov 20, 2024 •
edited
Loading

fixes #349

  • use the images from DARMA-tasking/workflows repository
  • remove redundant docker code and dependency scripts (they now reside in DARMA-tasking/workflows)
  • clean up and simplify GTest integration

@cz4rs cz4rs closed this Nov 28, 2024
@cz4rs cz4rs reopened this Nov 28, 2024
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from 444500e to 645e1f0 Compare November 28, 2024 12:36
@cz4rs cz4rs mentioned this pull request Nov 28, 2024
Merged
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch 2 times, most recently from 59f8fa3 to a4a4d44 Compare November 28, 2024 13:11
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from a4a4d44 to 42c2ff9 Compare December 31, 2024 10:38
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from 42c2ff9 to 2fcf7c5 Compare January 14, 2025 18:34
@cz4rs cz4rs mentioned this pull request Jan 14, 2025
Closed
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from 2fcf7c5 to bf4c83c Compare January 16, 2025 17:33
cz4rs added 2 commits January 16, 2025 18:43
@cz4rs
#349: ci: use DARMA-tasking/workflows images
bfdaec3 
Use images from the `workflows` repository and remove redundant docker
code in `magistrate`.
@cz4rs
#349: move Kokkos header inside the #if check
961dd0e
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from bf4c83c to a81d7e7 Compare January 16, 2025 17:44
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from a81d7e7 to 12aa10e Compare January 16, 2025 18:01
cz4rs
cz4rs commented Jan 16, 2025
View reviewed changes
tests/CMakeLists.txt
FetchContent_Declare(
googletest
DOWNLOAD_EXTRACT_TIMESTAMP FALSE
URL https://github.com/google/googletest/archive/refs/tags/release-1.12.1.tar.gz
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping 1.12.1 version here. Upgrade can be handled separately in #367.

cz4rs
cz4rs commented Jan 16, 2025
View reviewed changes
CMakeLists.txt
Comment on lines 147 to +151
if (magistrate_tests_enabled
AND "${CMAKE_PROJECT_NAME}" STREQUAL "${PROJECT_NAME}")
# CTest implies enable_testing() and defines the BUILD_TESTING option.
# The default of BUILD_TESTING is ON.
# Testing is only enabled if the actual project being built is VT.
# Testing is only enabled if the actual project being built is magistrate.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This condition seems strong enough to remove MAGISTRATE_HAS_GTEST and DISABLE_TPL_GTEST check.

cz4rs
cz4rs commented Jan 16, 2025
View reviewed changes
tests/CMakeLists.txt
@@ -2,6 +2,23 @@
set(PROJECT_TEST_UNIT_DIR ${CMAKE_CURRENT_SOURCE_DIR}/unit)
set(PROJECT_TEST_MPI_UNIT_DIR ${CMAKE_CURRENT_SOURCE_DIR}/unit/tests_mpi)

find_package(GTest)
if (NOT GTest_FOUND)
include(FetchContent)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use CMake's FetchContent instead of pre-installing GTest in Docker image.
This makes it possible to update GTest version independently in various DARMA projects.

@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from 137aaf5 to 14c038f Compare January 16, 2025 18:21
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from 699c49b to 5343692 Compare January 16, 2025 18:33
cz4rs
cz4rs commented Jan 16, 2025
View reviewed changes
docker-compose.yml Outdated Show resolved Hide resolved
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from 3a305ad to 8411e9a Compare January 16, 2025 21:24
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from 8411e9a to bf0a188 Compare January 17, 2025 14:46
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch 2 times, most recently from 46077fe to e136cce Compare January 20, 2025 18:57
@cz4rs cz4rs marked this pull request as ready for review January 23, 2025 10:45
@cz4rs cz4rs force-pushed the 349-docker-images-cleanup branch from e136cce to b7aa32a Compare January 23, 2025 10:46
@cz4rs cz4rs mentioned this pull request Jan 23, 2025
Open
2 tasks
@cz4rs cz4rs changed the title #349: docker images cleanup #349: use docker images from DARMA-tasking/workflows Jan 23, 2025
cwschilly
cwschilly approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@cwschilly cwschilly left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwschilly cwschilly cwschilly approved these changes

@lifflander lifflander Awaiting requested review from lifflander

@nmm0 nmm0 Awaiting requested review from nmm0

@nlslatt nlslatt Awaiting requested review from nlslatt

@pierrepebay pierrepebay Awaiting requested review from pierrepebay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CI] switch to using docker images from DARMA-tasking/workflows
2 participants
@cz4rs @cwschilly