Skip to content
This repository has been archived by the owner on Sep 4, 2024. It is now read-only.

Remove git versioning (unused) #31

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

nclack
Copy link
Member

@nclack nclack commented Sep 15, 2023

In an effort to get rid of warnings like: "GIT_TAG and GIT_HASH redefined", I'm going through and fixing up git-versioning.cmake scripts.

Here though, none of the targets build by core libs use GIT_TAG or GIT_HASH so I'm removing the script.

@nclack nclack marked this pull request as ready for review September 15, 2023 18:03
Copy link
Member

@aliddell aliddell left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

What does usage of the GIT_TAG and GIT_HASH definitions look like?

One consumer was CPACK (in CPACK_PACKAGE_VERSION), but that didn't need them be compile time definitions, right? I noticed that we don't actually call cpack anywhere in this repo - does that mean we could also remove include(CPack) in the top-level CMakeLists.txt? Or do we want/need targets that produces for other reasons?

The only other consumer I found of either of these definitions is in acquire-video-runtime: https://github.com/acquire-project/acquire-video-runtime/blob/773a6f5e863e4356c2d19666b5a12fde133dbaba/src/acquire.c#L76

Could we just remove these definitions everywhere except there?

@nclack
Copy link
Member Author

nclack commented Sep 15, 2023

Could we just remove these definitions everywhere except there?

I'm inclined to update the drivers.

If we do the update, the versions get reported during the cmake build.

Also, we don't use GIT_TAG and GIT_HASH in the drivers, but we probably should - there are pretty good reasons to have drivers report their own versions to the acquire runtime for reporting purposes.

@nclack
Copy link
Member Author

nclack commented Sep 15, 2023

could also remove include(CPack)

cpack gets invoked as a command line tool. It uses a bunch of variables that get defined by that include(CPack) step. Modifying CPACK_PACKAGE_VERSION in that top level git-versioning script works because of cmake's directory scoping rules (I think). It ends up setting the version used in the name of the installer/package that cpack produces.

@nclack nclack merged commit a82da24 into acquire-project:main Sep 15, 2023
3 checks passed
@andy-sweet
Copy link
Contributor

could also remove include(CPack)

cpack gets invoked as a command line tool. It uses a bunch of variables that get defined by that include(CPack) step. Modifying CPACK_PACKAGE_VERSION in that top level git-versioning script works because of cmake's directory scoping rules (I think). It ends up setting the version used in the name of the installer/package that cpack produces.

Sure. But cpack is never invoked from the command line in this repo (e.g. by a workflow). I guess it's nice to have the option to package/install acquire-core-libs though, so no need to remove.

@nclack nclack deleted the remove-git-versioning branch September 17, 2023 23:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants