Prevent users from misusing CMake #5782

hannes-steffenhagen-diffblue · 2021-01-25T15:24:59Z

When CMake is invoked from the source directory it will overwrite files
in there. In general CMake expects to be used for out-of-source builds.
Previously it was still possible to get this wrong by accident, this
just adds a check to prevent this from happening and tells people what
to do instead.

Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
White-space or formatting changes outside the feature-related changed lines are in commits of their own.

codecov · 2021-01-25T16:04:55Z

Codecov Report

Merging #5782 (e832c07) into develop (5efb6ad) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #5782   +/-   ##
========================================
  Coverage    69.64%   69.64%           
========================================
  Files         1248     1248           
  Lines       100920   100920           
========================================
  Hits         70290    70290           
  Misses       30630    30630

Flag	Coverage Δ
cproversmt2	`43.36% <ø> (ø)`
regression	`66.62% <ø> (ø)`
unit	`32.19% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5efb6ad...e832c07. Read the comment docs.

tautschnig

Thank you for adding those changes (and @TGWDB thank you for raising this topic!) The wording could do with some tweaking as indicated by my comments below.

tautschnig · 2021-01-25T16:35:07Z

CMakeLists.txt

+if("${CMAKE_BINARY_DIR}" STREQUAL "${CMAKE_SOURCE_DIR}")
+  message(FATAL_ERROR
+    "Do not generate CMake files inside of the cbmc source directory."
+    " Please either create a separate build directory and invoke cbmc from there, or specify a build directory with -B."


s/cbmc/cmake/ ?

tautschnig · 2021-01-25T16:35:21Z

CMakeLists.txt

@@ -8,6 +16,7 @@ if (NOT EXISTS ${CMAKE_BINARY_DIR}/CMakeCache.txt)
    endif()
 endif()

+


Necessary change?

tautschnig · 2021-01-25T16:35:45Z

CMakeLists.txt

@@ -1,5 +1,13 @@
 cmake_minimum_required(VERSION 3.2)

+# If cbmc generates files inside of the cbmc source directory this can lead to important files being overwritten, so we prevent that here


Why does "cbmc" generate anything (at build time!)?

tautschnig · 2021-01-25T16:37:44Z

CMakeLists.txt

+# If cbmc generates files inside of the cbmc source directory this can lead to important files being overwritten, so we prevent that here
+if("${CMAKE_BINARY_DIR}" STREQUAL "${CMAKE_SOURCE_DIR}")
+  message(FATAL_ERROR
+    "Do not generate CMake files inside of the cbmc source directory."


I'm not sure about the wording here: you are talking to the user here, who just invoked cmake - they didn't actually (intend to) create any files at all, and might thus be confused by this error message.

When CMake is invoked from the source directory it will overwrite files in there. In general CMake expects to be used for out-of-source builds. Previously it was still possible to get this wrong by accident, this just adds a check to prevent this from happening and tells people what to do instead.

hannes-steffenhagen-diffblue · 2021-01-26T14:26:19Z

@tautschnig I updated the wording

tautschnig · 2021-01-26T14:39:01Z

@tautschnig I updated the wording

That's great, thank you very much!

piotr-grabalski · 2021-01-26T14:39:38Z

CMakeLists.txt

+    " specifying a build directory.  Since this can lead to cmake overwriting"
+    " files in the source tree, this is prohibited.  Please perform an"
+    " out-of-source build by either creating a separate build directory and"
+    " invoking cmake from there, or specifying a build directory with -B.\n"


Maybe add 'flag' or 'option' after -B?

..., or specifying a build directory using -B option.

TGWDB · 2021-01-26T14:45:53Z

Looks fine for checking the single top level directory, do we care about mistakes related to subdirectories as well?
tgw@DB0907:~/github/hannes/cbmc$ cmake . -Bsrc
will continue fine and only over-write a couple of files.

hannes-steffenhagen-diffblue · 2021-01-26T14:47:54Z

@TGWDB I'm trying to prevent accidental misuse, it seems unlikely to me that someone would do that by accident?

TGWDB · 2021-01-26T14:51:40Z

@hannes-steffenhagen-diffblue Probably not a concern then, there is always a limit to how much safety/checking we can do.

NlightNFotis

LGTM, only one question, why are there 2 spaces in some lines (for example line 10)?

tautschnig · 2021-01-26T15:52:52Z

@hannes-steffenhagen-diffblue Probably not a concern then, there is always a limit to how much safety/checking we can do.

But perhaps that's still easy to add by replacing the STREQUAL by a prefix check?

TGWDB · 2021-01-26T15:59:04Z

@hannes-steffenhagen-diffblue Probably not a concern then, there is always a limit to how much safety/checking we can do.

But perhaps that's still easy to add by replacing the STREQUAL by a prefix check?

@tautschnig Except that this then prevents building in a subdirectory called "build" since the prefix will match.

tautschnig · 2021-01-26T16:48:02Z

@hannes-steffenhagen-diffblue Probably not a concern then, there is always a limit to how much safety/checking we can do.

But perhaps that's still easy to add by replacing the STREQUAL by a prefix check?

@tautschnig Except that this then prevents building in a subdirectory called "build" since the prefix will match.

Err, of course true. Let's stick with the current approach for now.

hannes-steffenhagen-diffblue force-pushed the cmake/prevent-misuse branch from 64e2bae to 4e8b4c7 Compare January 25, 2021 15:26

hannes-steffenhagen-diffblue mentioned this pull request Jan 25, 2021

Add regression test information to COMPILING.md #5781

Merged

7 tasks

tautschnig approved these changes Jan 25, 2021

View reviewed changes

hannes-steffenhagen-diffblue force-pushed the cmake/prevent-misuse branch from 4e8b4c7 to e832c07 Compare January 26, 2021 14:26

piotr-grabalski reviewed Jan 26, 2021

View reviewed changes

NlightNFotis approved these changes Jan 26, 2021

View reviewed changes

tautschnig merged commit f2fafac into diffblue:develop Jan 26, 2021

		@@ -8,6 +16,7 @@ if (NOT EXISTS ${CMAKE_BINARY_DIR}/CMakeCache.txt)
		endif()
		endif()

		@@ -1,5 +1,13 @@
		cmake_minimum_required(VERSION 3.2)

		# If cbmc generates files inside of the cbmc source directory this can lead to important files being overwritten, so we prevent that here

Prevent users from misusing CMake #5782

Prevent users from misusing CMake #5782

Uh oh!

Conversation

hannes-steffenhagen-diffblue commented Jan 25, 2021

Uh oh!

codecov bot commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tautschnig left a comment

Choose a reason for hiding this comment

Uh oh!

tautschnig Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

tautschnig Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

tautschnig Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

tautschnig Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

hannes-steffenhagen-diffblue commented Jan 26, 2021

Uh oh!

tautschnig commented Jan 26, 2021

Uh oh!

piotr-grabalski Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

TGWDB commented Jan 26, 2021

Uh oh!

hannes-steffenhagen-diffblue commented Jan 26, 2021

Uh oh!

TGWDB commented Jan 26, 2021

Uh oh!

NlightNFotis left a comment

Choose a reason for hiding this comment

Uh oh!

tautschnig commented Jan 26, 2021

Uh oh!

TGWDB commented Jan 26, 2021

Uh oh!

tautschnig commented Jan 26, 2021

Uh oh!

Uh oh!

codecov bot commented Jan 25, 2021 •

edited

Loading