-
Notifications
You must be signed in to change notification settings - Fork 12
Make the CMake examples be independent projects #40
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
Open
LecrisUT
wants to merge
4
commits into
fortuno-repos:main
Choose a base branch
from
LecrisUT:cmake/review
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| # This file is part of Fortuno. | ||
| # Licensed under the BSD-2-Clause Plus Patent license. | ||
| # SPDX-License-Identifier: BSD-2-Clause-Patent | ||
|
|
||
| function(Fortuno_add_test test) | ||
| #[===[.md | ||
| # Fortuno_add_test | ||
|
|
||
| Internal helper for adding functional tests testing whole CMake projects. | ||
|
|
||
| ## Synopsis | ||
| ```cmake | ||
| Template_add_test(<name> | ||
| [TEST_NAME <test_name>] | ||
| ) | ||
| ``` | ||
|
|
||
| ## Options | ||
|
|
||
| `<name>` | ||
| : Path to the CMake project to be executed relative to `${CMAKE_CURRENT_SOURCE_DIR}` | ||
|
|
||
| `TEST_NAME` [Default: `<name>`] | ||
| : Name for the test to be used as the ctest name | ||
| ]===] | ||
|
|
||
| set(ARGS_Options) | ||
| set(ARGS_OneValue | ||
| TEST_NAME | ||
| ) | ||
| set(ARGS_MultiValue) | ||
| cmake_parse_arguments(PARSE_ARGV 1 ARGS "${ARGS_Options}" "${ARGS_OneValue}" "${ARGS_MultiValue}") | ||
|
|
||
| # Check required/optional arguments | ||
| if (ARGC LESS 1) | ||
| message(FATAL_ERROR "Missing test name") | ||
| endif () | ||
| if (NOT DEFINED ARGS_TEST_NAME) | ||
| set(ARGS_TEST_NAME ${test}) | ||
| endif () | ||
|
|
||
| set(configure_args | ||
| -DCMAKE_Fortran_COMPILER=${CMAKE_Fortran_COMPILER} | ||
| ) | ||
| if (Fortuno_IS_TOP_LEVEL) | ||
| list(APPEND configure_args | ||
| # Generated Config file point to binary targets until it is installed | ||
| -DFortuno_ROOT=${Fortuno_BINARY_DIR} | ||
| -DFETCHCONTENT_SOURCE_DIR_FORTUNO=${Template_SOURCE_DIR} | ||
| ) | ||
| endif () | ||
|
|
||
| add_test(NAME ${ARGS_TEST_NAME} | ||
| COMMAND ${CMAKE_CTEST_COMMAND} --build-and-test ${CMAKE_CURRENT_SOURCE_DIR}/${test} | ||
| ${CMAKE_CURRENT_BINARY_DIR}/${test} | ||
| # Use the same build environment as the current runner | ||
| --build-generator "${CMAKE_GENERATOR}" | ||
| --build-options ${configure_args} | ||
| --test-command ${CMAKE_CTEST_COMMAND} | ||
| --test-dir ${CMAKE_CURRENT_BINARY_DIR}/${test} | ||
| --output-on-failure | ||
| ) | ||
| endfunction() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stupid question, but please help me to learn 😉 : What is the point of having the basic package version file and package config file outside of the install guard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in general I'd prefer to have separate ones (sorry!), as I find it easier to discuss about specific single topics as about too many at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on experiences with former projects, I think it would be worth to keep examples and tests separately for didactical purposes:
If we mix the two by putting everything under tests, first time users would be either confused and not finding the place to start or grabbing something, which was for internal testing only and not meant to be used by consumers of the library.
By the way, Fortran projects created with
fpm --fullalso have both, atestand anexampledirectory, so it might become a common pattern in the Fortran world.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, now, I see how you did that with symlinks. OK, that could be reasonable solution. When one turns the examples into individual projects (which probably indeed makes a lot of more sense), then the export test would be superfluous, which was testing whether the compiled library can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is one thing that I didn't understood until recently.
configure_package_config_fileand such are just fancy versions ofconfigure_filewhich generates the file (FortunoConfig.cmake) and puts it in the build_dir. And in the definition it tries to look for aFortunoTargets.cmakefile right next to itinstall(EXPORT)does not generate theFortunoTargets.cmakein build_dir, only in the install prefixexport(EXPORT)generates theFortunoTargets.cmakein the build_dir (with the context of the build targets, instead of the installed ones, e.g.target_include_directorieswill evaluate the$<BUILD_INTERFACE>instead of the$<INSTALL_INTERFACE>)As for why we would need the
FortunoConfig.cmakefile in the build_dir, it's for running the tests without needing to run the install stepNP, this part is relatively self-contained to making the examples as independent projects, I'll split off the description and make it an issue.
Yes, I was suspecting that that was the case. We still need them for now, because the running of the current example tests is disabled because they seem to fail by design. We can probably mark the relevant tests with
WILL_FAILorPASS_REGEX, but we need to split the tests to run individual tests and select which ones are meant to fail.