CMake: use git rev-parse to get GIT_COMMIT_HASH #1185
Conversation
|
|
|
Note: I don't use CMake much so this might not be the most sensible approach to this. I'm also not certain at which cmake version this was added so I'm not sure if |
The old approach of reading .git/HEAD does not work when using git
worktrees, where the folder layout looks roughly like:
solvespace.git/ - bare clone (.git dir)
solvespace.git/work - example worktree containing master
solvespage.git/worktrees/work/ - .git dir of worktree
solvespage.git/worktrees/work/HEAD - actual HEAD ref for master
First attempt was to just get GIT_ROOT from `git rev-parse --git-dir` but
that wasn't enough, since:
1. GIT_ROOT points to solvespage.git/worktrees/work/
2. GIT_ROOT/HEAD points to refs/heads/master
3. GIT_ROOT/refs/heads/master does not exist but the old implementation
would want to use this to get the sha
so we need two invocations of git rev-parse
1. `git rev-parse --git-dir` to get GIT_DIR
needed for setting GIT_DEPENDS
2. `git rev-parse HEAD` to get the sha of the worktree's HEAD
herrgahr
force-pushed
the
cmake-git-hash
branch
from
18b6b9f
to
83480b9
Compare
| set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS ${GIT_DEPENDS}) | ||
|
|
||
| string(STRIP ${HEAD_REF} HEAD_REF) |
There was a problem hiding this comment.
this is redundant since execute_process's OUTPUT_STRIP_TRAILING_WHITESPACE option takes care of trailing whitespace and the output of git rev-parse doesn't containg leading whitespace. But: if git rev-parse fails, ${HEAD_REF} is undefined and cmake gives a confusing error message regarding the number of arguments give to strip
|
Oohhh, I wasn't aware that this problem was being discussed there, the title didn't give it away ;) This was the typical drive-by fix, I just happened to check out the sources today after a looong time I already suspected rummaging through .git folder and HEAD files manually was for edge cases where git is not installed or somesuch. So I guess it makes sense to close this in favor of integrating @rpavlik 's CMake magic as it seems to cover all bases. Either way, I'd be a happy camper if the build worked with worktrees :) Should I close the PR and open an issue for this? |
I'd say leave the PR open for now, but it's up to @phkahler ultimately. And I'm not at all proficient with CMake to say whether Rpavlik's solution works with worktrees :-) |
|
I'm pretty sure my module works with work trees, but not 100% sure. Welcome! |
No @Symbian9 it does not violate the GPL. However we do not require it anymore, the code shall stay GPLv3+ |
@phkahler, It is violate in case that any changed file licensed under GPL (SolveSpace source actually GPL, not double licensed) could NOT be double licensed as stated in CLA Assistant: I already highlighted it here: #1098 (comment) So, as in actual pull request proposed changes to already GPL-licensed @jwesthues already clarified in 2fcc933 that GPL is a single license for contributing to SolveSpace repo:
And as result any pull request which authors signed CLA Assistant Agreement highly likely could NOT be merged into SolveSpace repo at all as such "double re-licensing" mentioned in signed CLA fully violates SolveSpace GPL-licensing. |
Oh man, don't leave stuff like that to me! If the @rpavlik solution does the job, lets go with that. Otherwise @ruevs you can merge this PR ;-) |
|
On second thought, this is a fix and simplification that is available now. If it gets supplanted later that's fine too. |
|
@ruevs so I guess I'll revert it tonight. |
The old approach of reading .git/HEAD does not work when using git worktrees, where the folder layout looks roughly like:
First attempt was to just get GIT_ROOT from
git rev-parse --git-dirbut that wasn't enough, since:so we need two invocations of git rev-parse
git rev-parse --git-dirto get GIT_DIR needed for setting GIT_DEPENDSgit rev-parse HEADto get the sha of the worktree's HEAD