-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Git info store object #3848
Git info store object #3848
Conversation
res/skins/Shade/skin.xml
Outdated
@@ -1,4 +1,5 @@ | |||
<!-- | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated change
if (gitVersion.isEmpty()) { | ||
gitVersion = QStringLiteral("unknown"); | ||
} | ||
|
||
QString gitBranch = VersionStore::gitBranch(); | ||
if (!gitBranch.isEmpty()) { | ||
gitVersion.append(QStringLiteral(" (") + gitBranch + QStringLiteral(" branch)")); | ||
gitVersion.append(QStringLiteral(" (") + gitBranch + QChar(')')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this change to make the about window a bit less wide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can even remove the branch name if it matches the version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may, but I don't think it's necessary. In any case this should happen in the cpp code, not in the CMake code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah you already did.
Some pre-commit issues. |
Done |
Still pre-commit issues. |
No idea why this passes locally the second time. :-/ |
#ifdef GIT_COMMIT_COUNT | ||
return GIT_COMMIT_COUNT; | ||
#else | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 maybe? or use std::optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 fits, because we have 0 commits if we are off the git worktree
src/util/gitinfostore.h
Outdated
static const char* branch(); | ||
static const char* describe(); | ||
static const char* date(); | ||
int commitCount(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static?
-DGIT_DESCRIBE="${GIT_DESCRIBE}" | ||
-DGIT_COMMIT_DATE="${GIT_COMMIT_DATE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about CPackConfig? I think you need to copy these variables into the CPack domain as well, and set them before including the GitInfo
module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daschuer ping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, we should allow to use the override values in cpack as well.
There are merge conflicts now. |
Done. |
-DINPUT_FILE="${CMAKE_CURRENT_SOURCE_DIR}/src/gitinfo.h.in" | ||
-DOUTPUT_FILE="${CMAKE_CURRENT_BINARY_DIR}/src/gitinfo.h" | ||
-P "${CMAKE_CURRENT_SOURCE_DIR}/cmake/scripts/gitinfo.cmake" | ||
COMMENT "Update git version information in gitinfo.h" | ||
BYPRODUCTS "${CMAKE_CURRENT_BINARY_DIR}/src/gitinfo.h" | ||
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" | ||
) | ||
add_dependencies(mixxx-lib mixxx-gitinfo) | ||
|
||
add_library(mixxx-gitinfostore STATIC EXCLUDE_FROM_ALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for making this a separate library? Why not add it to mixxx-lib instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve build time. Re-linking it to mixxx-lib takes long, even if just the git info changes.
From this point of view it is better to have more lbs than less. This will also reduce the memory requirements on Windows.
Building a local branch fails now:
|
Do we have updated build instructions for this use case?? |
This is what I have added to my build script:
|
@uklotzde can you run the git commit date command directly and post the output? |
|
Ah, please use |
Is this for Fedora? If you're building from the git tree setting those variables locally should not be necessary at all. |
The We need to build from a plain file archive, no Git repo available. I added them explicitly for continuously testing the build. Just to notice early when the build breaks because of invalid assumptions. |
Ok, I copied the new variables and their commands literally. |
This adds a git Info store project to speed up building when only the git info is updated.
This uses also the build date in case of tar ball builds unless a GIT_COMMIT_DATE is passed to cmake along with a GIT _DESCRIBE