Skip to content

Commit

Permalink
Fixes #364.
Browse files Browse the repository at this point in the history 
Made some changes to the InSourceBuild CMake module in an attempt
to prevent CMake-challenged individuals from trashing the source
tree when performing a build.
  • Loading branch information
@mdadams
mdadams committed Nov 28, 2023
1 parent 175731c commit a798ba0
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 7 deletions.
4 changes: 3 additions & 1 deletion CMakeLists.txt
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
cmake_minimum_required(VERSION 3.12)
cmake_minimum_required(VERSION 3.20)
# Version 3.20 needed for cmake_path.
# Version 3.19 needed for file(REAL_PATH ...).
# Version 3.12 needed for FindJPEG imported targets.
# Version 3.10 needed for FindOpenGL imported targets.
# Version 3.1 needed for FindGLUT imported targets.
Expand Down
28 changes: 22 additions & 6 deletions build/cmake/modules/InSourceBuild.cmake
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,31 @@
option(ALLOW_IN_SOURCE_BUILD "Allow an in-source build" OFF)

function(prevent_in_source_build)
get_filename_component(source_dir "${CMAKE_SOURCE_DIR}" REALPATH)
get_filename_component(binary_dir "${CMAKE_BINARY_DIR}" REALPATH)
if(source_dir STREQUAL binary_dir)

# Determine if an in-source build is in progress.
file(REAL_PATH "${CMAKE_SOURCE_DIR}" source_dir)
file(REAL_PATH "${CMAKE_BINARY_DIR}" binary_dir)
cmake_path(IS_PREFIX source_dir "${binary_dir}" result)
cmake_path(GET binary_dir FILENAME binary_dir_base)

# If an in-source build is in progress, and the build directory is not
# chosen in a very specific way, then stop the build.
if(result AND (NOT (binary_dir_base MATCHES "tmp*")))
message(FATAL_ERROR
"The use of an in-source build is not recommended. "
"For this reason, the use of in-source build is disabled by default. "
"If you want to override this default behavior, add the -DALLOW_IN_SOURCE_BUILD option to cmake."
"The use of an in-source build has been detected "
"(i.e., the binary directory specified to CMake is located "
"in or under the source directory). "
"This can potentially trash the source tree. "
"In fact, if you are seeing this message, you may have already "
"partially trashed the source tree. "
"The use of an in-source build is not officially supported and "
"is therefore disallowed by default. "
"If you like to live dangerously and would like to override "
"this default behavior, this can be accomplished via the "
"CMake option ALLOW_IN_SOURCE_BUILD."
)
endif()

endfunction()

if(NOT ALLOW_IN_SOURCE_BUILD)
Expand Down

5 comments on commit a798ba0

@heitbaum
Copy link

Choose a reason for hiding this comment

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

Feedback on this change - we use the following e.g. subdirectories in source .x86_64-libreelec-linux-gnu not tmp, so have had to use the -DALLOW_IN_SOURCE_BUILD=ON option

@mdadams
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@heitbaum Your usage of ALLOW_IN_SOURCE_BUILD is fine. I don't mean to scare you with the error message that CMake prints if you attempt an in-source build. In-source builds can be quite convenient to use in some workflows. The error message about in-source builds is meant to scare people who do not really know what they are doing. The reason for making in-source builds disallowed by default is to address the growing problem that many people who do not understand CMake are doing in-source builds where the CMake binary directory is chosen to match an already existing directory in the JasPer source tree. This, results in many exotic build failures (due to trashing the JasPer source tree) that are entirely the fault of the person using CMake and not JasPer. But these users file bug reports against JasPer anyways, which is a big hassle to deal with. So, I am hoping that this policy change for in-source builds will eliminate (or at least reduce) this kind of bogus bug report. I hope that this rationale makes sense. I have no intention to completely prevent the use of in-source builds. I just want to discourage people who do not know what they are doing from using them.

@heitbaum
Copy link

Choose a reason for hiding this comment

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

@heitbaum Your usage of ALLOW_IN_SOURCE_BUILD is fine. I don't mean to scare you with the error message that CMake prints if you attempt an in-source build. In-source builds can be quite convenient to use in some workflows. The error message about in-source builds is meant to scare people who do not really know what they are doing. The reason for making in-source builds disallowed by default is to address the growing problem that many people who do not understand CMake are doing in-source builds where the CMake binary directory is chosen to match an already existing directory in the JasPer source tree. This, results in many exotic build failures (due to trashing the JasPer source tree) that are entirely the fault of the person using CMake and not JasPer. But these users file bug reports against JasPer anyways, which is a big hassle to deal with. So, I am hoping that this policy change for in-source builds will eliminate (or at least reduce) this kind of bogus bug report. I hope that this rationale makes sense. I have no intention to completely prevent the use of in-source builds. I just want to discourage people who do not know what they are doing from using them.

all good - fully supportive on reducing the tickets. Just thought I would share our "tmp" path logic with you too. It was a simple change for our tooling and "hopefully" we understand the ramifications :-)

@mdadams
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Incidentally, thank you for providing feedback on this change. It is appreciated. Just to be clear, the solution I would advocate for your case is what you are already doing, namely, using the ALLOW_IN_SOURCE_BUILD variable. I prefer not to add more regexes in the InSourceBuild CMake module for exceptions to the "no in-source build by default" policy. I simply allow "tmp*" in the top-level directory of the source tree as an exception. Adding more special cases does not scale well. Just as a point of clarification, there should not be any negative consequences to you using ALLOW_IN_SOURCE_BUILD, at least as far as JasPer is concerned and I cannot imagine that there would be any negative consequences on your side either (aside from having to add the ALLOW_IN_SOURCE_BUILD variable to your build workflow in the first place).

@jubalh
Copy link
Member

@jubalh jubalh commented on a798ba0 Nov 30, 2023

Choose a reason for hiding this comment

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

fully supportive on reducing the tickets

I still would be happy if people would just search existing bugs before opening a new one :)

Please sign in to comment.