Skip to content

Conversation

pratikvn
Copy link
Member

Enable the file-config example conditionally, as it depends on nlohmann-json.

@pratikvn pratikvn added 1:ST:ready-for-review This PR is ready for review 1:ST:no-changelog-entry Skip the wiki check for changelog update labels Nov 15, 2024
@pratikvn pratikvn self-assigned this Nov 15, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:example This is related to the examples. labels Nov 15, 2024
@pratikvn pratikvn requested review from a team November 15, 2024 21:40
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

find_package should ideally always happen on the topmost level, so that we don't look for different configurations accidentally. I would suggest moving the call to the root CMakeLists.txt. Is there a downside to always pulling in the dependency? I would like to avoid too many fragmented possible configurations depending on small environment differences.

@yhmtsai yhmtsai mentioned this pull request Nov 18, 2024
@pratikvn pratikvn force-pushed the example-fix-file-config branch from 49aba31 to 600417b Compare November 22, 2024 14:13
@pratikvn pratikvn requested a review from upsj November 22, 2024 14:13
@pratikvn pratikvn requested a review from yhmtsai November 22, 2024 14:37
@pratikvn pratikvn added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Nov 22, 2024
@pratikvn pratikvn changed the title Enable the file-config example conditionally Always enable nlohmann_json with examples Nov 22, 2024
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 112 to 120
foreach(executor IN LISTS executors)
add_test(NAME example_file-config-solver_${config_name}_${executor}
COMMAND
"$<TARGET_FILE:file-config-solver>"
"${executor}" "${CMAKE_CURRENT_SOURCE_DIR}/file-config-solver/config/${config_name}.json"
"${CMAKE_CURRENT_SOURCE_DIR}/file-config-solver/data/A.mtx"
WORKING_DIRECTORY
"$<TARGET_FILE_DIR:ginkgo>")
add_test(NAME example_file-config-solver_${config_name}_${executor}
COMMAND
"$<TARGET_FILE:file-config-solver>"
"${executor}" "${CMAKE_CURRENT_SOURCE_DIR}/file-config-solver/config/${config_name}.json"
"${CMAKE_CURRENT_SOURCE_DIR}/file-config-solver/data/A.mtx"
WORKING_DIRECTORY
"$<TARGET_FILE_DIR:ginkgo>")
# Prevent performance issues with high core counts
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't actually change anything, right? I would just revert it then for a cleaner diff

@pratikvn pratikvn merged commit 8fa7bc5 into develop Nov 25, 2024
10 of 11 checks passed
@pratikvn pratikvn deleted the example-fix-file-config branch November 25, 2024 13:37
MarcelKoch pushed a commit to MarcelKoch/ginkgo that referenced this pull request Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1:ST:no-changelog-entry Skip the wiki check for changelog update 1:ST:ready-to-merge This PR is ready to merge. reg:build This is related to the build system. reg:example This is related to the examples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants