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

fix #2871 by introducing additional directories #2878

Merged
merged 5 commits into from
Nov 3, 2021
Merged

fix #2871 by introducing additional directories #2878

merged 5 commits into from
Nov 3, 2021

Conversation

shaomeng
Copy link
Collaborator

@shaomeng shaomeng commented Oct 26, 2021 •
edited

This PR fixes issue #2871 by moving 3 binaries to a directory named debug_binaries, and 7 binaries to a directory named test_binaries.

@shaomeng shaomeng changed the title fix #2871 by introducing debug_binaries directory fix #2871 by introducing additional directories Oct 26, 2021
@shaomeng
Copy link
Collaborator Author

@sgpearse this PR moves the test binaries to a different directory, so the ubuntu circleCI job fails. Mysteriously, the centos circleCI job succeeds. There might be something to investigate into.

@clyne clyne requested review from sgpearse and StasJ October 26, 2021 16:22
@sgpearse
Copy link
Collaborator

sgpearse commented Oct 26, 2021 •
edited

Hey, CentOS is passing because we only test for warnings in debug/release mode on that platform. We run smoke tests on ubuntu only.

I've fixed the path to the smoke tests so that they pass on CircleCI, submitted as a PR to this branch.

StasJ
StasJ reviewed Oct 26, 2021
View reviewed changes
Copy link
Collaborator

@StasJ StasJ left a comment

Choose a reason for hiding this comment

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

I don’t know if I’d consider vaporversion a debug binary. Also, shouldn’t the test binary location be specified as a variable rather than hard coded for every executable?

@shaomeng
Copy link
Collaborator Author

shaomeng commented Oct 26, 2021 •
edited

I don’t know if I’d consider vaporversion a debug binary.

I cannot find a good argument for one way or another. Please let me know if you find a good one.

Also, shouldn’t the test binary location be specified as a variable rather than hard coded for every executable?

Did you mean introducing a CMake configuration option that allows a user to specify where to put test binaries? I actually cannot think of a good use case where someone wants to change test binaries to a different directory.

sgpearse
sgpearse reviewed Oct 26, 2021
View reviewed changes
apps/vapor_check_udunits/CMakeLists.txt Outdated
@@ -1,4 +1,5 @@
add_executable (vapor_check_udunits vapor_check_udunits.cpp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This goes back to bin

apps/vaporpychecker/CMakeLists.txt Outdated
@@ -1,4 +1,5 @@
add_executable (vaporpychecker vaporpychecker.cpp)
set_target_properties(vaporpychecker PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/debug_binaries")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move to bin

apps/vaporversion/CMakeLists.txt Outdated
@@ -1,4 +1,5 @@
add_executable (vaporversion vaporversion.cpp)
set_target_properties(vaporversion PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/debug_binaries")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move to bin

test_apps/ParamsMgr/CMakeLists.txt Outdated
@@ -1,3 +1,4 @@
add_executable (test_ParamsMgr test_ParamsMgr.cpp)
set_target_properties(test_ParamsMgr PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/test_binaries")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Go to debug_binaries

test_apps/datamgr/CMakeLists.txt
@@ -1,3 +1,4 @@
add_executable (test_datamgr test_datamgr.cpp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move to debug_binaries

test_apps/grid_iter/CMakeLists.txt Outdated
@@ -1,3 +1,4 @@
add_executable (test_grid_iter test_grid_iter.cpp)
set_target_properties(test_grid_iter PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/test_binaries")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move to debug_binaries

test_apps/params2/CMakeLists.txt
@@ -1,3 +1,4 @@
add_executable (test_params2 test_params2.cpp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move to debug_binaries

test_apps/pyengine/CMakeLists.txt Outdated
@@ -1,3 +1,4 @@
add_executable (test_pyengine test_pyengine.cpp)
set_target_properties(test_pyengine PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/test_binaries")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move to debug_binaries

test_apps/quadtreerectangle/CMakeLists.txt
@@ -1,5 +1,6 @@

add_executable (test_quadtreerectangle "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move to debug_binaries

@sgpearse sgpearse mentioned this pull request Oct 26, 2021
Closed
@shaomeng
Copy link
Collaborator Author

All the review comments have been fixed in commit ceea4c1. Please review again.

StasJ
StasJ reviewed Oct 27, 2021
View reviewed changes
CMakeLists.txt Outdated
@@ -90,6 +90,8 @@ endif ()
set (CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set (CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
set (CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set (CMAKE_TEST_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/test_binaries)
set (CMAKE_DEBUG_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/debug_binaries)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your new variables match the naming scheme of built in CMake “magic variables” which can be confusing since they will behave differently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not familiar with this topic, so could you elaborate on what exactly the different behaviors are?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The preceding 3 variables are builtin and changing them configures cmake. The ones you added are normal variables.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

got it!

@shaomeng shaomeng requested a review from StasJ October 27, 2021 19:20
sgpearse
sgpearse approved these changes Nov 3, 2021
View reviewed changes
Copy link
Collaborator

@sgpearse sgpearse left a comment

Choose a reason for hiding this comment

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

With this PR, our build directory will have 4 places we put binaries (bin, test_apps, debug_binaries, and test_binaries). A little confusing. I think we should migrate the binaries in build/test_apps into either test_binaries or debug_binaries. But that can be done outside of this PR.

Opening #2882.

@shaomeng
Copy link
Collaborator Author

shaomeng commented Nov 3, 2021

Hey @sgpearse just to clarify, there are 3 places to put binaries. test_app does not have binaries.

@shaomeng shaomeng merged commit 15b7c55 into main Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StasJ StasJ StasJ approved these changes

@sgpearse sgpearse sgpearse approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@shaomeng @sgpearse @StasJ