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

Fix setting of default build type #3033

Merged
merged 2 commits into from Dec 6, 2017

Conversation

Projects
None yet
4 participants
@W4RH4WK
Copy link
Contributor

commented Nov 30, 2017

We just ran into a strange issue with the AllScale project. Investigating it, I found that setting the default build type in HPX does not work. This should be fixed by this PR.

Even if no CMAKE_BUILD_TYPE is specified by the user, CMake still creates a cache entry. This entry is empty, yet present -- therefore FORCE is required.

@sithhell

This comment has been minimized.

Copy link
Member

commented Nov 30, 2017

So, as far as I can see, this change forces the CMAKE_BUILD_TYPE into the global cache, right?
I don't think that fixes the referenced problem, it rather creates subtitle bugs by forcing a specific build type onto the parent, with might be unexpected.
Especially since in the build scripts of the allscale compiler, the caches aren't shared.
What do I miss?

@sithhell

This comment has been minimized.

Copy link
Member

commented Nov 30, 2017

I'd even be as radical as to remove this line all together. The cmake default is Debug.
We always advise our used to set the build type explicitly...

@W4RH4WK

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2017

Let me elaborate in more detail, maybe I made a mistake along the line.


By default, CMake does set CMAKE_BUILD_TYPE to empty string, not to Release, nor to Debug.
This can be seen with this example.

cmake_minimum_required(VERSION 3.5)
project(foo LANGUAGES C CXX)

message("CMake Build Type: ${CMAKE_BUILD_TYPE}")

Running CMake with this, and without stating CMAKE_BUILD_TYPE your output should look like this:

$ cmake ..
[...]
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Build Type:
-- Configuring done
-- Generating done
-- Build files have been written to: /home/w4rh4wk/Desktop/cmake_issue/default_bt/build

A look into the file CMakeCache.txt shows:

//Choose the type of build, options are: None(CMAKE_CXX_FLAGS or
// CMAKE_C_FLAGS used) Debug Release RelWithDebInfo MinSizeRel.
CMAKE_BUILD_TYPE:STRING=

Even though we did not set a value for the build type it is present in the cache.


I now add the line currently present in HPX's root CMakeLists.txt (doc string has been shortened).

cmake_minimum_required(VERSION 3.5)
project(foo LANGUAGES C CXX)

set(CMAKE_BUILD_TYPE "Release" CACHE STRING "..")

message("CMake Build Type: ${CMAKE_BUILD_TYPE}")

Turns out, I get the same output.

$ cmake ..
[...]
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Build Type:
-- Configuring done
-- Generating done
-- Build files have been written to:
 /home/w4rh4wk/Desktop/cmake_issue/flawed_set/build

This is because set() with the CACHE argument does not alter entries which are already present unless you also specify FORCE.
From the correpsonding CMake documentation:

Since cache entries are meant to provide user-settable values this does not overwrite existing cache entries by default. Use the FORCE option to overwrite existing entries.

When I add FORCE to the set() call, I get the following output:

$ cmake ..
[...]
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Build Type: Release
-- Configuring done
-- Generating done
-- Build files have been written to:
 /home/w4rh4wk/Desktop/cmake_issue/correct_set/build

Now also the cache entry looks good.

CMAKE_BUILD_TYPE:STRING=Release

This shows that the set() call inside HPX's root CMakeLists.txt does not do anything at the moment, which was probably not intended by the author.
My PR changes its behaviour to what the author (probably) wants.
This should clarify that my PR as a fix for HPX is valid.


Regarding: "[...] this change forces the CMAKE_BUILD_TYPE into the global cache [...]".

The set() call is still guarded by if(NOT CMAKE_BUILD_TYPE) which causes it to be only executed when CMAKE_BUILD_TYPE is set to something, that CMake counts as false.
And the empty string indeed evaluates to false.

Let me just add this if() to our running example.

cmake_minimum_required(VERSION 3.5)
project(foo LANGUAGES C CXX)

if(NOT CMAKE_BUILD_TYPE)
    set(CMAKE_BUILD_TYPE "Release" CACHE STRING ".." FORCE)
endif()

message("CMake Build Type: ${CMAKE_BUILD_TYPE}")

Calling CMake without any additional arguments sets the build type to Release.
Yet I can still set it to whatever I like:

$ cmake ..
[...]
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Build Type: Release
-- Configuring done
-- Generating done
-- Build files have been written to: /home/w4rh4wk/Desktop/cmake_issue/guarded_set/build

$ cmake -DCMAKE_BUILD_TYPE=Bar ..
[...]
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Build Type: Bar
-- Configuring done
-- Generating done
-- Build files have been written to: /home/w4rh4wk/Desktop/cmake_issue/guarded_set/build

The corresponding entry will end up in ${CMAKE_BINARY_DIR}/CMakeCache.txt.

I don't (yet) see a downside of doing this.
I consider it best practise to set CMake's default build type to something reasonable instead of accepting the empty string as a valid build type.


Now, how did I get here.

HPX as well as the AllScale Runtime are configured by the AllScale compiler's CMake infrastructure.
Both are configured using CMake's external project mechanism which gives each of them their own, isolated working directory.
This was chosen to minimise conflicts between the different CMake infrastructures of the AllScale Compiler and HPX + AllScale Runtime.

Before the changes mentioned in allscale/allscale_compiler#3 no build type was set, neither for HPX nor for the AllScale Runtime.
The result was that the AllScale Runtime library's output name was configured to liballscale.so.

Now with these new changes, the build type for the AllScale Runtime project is set to the same build type as used for the compiler (Release by default).
This results in the AllScale Runtime library's output name to be configured to libhpx_allscale.so.
Which is, as already mentioned, the way it should be.

After I discovered this behaviour I deduced that the add_hpx_component macro used for the AllScale Runtime library here behaves different, depending on the build type.

I then assumed that the code behind this macros (and probably others as well) depends on the correct initial setup of HPX and therefore investigated HPX's root CMakeLists.txt where I found the mentioned error, leading to this PR.


Regarding the AllScale Runtime.
At the time of submitting the PR I assumed the AllScale Runtime includes (parts of) HPX's root CMakeLists.txt to do the necessary setup for the HPX related macros to work correctly.
And from this the bug of not setting the default build type correctly is inherited.

Turns out, this assumption was wrong.
The AllScale Runtime does not include HPX's root CMakeLists.txt.
It just uses the provided macros, which seem to misbehave if no build type is set (at least this is what I think is happening).

Therefore, this PR does not fix the AllScale Runtime issue.
A possible fix would be to set the build type (if none is provided) in the AllScale Runtime CMake infrastructure before using the add_hpx_component macro -- given my assumption of this macro misbehaving (if no build type is set) is correct.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Dec 1, 2017

Thanks for your extended analysis! I think this PR is fine and we should go ahead with merging it. But @sithhell knows more about cmake and our build system than I do. I'll let him decide.

@sithhell
Copy link
Member

left a comment

The analysis is entirely correct. This PR doesn't fix any problem in particular, but leads to more consistent cache entries. Thanks!

@msimberg msimberg merged commit 60e9fa0 into STEllAR-GROUP:master Dec 6, 2017

4 of 5 checks passed

pycicle-Test errors 10
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
pycicle-Build errors 0
Details
pycicle-Config errors 0
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.