-
Notifications
You must be signed in to change notification settings - Fork 4
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
Windows build dir fix #16
base: develop
Are you sure you want to change the base?
Conversation
CMakeLists.txt
Outdated
@@ -202,7 +202,7 @@ endif () | |||
# Everything should be valid from here onwards. | |||
if (OPENCMISS_SETUP_TYPE STREQUAL "standard" AND NOT OPENCMISS_INDEPENDENT) | |||
set(OPENCMISS_MANAGE_SOURCE_DIR "${OPENCMISS_ROOT}/src/manage") | |||
set(OPENCMISS_MANAGE_BINARY_DIR "${OPENCMISS_ROOT}/build/manage${SINGLE_ARCHITECUTRE_BUILD_TYPE}") | |||
set(OPENCMISS_MANAGE_BINARY_DIR "${OPENCMISS_ROOT}/build/manage/${CMAKE_HOST_SYSTEM_NAME}${SINGLE_ARCHITECUTRE_BUILD_TYPE}") |
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.
CMAKE_SYSTEM_NAME should be used here CMAKE_HOST_SYSTEM_NAME is for cross-compiling purposes which we are not.
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.
The system name should also be separated from the architecture build type.
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.
The system name is separated. The SINGLE_ARCHITECTURE_BUILD_TYPE has a / prepended.
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.
There needs to be some documentation added around the use of the system name.
CMakeLists.txt
Outdated
@@ -202,7 +202,7 @@ endif () | |||
# Everything should be valid from here onwards. | |||
if (OPENCMISS_SETUP_TYPE STREQUAL "standard" AND NOT OPENCMISS_INDEPENDENT) | |||
set(OPENCMISS_MANAGE_SOURCE_DIR "${OPENCMISS_ROOT}/src/manage") | |||
set(OPENCMISS_MANAGE_BINARY_DIR "${OPENCMISS_ROOT}/build/manage${SINGLE_ARCHITECUTRE_BUILD_TYPE}") | |||
set(OPENCMISS_MANAGE_BINARY_DIR "${OPENCMISS_ROOT}/build/manage/${CMAKE_HOST_SYSTEM_NAME}${SINGLE_ARCHITECUTRE_BUILD_TYPE}") |
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.
Adding this prefix should be optional with the default option being not to have it.
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.
Why? It is safer to have it. It makes documenting where to go in manage much more difficult because it is there. As it there it means that files are separated so that when developers add different configs then they don't have to blow away what they have. For the amount of time people need to go to this directory the overhead of an additional tab button press is completely trivial. What is there that shouldn't be is release which is completely un-necessary and so if it is OK for that to be there for as long as it has been then this should not be a problem. For developers multi-config should be the default. Users wouldn't even go into this directory.
Splitting #2
Add in OS sep for manage. Resolves OpenCMISS/manage#84