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

cmake: Handle generated files in core_add_test correctly #9692

Merged
merged 3 commits into from May 4, 2016

Conversation

smspillaz
Copy link
Contributor

@smspillaz smspillaz commented Apr 27, 2016

Fixes #16708

# otherwise use the absolute path. We can't just prepend
# CMAKE_CURRENT_SOURCE_DIR as this source file might be genertaed
# in the build directory (in which case it would have been passed
# to SOURCES as an absolute path).

This comment was marked as spam.

This comment was marked as spam.

A source file might have an absolute path if it was being generated
and stored in the binary directory.
@smspillaz
Copy link
Contributor Author

@wsnipex Let me know if there's additional things that need to be addressed here.

@wsnipex
Copy link
Member

wsnipex commented May 2, 2016

fine for me. Maybe @fetzerch or @notspiff have an idea how to handle such generated files

@akva2
Copy link
Contributor

akva2 commented May 2, 2016

trivial bit; variable should be CORE prefixed (yes i know not entirely consistent).

i don't quite get the solution. while some of the contents of these tests are generated, the things we snoop for shouldn't be so we can as well snoop the template files afaik.

@smspillaz
Copy link
Contributor Author

@akva2 I was thinking more of the case where you have a "support" file which is generated. That file might not contain any tests, but is used by the tests. This happens in a PR that I am working on. Something has to be done with the generated file and in this case ignoring it makes the most sense. I gather that there might be templates which contain tests - I suppose the best way to handle that is to add them to the SOURCES of the test target (they will be automatically ignored by the build system, but still available for use by things like gtest_add_tests)

I don't prefer the approach taken by FindGTest.cmake in regexing files for tests. In practice I've found it to be quite fragile. That discussion is probably out of scope for the purposes of this PR, but maybe worth looking into in the future?

@akva2
Copy link
Contributor

akva2 commented May 2, 2016

the problem is that the regex'ing is the only way to map gtest to ctest. otherwise you just get a single test under ctest. i'm all ears for a better approach but it's the only one i have found.

i don't see why we need to bother with generated resources here at all. you just make the test target depend on the resource file and it will be generated before the test is executed.

@smspillaz
Copy link
Contributor Author

@akva2 Ah, that's not really the problem I was referring to.

The problem is this:

add_custom_command (OUTPUT my_generated_support_file.cpp
                                       COMMAND ...)

set (SOURCES ...
                          my_generated_support_file.cpp)
core_add_test (...)

In this case, the test uses symbols from my_generated_support_file.cpp. However, core_add_test calls gtest_add_tests which does something like this:

get_property (TEST_SOURCES TARGET test_target PROPERTY SOURCES)
foreach (SOURCE ${TEST_SOURCES})
    file (READ ${SOURCE} ...)
endforeach ()

However, at that point, ${SOURCE} might not actually exist on-disk, because it hasn't been generated yet (only happens at build time, not configure time). Because of that, we have to skip it.

@akva2
Copy link
Contributor

akva2 commented May 2, 2016

aha. now i get it. it's an unfortunate assumption on my part, that all sources going into the test executable will hold tests. then this should be fine, but the variable name is slightly confusing. this is not only for generated sources, but test support sources (or whatever name you want to apply, but generated is misleading).

note i do understand why them being generated trigger the issue, but thinking a bit wider, that's not the actual issue here is all i'm saying.

@akva2
Copy link
Contributor

akva2 commented May 2, 2016

actually looking at the code, what i'm suggesting is to add a second list of sources to be linked into the test application which is not passed to gtest_add_tests, then give core_add_test another parameter for such sources. can you take care of that? i can, but it won't be now for sure, likely in a few days.

if(NOT ARGN)
message(FATAL_ERROR "Missing ARGN: Read the documentation for GTEST_ADD_TESTS")
endif()
foreach(source ${ARGN})
get_property(GENERATED SOURCE "${source}" PROPERTY GENERATED)

This comment was marked as spam.

Generated files don't exist yet, so they can't just be passed to
file(READ). Unfortunately, the GENERATED property doesn't help
that much here, since it is only visible to lists in the same
directory as the relevant source file. Since we call gtest_add_tests
at the toplevel CMakeLists.txt, it won't be visible.

Generated files which form part of the tests but don't contain
any tests themselves should be put into SUPPORT_SOURCES before
core_add_test is called. In that case, they won't be considered
as part of the library.
@wsnipex
Copy link
Member

wsnipex commented May 3, 2016

this looks much cleaner now.
Could you please add a note in the README about support_sources usage in tests?

@smspillaz
Copy link
Contributor Author

smspillaz commented May 4, 2016 via email

@akva2
Copy link
Contributor

akva2 commented May 4, 2016

Good stuff thx for patience.

@wsnipex
Copy link
Member

wsnipex commented May 4, 2016

great, thanks!

@wsnipex wsnipex merged commit a031023 into xbmc:master May 4, 2016
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

4 participants