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

[build-script] Clear CMakeCache.txt before (re)configuration. #2226

Merged
merged 1 commit into from
Apr 18, 2016

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Apr 17, 2016

What's in this pull request?

Since cmake doesn't clear the cached values,
We should remove CMakeCache.txt before re-configuration.
(or explicitly unset cached values using -U)

For example:

build$ cat ../CMakeLists.txt

set(MY_OPTION "==DEFAULT==" CACHE STRING "My Option Value")
message("MyOption: ${MY_OPTION}")

build$ cmake -G Ninja ../ -DMY_OPTION=FOOBAR
...
MyOption: FOOBAR
...

build$ cmake -G Ninja ../  # <- This is the problem
...
MyOption: FOOBAR
...

build$ cmake -G Ninja ../ -UMY_OPTION
...
MyOption: ==DEFAULT==
...

In build-script-impl, compare:

# OK
cmake_options+=(
    -DSWIFT_MY_FLAG:BOOL=$(true_false "${MY_FLAG}")
)

# NOT OK
if [[ "${MY_VALUE}" ]]; then
    cmake_options+=(
        -DSWIFT_MY_VALUE:STRING="${MY_VALUE}"
    )
fi

In the latter case, we should explicitly specify -USWIFT_MY_VALUE in else branch to use the default value.
But it's very annoying to apply that for every options.
More easy solution is to clear the cache by deleting CMakeCache.txt


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

set -x
mkdir -p "${build_dir}"
rm -f "${cmake_cache_path}"
(cd "${build_dir}" && "${CMAKE}" "${cmake_options[@]}" ${USER_CONFIG_ARGS})
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved mkdir inside -x block because, I think, any operation that might mutates the file system should be visible from us.

@gribozavr
Copy link
Contributor

@rintaro Thank you, this looks like a good idea to me!

Do you know if this is the recommended way to clear configuration for CMake? Does it have any bad effects? Does it still allow incremental builds?

@rintaro
Copy link
Member Author

rintaro commented Apr 18, 2016

Do you know if this is the recommended way to clear configuration for CMake?

I don't know for "recommended way to clear configuration", but in general, hand-editing or deleting CMakeCache.txt is supported way.
https://cmake.org/runningcmake/:

After CMake has been run, and created a CMakeCache.txt file – you may edit it. The CMake GUI, will allow you to edit the options easily, or you can edit the file directly.

If your CMakeLists.txt files change significantly, you will need to remove the relevant entries from the cache file. If you have not already hand-edited the cache file, you could just delete it before re-running CMake.

Does it have any bad effects?

It may. Inside CMakeLists.txt, consider:

if(MY_OPTION STREQUAL "")
  execute_process(
    ... Do Something with side effects ...)
endif()

Obviously, this kind of problem should be fixed in CMakeLists.txt.

Since it's supported way, I believe cmake itself or core components must not have these kind of problem.

Does it still allow incremental builds?

Sorry, but I don't know for now.

@gribozavr
Copy link
Contributor

https://cmake.org/runningcmake/

Thank you, this is exactly what I was looking for! So the developers are recommending deleting the file, we should be good.

Does it still allow incremental builds?

Sorry, but I don't know for now.

I just ran a quick test on the master branch:

$ ./utils/build-script -R
[everything built]
$ ./utils/build-script -R
[nothing was rebuilt]
$ rm ../build/swift-macosx-x86_64/CMakeCache.txt
$ ./utils/build-script -R
[nothing was rebuilt]

So we should be good.

@gribozavr
Copy link
Contributor

@swift-ci Please test and merge

@gribozavr
Copy link
Contributor

The test failure is unrelated, merging.

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.

2 participants