ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.21k stars 391 forks source link

PROJECT_IS_TOP_LEVEL used before project() #968

Open rleigh-electradx opened 2 weeks ago

rleigh-electradx commented 2 weeks ago

In the top-level CMakeLists.txt:

set(MSG_PREFIX "etl |")
determine_version_with_git(${GIT_DIR_LOOKUP_POLICY})
if(NOT ETL_VERSION)
    determine_version_with_file("version.txt")
endif()

project(etl VERSION ${ETL_VERSION} LANGUAGES CXX)

And in determine_version_with_git():

if(PROJECT_IS_TOP_LEVEL)

There is unfortunately a bit of a chicken-and-egg problem here. determine_version_with_git() is called before project(), and so PROJECT_IS_TOP_LEVEL is TRUE even when it should be FALSE. For example, when included from another project which is the real top-level. This leads to a warning

message(WARNING "Version string ${VERSION} retrieved with git describe is invalid")

when it should have fallen back on using the version file silently.

PROJECT_IS_TOP_LEVEL should only be used after your project() call, otherwise it is inheriting the state from the project which is including your CMakeLists.txt as a subdirectory, and that may be TRUE or FALSE depending upon whether the project adding you is the top-level or not. It's not useful in determining whether you are the top-level or not.

Suggestion:

Compare CMAKE_CURRENT_LIST_DIR with PROJECT_SOURCE_DIR before the project() call. It will never be true unless you are the real top-level, for example:

set(ETL_IS_TOP_LEVEL FALSE)
if(CMAKE_CURRENT_LIST_DIR STREQUAL CMAKE_CURRENT_LIST_DIR)
  set(ETL_IS_TOP_LEVEL TRUE)
endif()

and then use ETL_IS_TOP_LEVEL in place of PROJECT_IS_TOP_LEVEL in determine_version_with_git().

Kind regards, Roger

jwellbelove commented 1 week ago

I'm not in anyway an expert with CMake and my free time is a bit limited at the moment. Would you be able to do this as a pull request?

rleigh-electradx commented 1 week ago

@jwellbelove Absolutely no problem. I'll get this opened soon.