D24641: Collect more information from version control systems

[Harald Sitter]

sitter added a comment.


  This is starting to look really good. All functions will need documenting in the header of that file so they show up on api.kde.org, see other modules for examples.

INLINE COMMENTS

> ECMSourceVersionControl.cmake:61
> +
> +set(_ECM_SOURCE_VERSION_CONTROL_ALREADY_WARNED_NOT_VCS FALSE)
> +set(_ECM_SOURCE_VERSION_CONTROL_ALREADY_WARNED_MISSING_GIT_EXEC FALSE)

Do we really need this? It seems to me we could just spam it for every call, in the grand scheme of things it makes no difference but is less logic one has to worry about when extending this module.

> ECMSourceVersionControl.cmake:75
> +        # Git
> +        if(NOT _ECM_SOURCE_VERSION_GIT_EXECUTABLE)
> +            find_program(_ECM_SOURCE_VERSION_GIT_EXECUTABLE

I'd move the exec detection and missing reporting into its own helper which gets called by all "public" functions. Currently the logic is duped in both functions.

> ECMSourceVersionControl.cmake:87
> +            )
> +            set(ECM_SOURCE_VERSION_CONTROL_REVISION "${ECM_SOURCE_VERSION_CONTROL_REVISION}" PARENT_SCOPE)
> +        else()

I think it's more idiomatic if you accept the variable name as a function argument instead of hardcoding one.

For example `find_program`, but really most helpers work like that off the top of my head.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24641

To: thomasfischer, sitter, kossebau
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191115/07683112/attachment.html>