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

New Test Dependency Handling #1571

Merged
merged 7 commits into from May 16, 2017

Conversation

tjhei
Copy link
Member

@tjhei tjhei commented May 10, 2017

No description provided.

SET(_test_full ${_test})
GET_FILENAME_COMPONENT(_test ${_test} NAME_WE)
GET_FILENAME_COMPONENT(testname ${_test} NAME_WE)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's a temp variable -- make it _testname?


IF(_use_test)

MESSAGE(STATUS "** processing test ${testname}:")
Copy link
Contributor

Choose a reason for hiding this comment

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

that's going to produce a lot of output

Copy link
Member Author

Choose a reason for hiding this comment

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

it all only lands in tests/setup_tests.log

SET(_testlib 'set Additional shared libraries = ./lib${_test}.so')
SET(_testdepends ${_test})
SET(_testlib 'set Additional shared libraries = ./lib${testname}.so')
SET(_testdepends ${testname})
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, so this weirds me out a bit: you've extracted a testname, and now you're also making ${testname} into a target that you depend on?

Copy link
Contributor

Choose a reason for hiding this comment

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

what I'm getting at is that it's weird that the target for the shared lib is called the same as the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

added a comment. But <xyz> creates lib<xyz>.so.

FILE(GLOB_RECURSE _outputs RELATIVE ${CMAKE_CURRENT_SOURCE_DIR}/${_test}/
${CMAKE_CURRENT_SOURCE_DIR}/${_test}/[a-zA-Z0-9]*)
FILE(GLOB_RECURSE _outputs RELATIVE ${CMAKE_CURRENT_SOURCE_DIR}/${testname}/
${CMAKE_CURRENT_SOURCE_DIR}/${nametest}/[a-zA-Z0-9]*)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be ${testname} in the second line of the command!

Copy link
Member Author

Choose a reason for hiding this comment

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

yes!

COMMAND cp ${CMAKE_CURRENT_SOURCE_DIR}/${_test}.prm
${CMAKE_CURRENT_BINARY_DIR}/${_test}.x.prm
ADD_CUSTOM_COMMAND(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${testname}.x.prm
COMMAND echo "making ${CMAKE_CURRENT_BINARY_DIR}/${testname}.x.prm"
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to keep the echo?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

${CMAKE_CURRENT_SOURCE_DIR}
${CMAKE_CURRENT_BINARY_DIR}
DEPENDS ${_ref_file}
${_run_file})
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this was the problem with BYPRODUCT: ${_run_file} is an unsatisfiable dependency because we have no targets that produce them. Don't you want to depend on screen-output.notime here for which there actually is a rule that has it as output?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. ${_run_file} is the normalized bla.notime file that is generated by an explicit rule.

# in subdirectories (for example solution/solution...), so replace them.
STRING(REPLACE "/" "-" _target_name ${testname}.${_name}.diff)
ADD_CUSTOM_TARGET(${_target_name}
DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/output-${testname}/${_name}.diff)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is one of the phony targets you couldn't get rid of

# STRING(REPLACE "/" "-" _target_name_copy_output make-${_test}-${_output}.cmp.notime)
# ADD_CUSTOM_TARGET(${_target_name_copy_output}#
# DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/output-${_test}/${_output}.cmp.notime)
# ADD_DEPENDENCIES(${_test}.normalize ${_target_name_copy_output})
Copy link
Contributor

Choose a reason for hiding this comment

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

what's happening here with these commented out things? same in the following lines?

@bangerth
Copy link
Contributor

ping?

@tjhei
Copy link
Member Author

tjhei commented May 13, 2017

pong!

@tjhei tjhei force-pushed the test_dependencies_reloaded branch from b485073 to 7fea806 Compare May 13, 2017 23:51
@tjhei
Copy link
Member Author

tjhei commented May 16, 2017

What do we do about this PR? We could disable rerunning cmake in the tests/ subdirectory automatically (only on setup_tests) to speed up things. Otherwise I think this is good to go.

@bangerth
Copy link
Contributor

Oh, I missed that you had made the requested changes.

Let's give it a shot!

@bangerth bangerth merged commit f9902bb into geodynamics:master May 16, 2017
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

2 participants